-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sdk processor test #545
base: main
Are you sure you want to change the base?
sdk processor test #545
Conversation
f2383ea
to
60f22b2
Compare
review as i am investigating the test issue. |
} | ||
|
||
/// Helper function to initialize the Events Processor. | ||
async fn init_events_processor(db_url: String) -> EventsProcessor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want to explore some generic solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a mapping between processor -> config and I wonder if we can reuse it here to know which config each processor uses to instantiate the default of that config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are thinking of that for SDK migration, I think we can reuse that mapping for us.
but i don't think that we can support that for the custom processors.
bbd2f6f
to
a240e78
Compare
|
||
// Example implementation for the EventsProcessor | ||
pub struct EventsProcessorTestHelper; | ||
|
||
impl ProcessorTestHelper for EventsProcessorTestHelper { | ||
fn load_data(&self, conn: &mut PgConnection, txn_version: &str) -> Result<Value> { | ||
let events_res = diesel::sql_query("SELECT * FROM events") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sycned offline: we don't need this trait and we can use test helper functions instead!
} | ||
|
||
/// Helper function to initialize the Events Processor. | ||
async fn init_events_processor(db_url: String) -> EventsProcessor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a mapping between processor -> config and I wonder if we can reuse it here to know which config each processor uses to instantiate the default of that config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should we move this to a testing-framework
crate in SDK?
yeah, should we leave the current implementation in the processor repo as it is for now. and update the tests once we start migrating? |
Yeah we can leave it here for now, but let's move this PR over to the SDK! |
rust/Cargo.toml
Outdated
aptos-protos = { git = "https://github.com/aptos-labs/aptos-core.git", rev = "5c48aee129b5a141be2792ffa3d9bd0a1a61c9cb" } | ||
aptos-system-utils = { git = "https://github.com/aptos-labs/aptos-core.git", rev = "202bdccff2b2d333a385ae86a4fcf23e89da9f62" } | ||
aptos-indexer-test-transactions = { git = "https://github.com/aptos-labs/aptos-core.git", rev = "7246f0536d599789d7dd5e9fb776554abbe11eac" } | ||
aptos-indexer-test-transactions = { git = "https://github.com/aptos-labs/aptos-core.git", rev = "c583502c69529fddbbf7e47bf920f1dc60e71b72" } | ||
testing-framework = { git = "https://github.com/aptos-labs/aptos-indexer-processor-sdk.git", rev = "ba5e221e9a0990345287155e3ce3f8d5aea513af" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefix this with aptos-indexer?
move|db_url| { | ||
// Custom validation logic | ||
let mut conn = PgConnection::establish(&db_url).expect("Failed to establish DB connection"); | ||
let test_helper = Box::new(EventsProcessorTestHelper) as Box<dyn ProcessorTestHelper>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test helper trait necessary? I feel like we could achieve the same thing with a function, something like load_events_with_version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh this isn't required at all, we need to tune this for our internal tests
c6071f0
to
d719d08
Compare
println!("Custom arguments: {:?}", custom_args); | ||
|
||
// Manually parse the "--diff" flag | ||
let diff_flag = custom_args.contains(&"--diff".to_string()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will probably change it to --generate-output or somethign else. current one is misleading. we are not diffing but generating
d719d08
to
f3acc80
Compare
No description provided.