Update SDK Tests using the latest change in mock grpc #559

yuunlimm · 2024-10-21T15:29:55Z

Description

update tests based on the latest change in sdk testing framework where it stubs out the txn version in mock grpc to avoid having gaps.

TODO:

naming structure of files for diffing all txns, whether we keep using the original version as the folder name(preferred) or not.

Test Plan

yuunlimm · 2024-10-21T15:30:11Z

add test coverage CI #557
add db output diff tests for existing processors #556
Update SDK Tests using the latest change in mock grpc #559 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @yuunlimm and the rest of your teammates on Graphite

rtso · 2024-10-21T16:14:13Z

rust/integration-tests/src/sdk_tests/events_processor_tests.rs

-            .first()
-            .expect("No transactions found")
-            .version;
+        let starting_version = 1;


Having user define starting version themself might get confusing, how do they know to always start at 1? We can have a function that takes in the vec of transactions they want to test and returns a TransactionStreamConfig already set up with the right configuration that will work with the mock grpc.

yeah that makes sense, I also thought it would be a devEx gap. then, it would also makes more sense to name the folder with original version for us too, when the custom file name isn't specified.

one downside of stubbing out txn_versions would confuse users when they write their validation logic. we would have to tell them to use version 1, 2, 3, to query db.

hmm what if instead of stubbing, the mock grpc just returns the transactions sorted in order of version?

so then user uses [txn3, txn1, txn5] as an input, then grpc retruns [txn1, txn3, txn5]?

Yeah exactly. I guess we have to choose between

whether mock grpc allows gaps within the transaction batches

stubbing out transaction versions -> you lose contextual information about the transaction

require that scenario test txn's have no gaps (this is hard since there's no guarantee when submitting txn's that there won't be a gap)

Taking a step back, I think the goal here is to be able to test multiple txn's in a single test, so I think #1 could work here. The risk I see with #1 is if the user is writing a multi-txn test and their processor panics if it sees gaps within a txn batch. I think this is a minimal risk; as a mitigation, the user could always fallback to using single-txn test case.

yeah the goal of this test is to processors not to test grpc, so it makes sense to allow mock grpc with gaps.

rtso · 2024-10-21T16:15:27Z

rust/integration-tests/src/sdk_tests/events_processor_tests.rs

+        pub const ALL_IMPORTED_TESTNET_TXNS: &[&[u8]] = &[
+            IMPORTED_TESTNET_TXNS_1_GENESIS,
+            IMPORTED_TESTNET_TXNS_2_NEW_BLOCK_EVENT,
+            IMPORTED_TESTNET_TXNS_3_EMPTY_TXN,
+            IMPORTED_TESTNET_TXNS_278556781_V1_COIN_REGISTER_FA_METADATA,
+            IMPORTED_TESTNET_TXNS_1255836496_V2_FA_METADATA_,
+            IMPORTED_TESTNET_TXNS_5523474016_VALIDATOR_TXN,
+            IMPORTED_TESTNET_TXNS_5979639459_COIN_REGISTER,
+            IMPORTED_TESTNET_TXNS_5992795934_FA_ACTIVITIES,
+        ];


For the intentional tests, what are your thoughts on splitting these transactions up into separate test cases? Right now the test name is testnet_events_processor_db_output_diff_test but we could make it more contextual and include what scenario it's actually testing.

yeah i can split this into a separate test case.

rtso · 2024-10-21T16:16:48Z

rust/integration-tests/src/sdk_tests/mod.rs


 #[cfg(test)]
 pub mod events_processor_tests;
 #[cfg(test)]
 pub mod fungible_asset_processor_tests;

 #[allow(dead_code)]
-pub const DEFAULT_OUTPUT_FOLDER: &str = "sdk_expected_db_output_files/";
+pub const DEFAULT_OUTPUT_FOLDER: &str = "sdk_expected_db_output_files2";


Did you mean to change this?

I wanted to show what the new folder structure would look like so that we can compare. However, since we are going to keep the original version as a folder name, you can ignore this!

rtso · 2024-10-21T16:19:00Z

..._expected_db_output_files2/imported_testnet_txns/fungible_asset_processor/8/coin_supply.json

@@ -1,10 +1,10 @@
 [
  {
-    "transaction_version": 21,
+    "transaction_version": 8,


Maybe we even stub out transaction_version, transaction_timestamp, and transaction_epoch, since these are not relevant to the test. Can add as a TODO

yeah wanted to hear your opinion on these, wasn't sure if they would really care about those.

I think your point of using the txn_version to debug a failed test is valid, so we should probably keep for now.

rtso

Minor comments, stamping to unblock!

yuunlimm and others added 2 commits October 21, 2024 08:27

Add Sdk tests (#554)

aa631b3

update manipulated version

33bfa81

yuunlimm marked this pull request as ready for review October 21, 2024 15:33

yuunlimm requested review from rtso and a team October 21, 2024 15:33

yuunlimm changed the title ~~Add Sdk tests (#554)~~ Update SDK Tests using the latest change in mock grpc Oct 21, 2024

This was referenced Oct 21, 2024

add db output diff tests for existing processors #556

Merged

add test coverage CI #557

Closed

rtso reviewed Oct 21, 2024

View reviewed changes

rtso approved these changes Oct 21, 2024

View reviewed changes

yuunlimm mentioned this pull request Oct 21, 2024

abstract stream config setup aptos-labs/aptos-indexer-processor-sdk#74

Merged

yuunlimm added 2 commits October 21, 2024 12:59

split tests

53a8b09

update tests

264877c

yuunlimm force-pushed the 10-16-support_multi_txns_testing branch from 519dbe3 to 264877c Compare October 22, 2024 18:08

yuunlimm merged commit e825969 into main Oct 23, 2024
7 checks passed

yuunlimm deleted the 10-16-support_multi_txns_testing branch October 23, 2024 00:11

yuunlimm restored the 10-16-support_multi_txns_testing branch October 24, 2024 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SDK Tests using the latest change in mock grpc #559

Update SDK Tests using the latest change in mock grpc #559

yuunlimm commented Oct 21, 2024 •

edited

Loading

yuunlimm commented Oct 21, 2024 •

edited

Loading

rtso Oct 21, 2024

yuunlimm Oct 21, 2024

yuunlimm Oct 21, 2024

rtso Oct 21, 2024

yuunlimm Oct 21, 2024

rtso Oct 21, 2024 •

edited

Loading

yuunlimm Oct 21, 2024

rtso Oct 21, 2024

yuunlimm Oct 21, 2024

rtso Oct 21, 2024

yuunlimm Oct 21, 2024

rtso Oct 21, 2024

yuunlimm Oct 21, 2024

rtso Oct 21, 2024

rtso left a comment

Update SDK Tests using the latest change in mock grpc #559

Update SDK Tests using the latest change in mock grpc #559

Conversation

yuunlimm commented Oct 21, 2024 • edited Loading

Description

Test Plan

yuunlimm commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtso Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtso left a comment

Choose a reason for hiding this comment

yuunlimm commented Oct 21, 2024 •

edited

Loading

yuunlimm commented Oct 21, 2024 •

edited

Loading

rtso Oct 21, 2024 •

edited

Loading