[Move] Allow Bytecode Dependencies for Unit Testing #15252

fEst1ck · 2024-11-12T08:51:46Z

Description

This PR allows Move unit tests to have bytecode dependencies by including bytecode modules in the module_info field of TestPlan.

How Has This Been Tested?

third_party/move/tools/move-unit-test/tests/pkg_tests.rs and manually tested with aptos move test.

Type of Change

New feature
Bug fix

Which Components or Systems Does This Change Impact?

Developer Infrastructure
Move Compiler

trunk-io · 2024-11-12T08:51:49Z

⏱️ 1h 12m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-cargo-deny	12m	🟩 ⬜ 🟩 🟩 🟩 (+2 more)
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
check-dynamic-deps	9m	🟩 🟩 🟩 🟩 🟩 (+2 more)
rust-move-tests	5m	⬜
general-lints	3m	🟩 🟩 🟩 🟩 🟩 (+2 more)
semgrep/ci	3m	🟩 🟩 🟩 🟩 🟩 (+2 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+2 more)
rust-move-tests	1m	⬜
rust-move-tests	1m	⬜
permission-check	22s	🟩 🟩 🟩 🟩 🟩 (+2 more)
permission-check	19s	🟩 🟩 🟩 🟩 🟩 (+2 more)

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

brmataptos · 2024-11-30T21:22:52Z

third_party/move/tools/move-unit-test/tests/packages/one-bytecode-dep/Move.toml

+
+[dependencies]
+Dep = { local = "../dep" }
+MoveStdlib = { local = "../../../../../../../aptos-move/framework/move-stdlib" }


Better not to make third_party to depend on aptos-move. Note that you don't know whether framework/move-stdlib has been compiled at this point or not because there may or may not be an earlier build of framework/move_stdlib or not. Can you create a smaller local test library to third_party to use here?

For a simple test like this that doesn't depend on any real Move logic, I think we can manage to not use the stdlib at all.

vgao1996

LGTM except for some minor things

On a side note: presumably the reason why you added a .gitkeep file is that the package/unit-test system would have trouble handling the package if the sources directory does not exist. To me that's another thing we should fix, but let's do it in a separate PR.

vgao1996 · 2024-12-03T17:57:20Z

third_party/move/tools/move-unit-test/src/lib.rs

+        println!("source_files: {:?}", source_files);
+        println!("deps: {:?}", deps);


Do we want to display these to the users?

vgao1996 · 2024-12-03T18:09:00Z

third_party/move/tools/move-unit-test/tests/pkg_tests.rs

+#[test]
+fn one_bytecode_dep() {
+    run_tests_for_pkg("tests/packages/one-bytecode-dep", true);
+    run_tests_for_pkg("tests/packages/one-bytecode-dep", false);
+}


For now this is fine, but in the future we'll probably want to replace this with a test harness that automatically discovers all Move packages under a package directory and runs unit tests for them.

Left a todo

vgao1996 · 2024-12-03T18:14:40Z

third_party/move/tools/move-unit-test/tests/packages/one-bytecode-dep/Move.toml

+
+[dependencies]
+Dep = { local = "../dep" }
+MoveStdlib = { local = "../../../../../../../aptos-move/framework/move-stdlib" }


For a simple test like this that doesn't depend on any real Move logic, I think we can manage to not use the stdlib at all.

vgao1996 · 2024-12-03T18:16:18Z

third_party/move/tools/move-unit-test/src/lib.rs

@@ -187,7 +189,7 @@ impl UnitTestingConfig {
        let (units, warnings) =
            diagnostics::unwrap_or_report_diagnostics(&files, compilation_result);
        diagnostics::report_warnings(&files, warnings);
-        test_plan.map(|tests| TestPlan::new(tests, files, units))
+        test_plan.map(|tests| TestPlan::new(tests, files, units, Default::default()))


Nit: might be clearer to explicitly use empty vector instead of Default::default() here.

rahxephon89

Assuming all previous comments are handled.

github-actions · 2025-01-07T08:03:08Z

✅ Forge suite `realistic_env_max_load` success on `811033cf1eaa0b07282512366b75809994809c55`

two traffics test: inner traffic : committed: 14960.68 txn/s, latency: 2657.68 ms, (p50: 2700 ms, p70: 2700, p90: 2900 ms, p99: 3200 ms), latency samples: 5688440
two traffics test : committed: 99.99 txn/s, latency: 1335.09 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 1800 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.553, avg: 1.522", "ConsensusProposalToOrdered: max: 0.286, avg: 0.285", "ConsensusOrderedToCommit: max: 0.310, avg: 0.295", "ConsensusProposalToCommit: max: 0.595, avg: 0.580"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.52s no progress at version 4744859 (avg 0.19s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.64s no progress at version 2774688 (avg 0.64s) [limit 16].
Test Ok

github-actions · 2025-01-07T08:05:11Z

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `811033cf1eaa0b07282512366b75809994809c55`

Compatibility test results for 6593fb81261f25490ffddc2252a861c994234c2a ==> 811033cf1eaa0b07282512366b75809994809c55 (PR)
1. Check liveness of validators at old version: 6593fb81261f25490ffddc2252a861c994234c2a
compatibility::simple-validator-upgrade::liveness-check : committed: 17528.61 txn/s, latency: 1937.91 ms, (p50: 1900 ms, p70: 2100, p90: 2400 ms, p99: 3100 ms), latency samples: 583820
2. Upgrading first Validator to new version: 811033cf1eaa0b07282512366b75809994809c55
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7186.37 txn/s, latency: 4174.12 ms, (p50: 4800 ms, p70: 5000, p90: 5200 ms, p99: 5400 ms), latency samples: 133160
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7246.87 txn/s, latency: 4681.28 ms, (p50: 5100 ms, p70: 5200, p90: 5300 ms, p99: 5500 ms), latency samples: 241980
3. Upgrading rest of first batch to new version: 811033cf1eaa0b07282512366b75809994809c55
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6698.21 txn/s, latency: 4144.23 ms, (p50: 4300 ms, p70: 5200, p90: 6800 ms, p99: 6900 ms), latency samples: 124500
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7837.44 txn/s, latency: 4295.24 ms, (p50: 4700 ms, p70: 4800, p90: 5000 ms, p99: 5200 ms), latency samples: 257320
4. upgrading second batch to new version: 811033cf1eaa0b07282512366b75809994809c55
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 12341.37 txn/s, latency: 2361.69 ms, (p50: 2700 ms, p70: 2800, p90: 2900 ms, p99: 2900 ms), latency samples: 215360
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 12359.01 txn/s, latency: 2668.24 ms, (p50: 2800 ms, p70: 2900, p90: 3100 ms, p99: 3300 ms), latency samples: 399400
5. check swarm health
Compatibility test for 6593fb81261f25490ffddc2252a861c994234c2a ==> 811033cf1eaa0b07282512366b75809994809c55 passed
Test Ok

fEst1ck force-pushed the bytecode-dep-unit-test branch from 43cb926 to 62d1fa2 Compare November 13, 2024 07:07

fEst1ck marked this pull request as ready for review November 13, 2024 07:44

fEst1ck requested review from davidiw, movekevin and wrwg as code owners November 13, 2024 07:44

fEst1ck requested a review from vgao1996 November 13, 2024 09:07

fEst1ck added the move-tools label Nov 14, 2024

brmataptos reviewed Nov 30, 2024

View reviewed changes

vgao1996 approved these changes Dec 3, 2024

View reviewed changes

rahxephon89 approved these changes Dec 16, 2024

View reviewed changes

Zekun Wang added 8 commits January 6, 2025 12:11

fix bytecode deps for unit tests

998113c

add tests

3157579

linter & remove debug print

65ded3e

reset Cargo.lock

c9868f8

add bytecode files

b274549

reset Cargo.lock

1a46201

add .gitkeep for empty dir

898ee3a

address comments

1e3e7e8

fEst1ck force-pushed the bytecode-dep-unit-test branch from b07c641 to 1e3e7e8 Compare January 6, 2025 17:13

resolve merge conflicts

0876ea5

fEst1ck enabled auto-merge (squash) January 6, 2025 17:18

This comment has been minimized.

Sign in to view

Merge branch 'main' into bytecode-dep-unit-test

b6a64ec

This comment has been minimized.

Sign in to view

Merge branch 'main' into bytecode-dep-unit-test

811033c

This comment has been minimized.

Sign in to view

fEst1ck merged commit aaa3514 into aptos-labs:main Jan 7, 2025
46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Move] Allow Bytecode Dependencies for Unit Testing #15252

[Move] Allow Bytecode Dependencies for Unit Testing #15252

fEst1ck commented Nov 12, 2024 •

edited

Loading

trunk-io bot commented Nov 12, 2024 •

edited

Loading

brmataptos Nov 30, 2024

vgao1996 Dec 3, 2024

vgao1996 left a comment

vgao1996 Dec 3, 2024

fEst1ck Jan 6, 2025

vgao1996 Dec 3, 2024

fEst1ck Jan 6, 2025

vgao1996 Dec 3, 2024

vgao1996 Dec 3, 2024

fEst1ck Jan 6, 2025

rahxephon89 left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 7, 2025

github-actions bot commented Jan 7, 2025

		println!("source_files: {:?}", source_files);
		println!("deps: {:?}", deps);

[Move] Allow Bytecode Dependencies for Unit Testing #15252

[Move] Allow Bytecode Dependencies for Unit Testing #15252

Conversation

fEst1ck commented Nov 12, 2024 • edited Loading

Description

How Has This Been Tested?

Type of Change

Which Components or Systems Does This Change Impact?

trunk-io bot commented Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgao1996 left a 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

rahxephon89 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 7, 2025

✅ Forge suite realistic_env_max_load success on 811033cf1eaa0b07282512366b75809994809c55

github-actions bot commented Jan 7, 2025

✅ Forge suite compat success on 6593fb81261f25490ffddc2252a861c994234c2a ==> 811033cf1eaa0b07282512366b75809994809c55

fEst1ck commented Nov 12, 2024 •

edited

Loading

trunk-io bot commented Nov 12, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `811033cf1eaa0b07282512366b75809994809c55`

✅ Forge suite `compat` success on `6593fb81261f25490ffddc2252a861c994234c2a` ==> `811033cf1eaa0b07282512366b75809994809c55`