Skip to content
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

- support querying and validating multiple tables for existing proces… #550

Closed
wants to merge 6 commits into from

Conversation

yuunlimm
Copy link
Contributor

@yuunlimm yuunlimm commented Oct 15, 2024

Description

  • support querying and validating multiple tables for existing processors.
  • added more tables handled by processors
  • add support cli args

https://github.com/aptos-labs/aptos-indexer-processors/pull/545/files Part 1

…sors.

- added more tables handled by processors
- add support cli args
@yuunlimm yuunlimm marked this pull request as ready for review October 15, 2024 20:54
@yuunlimm yuunlimm force-pushed the yuunlimm/update_existing_test branch from a9a7351 to 60c90da Compare October 15, 2024 20:59
@yuunlimm yuunlimm requested a review from a team October 15, 2024 21:02
}

#[allow(dead_code)]
pub fn parse_test_args() -> TestArgs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this helpfully complain if insufficient/bad args are provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing is that we would have to passing addition -- to bypass failing unrecognized argument from cargo test

1. cargo test
2. cargo test diff_tests
3. cargo test diff_tests -- --nocapture --generate-output
4. cargo test diff_tests -- -- --generate-output <-- like this 
5. cargo test -- --nocapture

@yuunlimm
Copy link
Contributor Author

test passed locally.

test diff_tests::all_tests::test::test_all_scripted_txns_schema_output_for_all_processors ... ok
test diff_tests::all_tests::test::test_all_testnet_txns_schema_output_for_all_processors ... ok
test diff_tests::all_tests::test::test_all_mainnet_txns_schema_output_for_all_processors ... ok 

@yuunlimm yuunlimm force-pushed the yuunlimm/update_existing_test branch from 00e3be6 to b796b33 Compare October 16, 2024 18:59
@@ -25,25 +25,37 @@ pub fn remove_transaction_timestamp(value: &mut Value) {
}

#[allow(dead_code)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we put all these in a mod tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this prevents us from importing : aptos-labs/aptos-indexer-processor-sdk#73

(test_args.generate_output, test_args.output_path)
}

pub fn parse_test_args() -> TestArgs {
Copy link
Collaborator

@rtso rtso Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also reuse the function defined in the SDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which function are you referring to?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I thought parse_test_args is part of the SDK. I feel like it should be part of the testing framework SDK, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah also thought about that whether we should let them decide to use their own way of passing those args (generate-files and custom_path), b/c they might want more customzied way. but we can expose first and gather their feedback

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I prefer us to standardize the test tooling and establish consistency in structuring / running tests. If we allow too much customization, it might be confusing and too much extra work. The only parts users should provide are the processor config and the db outputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuunlimm yuunlimm force-pushed the yuunlimm/update_existing_test branch from 35c113e to 1f5523a Compare October 16, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants