[Compiler-v2] Fix test plan in move-unit-test to use compiler v2 #15388

rahxephon89 · 2024-11-25T07:15:52Z

Description

This PR closes #15385 by explicitly using test plan builder v2 in move-unit-test when MOVE_COMPILER_V2 is set to true.

How Has This Been Tested?

Existing test cases in move-unit-test, see updated baseline files.

Key Areas to Review

Whether flags are correctly set when running tests using Compiler v2.

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-11-25T07:15:56Z

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

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	27m	🟩
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	12m	🟩
rust-cargo-deny	7m	🟩 🟩 🟩 🟩
rust-move-tests	7m	⬜
check-dynamic-deps	6m	🟩 🟩 🟩 🟩 🟩
rust-doc-tests	6m	🟩
test-target-determinator	5m	🟩
execution-performance / test-target-determinator	5m	🟩
check	4m	🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
general-lints	2m	🟩 🟩 🟩 🟩
fetch-last-released-docker-image-tag	2m	🟩
file_change_determinator	44s	🟩 🟩 🟩 🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
execution-performance / single-node-performance	27m	17m

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

vineethk

Thanks for the fix, let's get this in soon please :)

vineethk · 2024-11-25T13:44:56Z

third_party/move/tools/move-unit-test/tests/test_sources/native_signer_creation.v2_exp

 │    │
 │ 36 │     fun test_doesnt_exist() {
 │    │         ----------------- In this function in 0x1::M
 │    ·
-│ 47 │         abort 0
-│    │         ^^^^^^^ Test was not expected to error, but it aborted with code 0 originating in the module 0000000000000000000000000000000000000000000000000000000000000001::M rooted here
+│ 43 │             i = i + 1;


I am guessing this is because of peephole as well, if we can get this PR merged in, I can rebase and further validate the peephole locations fix.

brmataptos

Looks ok, but please check what happens if there is a compile error for the tests.

brmataptos · 2024-11-25T20:30:56Z

aptos-move/framework/aptos-framework/tests/compiler-v2-doc/function_info.md

@@ -147,7 +147,7 @@ Creates a new function info from names.



-<pre><code><b>public</b>(<b>friend</b>) <b>fun</b> <a href="function_info.md#0x1_function_info_new_function_info_from_address">new_function_info_from_address</a>(module_address: <b>address</b>, module_name: <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_String">string::String</a>, function_name: <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_String">string::String</a>): <a href="function_info.md#0x1_function_info_FunctionInfo">function_info::FunctionInfo</a>
+<pre><code><b>public</b> <b>fun</b> <a href="function_info.md#0x1_function_info_new_function_info_from_address">new_function_info_from_address</a>(module_address: <b>address</b>, module_name: <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_String">string::String</a>, function_name: <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_String">string::String</a>): <a href="function_info.md#0x1_function_info_FunctionInfo">function_info::FunctionInfo</a>


I don't like this backtracking, though it's also generated by V1. Filed an issue for it. #15392

brmataptos · 2024-11-25T20:45:35Z

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

+            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))


Last line is common for the 2 branches. It might be better to wrap the whole if in a let (test_plan, files, units) = if ... and have a single such line. I'm actually a little surprised we don't have more in common.

Maybe we didn't before, but we also should bail out if there are errors.

github-actions · 2024-11-25T22:00:01Z

✅ Forge suite `realistic_env_max_load` success on `51019e837072e6027d8b1f0308cc352cc0e030b3`

two traffics test: inner traffic : committed: 14111.37 txn/s, latency: 2818.52 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5365460
two traffics test : committed: 99.98 txn/s, latency: 2022.53 ms, (p50: 2000 ms, p70: 2100, p90: 2200 ms, p99: 17400 ms), latency samples: 1720
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.094, avg: 1.202", "ConsensusProposalToOrdered: max: 0.322, avg: 0.293", "ConsensusOrderedToCommit: max: 0.372, avg: 0.363", "ConsensusProposalToCommit: max: 0.662, avg: 0.656"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.82s no progress at version 34225 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 15.46s no progress at version 2275742 (avg 15.46s) [limit 16].
Test Ok

github-actions · 2024-11-25T22:02:24Z

✅ Forge suite `framework_upgrade` success on `94976266c0e4edd1a31a3783be4c4184f8892ff5` ==> `51019e837072e6027d8b1f0308cc352cc0e030b3`

Compatibility test results for 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 51019e837072e6027d8b1f0308cc352cc0e030b3 (PR)
Upgrade the nodes to version: 51019e837072e6027d8b1f0308cc352cc0e030b3
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1278.61 txn/s, submitted: 1282.59 txn/s, failed submission: 3.98 txn/s, expired: 3.98 txn/s, latency: 2349.19 ms, (p50: 2100 ms, p70: 2400, p90: 3300 ms, p99: 4500 ms), latency samples: 115540
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1547.18 txn/s, submitted: 1548.43 txn/s, failed submission: 1.25 txn/s, expired: 1.25 txn/s, latency: 2195.38 ms, (p50: 2100 ms, p70: 2100, p90: 3000 ms, p99: 4300 ms), latency samples: 123340
5. check swarm health
Compatibility test for 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 51019e837072e6027d8b1f0308cc352cc0e030b3 passed
Upgrade the remaining nodes to version: 51019e837072e6027d8b1f0308cc352cc0e030b3
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1353.00 txn/s, submitted: 1355.95 txn/s, failed submission: 2.95 txn/s, expired: 2.95 txn/s, latency: 2325.84 ms, (p50: 2100 ms, p70: 2400, p90: 3600 ms, p99: 4600 ms), latency samples: 119160
Test Ok

github-actions · 2024-11-25T22:02:33Z

✅ Forge suite `compat` success on `94976266c0e4edd1a31a3783be4c4184f8892ff5` ==> `51019e837072e6027d8b1f0308cc352cc0e030b3`

Compatibility test results for 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 51019e837072e6027d8b1f0308cc352cc0e030b3 (PR)
1. Check liveness of validators at old version: 94976266c0e4edd1a31a3783be4c4184f8892ff5
compatibility::simple-validator-upgrade::liveness-check : committed: 16704.09 txn/s, latency: 2021.88 ms, (p50: 2100 ms, p70: 2100, p90: 2400 ms, p99: 2800 ms), latency samples: 542380
2. Upgrading first Validator to new version: 51019e837072e6027d8b1f0308cc352cc0e030b3
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7948.86 txn/s, latency: 3615.71 ms, (p50: 4100 ms, p70: 4200, p90: 4300 ms, p99: 4400 ms), latency samples: 145520
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7344.66 txn/s, latency: 4320.28 ms, (p50: 4300 ms, p70: 4300, p90: 6700 ms, p99: 7000 ms), latency samples: 242480
3. Upgrading rest of first batch to new version: 51019e837072e6027d8b1f0308cc352cc0e030b3
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7214.18 txn/s, latency: 3647.05 ms, (p50: 4100 ms, p70: 4200, p90: 4300 ms, p99: 4700 ms), latency samples: 147420
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7786.65 txn/s, latency: 4105.87 ms, (p50: 4200 ms, p70: 4300, p90: 6400 ms, p99: 6800 ms), latency samples: 258360
4. upgrading second batch to new version: 51019e837072e6027d8b1f0308cc352cc0e030b3
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11511.04 txn/s, latency: 2409.40 ms, (p50: 2700 ms, p70: 2800, p90: 2900 ms, p99: 3100 ms), latency samples: 197980
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11349.60 txn/s, latency: 2798.22 ms, (p50: 2800 ms, p70: 2900, p90: 3400 ms, p99: 4500 ms), latency samples: 368720
5. check swarm health
Compatibility test for 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 51019e837072e6027d8b1f0308cc352cc0e030b3 passed
Test Ok

rahxephon89 force-pushed the teng/fix-15385 branch from 80986e1 to 4f904cb Compare November 25, 2024 07:39

rahxephon89 changed the title ~~[WIP][Compiler-v2] Fix test plan builder in move-unit-test to use compiler v2~~ [WIP][Compiler-v2] Fix test plan in move-unit-test to use compiler v2 Nov 25, 2024

rahxephon89 changed the title ~~[WIP][Compiler-v2] Fix test plan in move-unit-test to use compiler v2~~ [Compiler-v2] Fix test plan in move-unit-test to use compiler v2 Nov 25, 2024

rahxephon89 marked this pull request as ready for review November 25, 2024 08:03

rahxephon89 requested review from davidiw, movekevin and wrwg as code owners November 25, 2024 08:03

rahxephon89 requested review from brmataptos and vineethk November 25, 2024 08:03

vineethk approved these changes Nov 25, 2024

View reviewed changes

rahxephon89 requested a review from fEst1ck November 25, 2024 17:47

brmataptos approved these changes Nov 25, 2024

View reviewed changes

fix move-unit-test

26b261a

rahxephon89 force-pushed the teng/fix-15385 branch from 4f904cb to 26b261a Compare November 25, 2024 21:21

Merge branch 'main' into teng/fix-15385

51019e8

rahxephon89 enabled auto-merge (squash) November 25, 2024 21:28

This comment has been minimized.

Sign in to view

rahxephon89 merged commit e2a9041 into main Nov 25, 2024
75 checks passed

rahxephon89 deleted the teng/fix-15385 branch November 25, 2024 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Compiler-v2] Fix test plan in move-unit-test to use compiler v2 #15388

[Compiler-v2] Fix test plan in move-unit-test to use compiler v2 #15388

rahxephon89 commented Nov 25, 2024 •

edited

Loading

trunk-io bot commented Nov 25, 2024 •

edited

Loading

vineethk left a comment

vineethk Nov 25, 2024

brmataptos left a comment

brmataptos Nov 25, 2024

brmataptos Nov 25, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024

		@@ -147,7 +147,7 @@ Creates a new function info from names.



		<pre><code><b>public</b>(<b>friend</b>) <b>fun</b> <a href="function_info.md#0x1_function_info_new_function_info_from_address">new_function_info_from_address</a>(module_address: <b>address</b>, module_name: <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_String">string::String</a>, function_name: <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_String">string::String</a>): <a href="function_info.md#0x1_function_info_FunctionInfo">function_info::FunctionInfo</a>
		<pre><code><b>public</b> <b>fun</b> <a href="function_info.md#0x1_function_info_new_function_info_from_address">new_function_info_from_address</a>(module_address: <b>address</b>, module_name: <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_String">string::String</a>, function_name: <a href="../../../aptos-stdlib/../move-stdlib/tests/compiler-v2-doc/string.md#0x1_string_String">string::String</a>): <a href="function_info.md#0x1_function_info_FunctionInfo">function_info::FunctionInfo</a>

[Compiler-v2] Fix test plan in move-unit-test to use compiler v2 #15388

[Compiler-v2] Fix test plan in move-unit-test to use compiler v2 #15388

Conversation

rahxephon89 commented Nov 25, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

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

vineethk left a comment

Choose a reason for hiding this comment

vineethk Nov 25, 2024

Choose a reason for hiding this comment

brmataptos left a comment

Choose a reason for hiding this comment

brmataptos Nov 25, 2024

Choose a reason for hiding this comment

brmataptos Nov 25, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 25, 2024

✅ Forge suite realistic_env_max_load success on 51019e837072e6027d8b1f0308cc352cc0e030b3

github-actions bot commented Nov 25, 2024

✅ Forge suite framework_upgrade success on 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 51019e837072e6027d8b1f0308cc352cc0e030b3

github-actions bot commented Nov 25, 2024

✅ Forge suite compat success on 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 51019e837072e6027d8b1f0308cc352cc0e030b3

rahxephon89 commented Nov 25, 2024 •

edited

Loading

trunk-io bot commented Nov 25, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `51019e837072e6027d8b1f0308cc352cc0e030b3`

✅ Forge suite `framework_upgrade` success on `94976266c0e4edd1a31a3783be4c4184f8892ff5` ==> `51019e837072e6027d8b1f0308cc352cc0e030b3`

✅ Forge suite `compat` success on `94976266c0e4edd1a31a3783be4c4184f8892ff5` ==> `51019e837072e6027d8b1f0308cc352cc0e030b3`