[transaction test] refactoring publishing flow to test_context #6529

angieyth · 2023-02-07T19:47:18Z

Description

In this PR I move some helper functions from resource_groups.rs to text_context.rs to be shared forward.

Test Plan

Test 2 modified tests to make sure they work as before
also add RUST_MIN_STACK=4297152 in the command to avoid the stack overflow error
RUST_MIN_STACK=4297152 cargo test test_read_resource_group
RUST_MIN_STACK=4297152 cargo test test_get_txn_execute_failed_by_invalid_entry_function

Here's the list of the current tests in transaction_tests.rs
https://www.notion.so/aptoslabs/2eb26689160a46379ebe0ab6327679ac?v=27ededd87b5d4e328d42c9cf8eab6dc6

api/src/tests/resource_groups.rs

api/src/tests/transactions_test.rs

api/test-context/src/test_context.rs

movekevin · 2023-02-09T09:59:55Z

api/src/tests/transactions_test.rs

@@ -815,38 +817,35 @@ async fn test_get_txn_execute_failed_by_entry_function_invalid_function_name() {
    .await;
 }

-#[ignore] // Re-enable when change is moved to new publish flow
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn test_get_txn_execute_failed_by_entry_function_execution_failure() {


Are you planning to update other module publishing tests in this same PR or a separate one?

I'll do that in the following PR.

...sts__transactions_test__test_get_txn_execute_failed_by_entry_function_execution_failure.json

movekevin · 2023-02-09T10:01:21Z

Cargo.lock

@@ -288,6 +289,7 @@ dependencies = [
 "aptos-types",
 "aptos-vm",
 "aptos-vm-validator",
+ "bcs 0.1.4 (git+https://github.com/aptos-labs/bcs.git?rev=d31fab9d81748e2594be5cd5cdf845786a30562d)",


Where did this come from?

I'm not sure... I don't think I need this? how should I remove?

angieyth · 2023-02-09T21:36:33Z

In the last commit

address comments
rebased and made changes on the top of [Quorum Store] Implementation of quorum store components #6055

movekevin · 2023-02-14T16:38:38Z

api/test-context/src/test_context.rs

@@ -296,8 +295,15 @@ impl TestContext {
        TransactionFactory::new(self.context.chain_id())
    }

-    pub fn root_account(&self) -> LocalAccount {
-        LocalAccount::new(aptos_test_root_address(), self.root_key.private_key(), 0)
+    pub async fn root_account(&self) -> LocalAccount {


Why not just avoid fetching the sequence number altogether by storing it in the test_context? This would avoid having to make this root_account function async as well.

but in this way we have to make new_text_context() an async func. I think it's fine either way.

api/test-context/src/test_context.rs

movekevin · 2023-02-14T16:39:59Z

api/test-context/src/test_context.rs

+        self.commit_mempool_txns(1).await;
+        account
+    }
+    pub async fn create_user_account(&self, account: &LocalAccount) -> SignedTransaction {


Is this still useful if create_account already takes care of submitting the tx? Can we remove this somehow?

I think it's still useful because we test different things during submitting the txn. that being said, I think we can replace some with the new create_account(). I don't want to keep growing this PR. I can do it in the following ones.

movekevin · 2023-02-14T16:40:14Z

api/test-context/src/test_context.rs

        self.create_user_account_by(&mut tc, account)
    }

-    pub fn mint_user_account(&self, account: &LocalAccount) -> SignedTransaction {
-        let mut tc = self.root_account();
+    pub async fn mint_user_account(&self, account: &LocalAccount) -> SignedTransaction {


Same here. This is already part of create_account now

api/test-context/src/test_context.rs

movekevin · 2023-02-14T16:41:40Z

api/test-context/src/test_context.rs

+        account_address: &AccountAddress,
+        resource: &str,
+    ) -> Option<Value> {
+        let request = format!("/accounts/{}/resources", account_address);


Why not use the API for fetching a specific resource instead of fetching them all then filtering for the right one?

If we use context.get() it throws an error when the resource doesn't exist.

movekevin · 2023-02-14T16:42:12Z

api/test-context/src/test_context.rs

+        self.get(&request).await
+    }
+
+    pub fn build_package(


Do we need separate build_package and publish_package?

it comes from #6055. In that PR the author split the function into 2, I moved those functions here.

movekevin · 2023-02-14T16:42:47Z

api/test-context/src/test_context.rs

@@ -438,18 +512,29 @@ impl TestContext {
            .unwrap();
    }

+    pub async fn get_sequence_number(&self, account: AccountAddress) -> u64 {
+        let account_resource = self
+            .api_get_account_resource(account, "0x1", "account", "Account")


Do you know why we need to pass "0x1", "account", "Account" to api_get_account_resource? The account resource is always at the same path

movekevin · 2023-02-15T05:02:06Z

api/test-context/src/test_context.rs

+            .await;
+        self.commit_mempool_txns(1).await;
+        account
+    }


nit: add an empty line in between functions

gregnazario · 2023-02-22T18:34:32Z

api/test-context/move/Move.toml

@@ -0,0 +1,9 @@
+[package]
+name = "TransactationTests"


TransactionTests

gregnazario · 2023-02-22T18:37:39Z

Please run scripts/rust_lint.sh for formatting

…ewrite the test test_get_txn_execute_failed_by_entry_function_execution_failure

…h other tests

github-actions · 2023-02-23T23:45:04Z

✅ Forge suite `land_blocking` success on `cd9744b371a93137c3bc44f09f69d6897b823af2`

performance benchmark with full nodes : 5823 TPS, 6811 ms latency, 8700 ms p99 latency,no expired txns
Test Ok

github-actions · 2023-02-23T23:47:30Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `cd9744b371a93137c3bc44f09f69d6897b823af2`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> cd9744b371a93137c3bc44f09f69d6897b823af2 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7728 TPS, 4974 ms latency, 7000 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: cd9744b371a93137c3bc44f09f69d6897b823af2
compatibility::simple-validator-upgrade::single-validator-upgrade : 5058 TPS, 7862 ms latency, 10800 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: cd9744b371a93137c3bc44f09f69d6897b823af2
compatibility::simple-validator-upgrade::half-validator-upgrade : 4701 TPS, 8413 ms latency, 10900 ms p99 latency,no expired txns
4. upgrading second batch to new version: cd9744b371a93137c3bc44f09f69d6897b823af2
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6728 TPS, 5657 ms latency, 10300 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> cd9744b371a93137c3bc44f09f69d6897b823af2 passed
Test Ok

github-actions · 2023-02-23T23:48:49Z

✅ Forge suite `framework_upgrade` success on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `cd9744b371a93137c3bc44f09f69d6897b823af2`

Compatibility test results for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> cd9744b371a93137c3bc44f09f69d6897b823af2 (PR)
Upgrade the nodes to version: cd9744b371a93137c3bc44f09f69d6897b823af2
framework_upgrade::framework-upgrade::full-framework-upgrade : 6600 TPS, 5730 ms latency, 9600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> cd9744b371a93137c3bc44f09f69d6897b823af2 passed
Test Ok

angieyth requested review from banool, gregnazario and 0xmaayan as code owners February 7, 2023 19:47

angieyth marked this pull request as draft February 7, 2023 19:54

angieyth marked this pull request as ready for review February 7, 2023 20:07

angieyth requested a review from areshand as a code owner February 8, 2023 23:31

angieyth force-pushed the angie/transaction_tests branch from a45ba8e to 4591e5c Compare February 8, 2023 23:53

angieyth requested a review from movekevin February 8, 2023 23:53

movekevin reviewed Feb 9, 2023

View reviewed changes

angieyth force-pushed the angie/transaction_tests branch from c2130ee to 62ea52e Compare February 9, 2023 20:22

angieyth force-pushed the angie/transaction_tests branch from ff280da to 35d4056 Compare February 10, 2023 18:39

movekevin reviewed Feb 14, 2023

View reviewed changes

angieyth force-pushed the angie/transaction_tests branch from 35d4056 to 57b3c2e Compare February 15, 2023 01:08

movekevin reviewed Feb 15, 2023

View reviewed changes

movekevin approved these changes Feb 15, 2023

View reviewed changes

angieyth requested a review from davidiw February 17, 2023 18:55

gregnazario approved these changes Feb 22, 2023

View reviewed changes

api/test-context/move/Move.toml

@@ -0,0 +1,9 @@

[package]

name = "TransactationTests"

Copy link

Contributor

gregnazario Feb 22, 2023

Choose a reason for hiding this comment

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

TransactionTests

angieyth force-pushed the angie/transaction_tests branch 2 times, most recently from c4a88a7 to a1887a6 Compare February 23, 2023 17:45

angieyth added 8 commits February 23, 2023 13:57

move helper functions to text_context.rs and fix some typo

4bd9830

remove a line

1098007

[transaction test] adding new move module for transaction tests and r…

0553129

…ewrite the test test_get_txn_execute_failed_by_entry_function_execution_failure

[transaction test] move create_account() to test_context to share wit…

94464b6

…h other tests

[transaction tests] fix lint

a2894f9

address comments and further refactoring

ce7053c

rebase and make changes to publish_packages

35580cb

refactor root_account and make it async base on #5894

7f3721f

angieyth added 2 commits February 23, 2023 13:57

fix lint errors

4b48fa5

fix merge conflicts

cd9744b

angieyth force-pushed the angie/transaction_tests branch from a1887a6 to cd9744b Compare February 23, 2023 21:57

angieyth enabled auto-merge (squash) February 23, 2023 23:01

This comment has been minimized.

Sign in to view

angieyth merged commit 1755e84 into main Feb 23, 2023

angieyth deleted the angie/transaction_tests branch February 23, 2023 23:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[transaction test] refactoring publishing flow to test_context #6529

[transaction test] refactoring publishing flow to test_context #6529

angieyth commented Feb 7, 2023 •

edited

Loading

movekevin Feb 9, 2023

angieyth Feb 9, 2023 •

edited

Loading

movekevin Feb 9, 2023

angieyth Feb 9, 2023

angieyth commented Feb 9, 2023

movekevin Feb 14, 2023

angieyth Feb 14, 2023 •

edited

Loading

movekevin Feb 14, 2023

angieyth Feb 15, 2023

movekevin Feb 14, 2023

movekevin Feb 14, 2023

angieyth Feb 14, 2023 •

edited

Loading

movekevin Feb 14, 2023

angieyth Feb 14, 2023

movekevin Feb 14, 2023

movekevin Feb 15, 2023 •

edited

Loading

gregnazario Feb 22, 2023

gregnazario commented Feb 22, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 23, 2023

github-actions bot commented Feb 23, 2023

github-actions bot commented Feb 23, 2023

[transaction test] refactoring publishing flow to test_context #6529

[transaction test] refactoring publishing flow to test_context #6529

Conversation

angieyth commented Feb 7, 2023 • edited Loading

Description

Test Plan

Choose a reason for hiding this comment

angieyth Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angieyth commented Feb 9, 2023

Choose a reason for hiding this comment

angieyth Feb 14, 2023 • 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

angieyth Feb 14, 2023 • 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

movekevin Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregnazario commented Feb 22, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Feb 23, 2023

✅ Forge suite land_blocking success on cd9744b371a93137c3bc44f09f69d6897b823af2

github-actions bot commented Feb 23, 2023

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> cd9744b371a93137c3bc44f09f69d6897b823af2

github-actions bot commented Feb 23, 2023

✅ Forge suite framework_upgrade success on cb4ba0a57c998c60cbab65af31a64875d2588ca5 ==> cd9744b371a93137c3bc44f09f69d6897b823af2

angieyth commented Feb 7, 2023 •

edited

Loading

angieyth Feb 9, 2023 •

edited

Loading

angieyth Feb 14, 2023 •

edited

Loading

angieyth Feb 14, 2023 •

edited

Loading

movekevin Feb 15, 2023 •

edited

Loading

✅ Forge suite `land_blocking` success on `cd9744b371a93137c3bc44f09f69d6897b823af2`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `cd9744b371a93137c3bc44f09f69d6897b823af2`

✅ Forge suite `framework_upgrade` success on `cb4ba0a57c998c60cbab65af31a64875d2588ca5` ==> `cd9744b371a93137c3bc44f09f69d6897b823af2`