[compiler] More tests for "for" loops #11584

brmataptos · 2024-01-06T00:53:05Z

Description

Add a few more tests for for loops. Copy some tests over to move-compiler-v2 to also test there.
For correct code, use transactional-tests, but for compiler errors, use tests.

Also fix a typo in one test making it not useful.

Addresses Issue #11583.

Test Plan

These are tests.

trunk-io · 2024-01-06T00:53:10Z

⏱️ 8h 18m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-unit-tests	2h 42m	🟩 🟩 ⬜ 🟩 🟩 (+1 more)
windows-build	1h 49m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-unit-coverage	1h 41m	🟩 🟩 ⬜ 🟥 🟩 (+1 more)
rust-lints	44m	🟩 🟩 🟩 🟩 🟩 (+1 more)
run-tests-main-branch	25m	🟩 🟩 🟩 🟩 🟩 (+1 more)
check	24m	🟩 🟩 🟩 🟩 🟩 (+1 more)
general-lints	16m	🟩 🟩 🟩 🟩 🟩 (+1 more)
check-dynamic-deps	13m	🟩 🟩 🟩 🟩 🟩 (+1 more)
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	17s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	16s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	16s	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	15s	🟩 🟩 🟩 🟩 🟩 (+1 more)

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

Job	Duration	vs 7d avg	Delta
windows-build	11m	18m

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

codecov · 2024-01-06T01:13:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (409779a) 68.8% compared to head (a3418de) 68.9%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #11584     +/-   ##
=========================================
+ Coverage    68.8%    68.9%   +0.1%     
=========================================
  Files         775      775             
  Lines      178229   178229             
=========================================
+ Hits       122677   122866    +189     
+ Misses      55552    55363    -189

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vineethk

Have some minor comments, LGTM otherwise.

Thanks for doing this!

vineethk · 2024-01-09T01:09:20Z

...party/move/move-compiler/transactional-tests/tests/control_flow/break_continue_for_loop.move

-            continue
-        }
-        assert!(move x == 5, 42);
+        for (i in 0..10) { // 0,  1,  2,  3,  4, 5, 6, 7, 8, 9, 10


Suggested change

for (i in 0..10) { // 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10

for (i in 0..10) { // 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Upper bound is exclusive.

vineethk · 2024-01-09T01:11:34Z

...party/move/move-compiler/transactional-tests/tests/control_flow/break_continue_for_loop.move

+        for (i in 0..10) { // 0,  1,  2,  3,  4, 5, 6, 7, 8, 9, 10
+            x = x + 1;     // 1,  4, 13, 16,
+            if (x >= 15) break;
+	    x = x + 2;     // 3,  6, 15,  -


Suggested change

x = x + 2; // 3, 6, 15, -

x = x + 2; // 3, 6, 15, -

Indentation here and below (assignments to x) should be fixed for readability.

oops, emacs added tabs. gotta get rid of that.

vineethk · 2024-01-09T01:14:26Z

...ove/move-compiler/transactional-tests/tests/control_flow/break_continue_for_loop_nested.exp1

@@ -0,0 +1,151 @@
+processed 1 task
+
+task 0 'print-bytecode'. lines 1-25:


What is this *.exp1 file?

oops, was a backup file.

vineethk · 2024-01-09T01:15:40Z

...ove/move-compiler/transactional-tests/tests/control_flow/break_continue_for_loop_nested.move

+    fun main() {
+        let k = 0;
+	let y = 0;
+	for (j in 0..10) { //j:  0,  1,  2,  3,  4, 5, 6, 7, 8, 9, 10


The indentation in this test is broken: makes it hard to read.

again, tab issue.

vineethk · 2024-01-09T01:21:43Z

...y/move/move-compiler/transactional-tests/tests/control_flow/function_call_mut_arg_twice.move

@@ -0,0 +1,18 @@
+//# publish


Looks like many non-for-loop related tests (like this one) were also added. I am assuming this is intentional.

I was making /control_flow/ the same for V1 and V2.

But I've also backpedaled and removed a test that doesn't yield expected warnings on -v2 now.

vineethk · 2024-01-09T01:23:18Z

...d_party/move/move-compiler-v2/tests/checking/control_flow/infinite_loop_with_dead_exits.move

+address 0x42 {
+module M {
+
+    // fun t1() {


Weird to have commented out code here. Any particular reason?

vineethk · 2024-01-09T01:25:27Z

...ty/move/move-compiler-v2/transactional-tests/tests/control_flow/break_continue_for_loop.move

+script {
+    fun main() {
+        let x = 0;
+        for (i in 0..10) { // 0,  1,  2,  3,  4, 5, 6, 7, 8, 9, 10


Similar comments as made on the "original" file.

vineethk · 2024-01-09T01:26:10Z

.../move-compiler-v2/transactional-tests/tests/control_flow/break_continue_for_loop_nested.move

+	    y = y + 2;     //y:  3,  6, 15,  -
+	    let x = 0;
+            for (i in 0..10) { //i:  0,  1,  2,  3,  4, 5, 6, 7, 8, 9, 10
+		x = x + 1;     //x:  1,  4, 13, 16,


Please fix indentation if possible.

Sorry, tabs again.

… from -v2/tests since output is incorrect

github-actions · 2024-01-18T07:41:16Z

✅ Forge suite `compat` success on `aptos-node-v1.8.3` ==> `a3418de924c811edd3703d2f8ab8f0e215fdc7c3`

Compatibility test results for aptos-node-v1.8.3 ==> a3418de924c811edd3703d2f8ab8f0e215fdc7c3 (PR)
1. Check liveness of validators at old version: aptos-node-v1.8.3
compatibility::simple-validator-upgrade::liveness-check : committed: 3248 txn/s, latency: 6743 ms, (p50: 6900 ms, p90: 9600 ms, p99: 14500 ms), latency samples: 178640
2. Upgrading first Validator to new version: a3418de924c811edd3703d2f8ab8f0e215fdc7c3
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1775 txn/s, latency: 16326 ms, (p50: 19200 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 92340
3. Upgrading rest of first batch to new version: a3418de924c811edd3703d2f8ab8f0e215fdc7c3
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1788 txn/s, latency: 16292 ms, (p50: 18700 ms, p90: 22200 ms, p99: 22400 ms), latency samples: 93000
4. upgrading second batch to new version: a3418de924c811edd3703d2f8ab8f0e215fdc7c3
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3606 txn/s, latency: 8762 ms, (p50: 9900 ms, p90: 12400 ms, p99: 12700 ms), latency samples: 144260
5. check swarm health
Compatibility test for aptos-node-v1.8.3 ==> a3418de924c811edd3703d2f8ab8f0e215fdc7c3 passed
Test Ok

github-actions · 2024-01-18T07:42:37Z

✅ Forge suite `realistic_env_max_load` success on `a3418de924c811edd3703d2f8ab8f0e215fdc7c3`

two traffics test: inner traffic : committed: 7182 txn/s, latency: 5327 ms, (p50: 4800 ms, p90: 6800 ms, p99: 14400 ms), latency samples: 3110020
two traffics test : committed: 100 txn/s, latency: 2324 ms, (p50: 2200 ms, p90: 2600 ms, p99: 8300 ms), latency samples: 1860
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.258, avg: 0.209", "QsPosToProposal: max: 0.150, avg: 0.142", "ConsensusProposalToOrdered: max: 0.596, avg: 0.546", "ConsensusOrderedToCommit: max: 0.491, avg: 0.465", "ConsensusProposalToCommit: max: 1.058, avg: 1.010"]
Max round gap was 1 [limit 4] at version 1511389. Max no progress secs was 9.330177 [limit 10] at version 1511389.
Test Ok

github-actions · 2024-01-18T08:04:38Z

❌ Forge suite `framework_upgrade` failure on `aptos-node-v1.8.3` ==> `a3418de924c811edd3703d2f8ab8f0e215fdc7c3`

Compatibility test results for aptos-node-v1.8.3 ==> a3418de924c811edd3703d2f8ab8f0e215fdc7c3 (PR)
Upgrade the nodes to version: a3418de924c811edd3703d2f8ab8f0e215fdc7c3
Test Failed: API error: Unknown error error sending request for url (http://aptos-node-3-validator.forge-framework-upgrade-pr-11584.svc:8080/v1/accounts/0000000000000000000000000000000000000000000000000000000000000001/resource/0x1::block::BlockResource): error trying to connect: dns error: failed to lookup address information: Name or service not known

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: __libc_start_main
  15: <unknown>
Trailing Log Lines:
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: __libc_start_main
  15: <unknown>


Swarm logs can be found here: See fgi output for more information.
thread 'main' panicked at testsuite/forge/src/backend/k8s/swarm.rs:676:18:
called `Result::unwrap()` on an `Err` value: ApiError: namespaces "forge-framework-upgrade-pr-11584" not found: NotFound (ErrorResponse { status: "Failure", message: "namespaces \"forge-framework-upgrade-pr-11584\" not found", reason: "NotFound", code: 404 })

Caused by:
    namespaces "forge-framework-upgrade-pr-11584" not found: NotFound

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: <unknown>
  15: __libc_start_main
  16: <unknown>
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Debugging output:

brmataptos requested review from aalok-t and vineethk January 6, 2024 00:53

brmataptos force-pushed the brm-issue-11583 branch from 1b19d7a to da54458 Compare January 7, 2024 04:22

vineethk reviewed Jan 9, 2024

View reviewed changes

brmataptos requested a review from wrwg January 9, 2024 19:27

brmataptos force-pushed the brm-issue-11583 branch from 408bbaf to a167b09 Compare January 9, 2024 19:44

brmataptos mentioned this pull request Jan 10, 2024

[Bug] Unused iterator in for loop #11427

Open

brmataptos removed the request for review from aalok-t January 16, 2024 18:28

brmataptos changed the title ~~More tests for "for" loops~~ [compiler] More tests for "for" loops Jan 16, 2024

vineethk approved these changes Jan 16, 2024

View reviewed changes

brmataptos force-pushed the brm-issue-11583 branch from 1981f03 to bab43d5 Compare January 17, 2024 04:36

wrwg approved these changes Jan 17, 2024

View reviewed changes

brmataptos added 4 commits January 17, 2024 23:17

add some tests for for loops

fdd5678

fix missing output case

a8d15ac

get rid of tabs, commented tests, remove infinite_lop_with_dead_exits…

14e56cb

… from -v2/tests since output is incorrect

add tests illustrating issue #11427

a3418de

brmataptos force-pushed the brm-issue-11583 branch from f75fe69 to a3418de Compare January 18, 2024 07:17

brmataptos enabled auto-merge (squash) January 18, 2024 07:18

This comment has been minimized.

Sign in to view

brmataptos merged commit 8f0c41a into main Jan 18, 2024
81 of 85 checks passed

brmataptos deleted the brm-issue-11583 branch January 18, 2024 07:47

vineethk mentioned this pull request Jan 19, 2024

[Bug][move-compiler] more tests for for loops #11583

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler] More tests for "for" loops #11584

[compiler] More tests for "for" loops #11584

brmataptos commented Jan 6, 2024

trunk-io bot commented Jan 6, 2024 •

edited

Loading

codecov bot commented Jan 6, 2024 •

edited

Loading

vineethk left a comment

vineethk Jan 9, 2024

vineethk Jan 9, 2024

brmataptos Jan 9, 2024

vineethk Jan 9, 2024

vineethk Jan 9, 2024

brmataptos Jan 9, 2024

vineethk Jan 9, 2024

brmataptos Jan 9, 2024

vineethk Jan 9, 2024

brmataptos Jan 9, 2024

vineethk Jan 9, 2024

brmataptos Jan 9, 2024

brmataptos Jan 9, 2024

vineethk Jan 9, 2024

vineethk Jan 9, 2024

vineethk Jan 9, 2024

brmataptos Jan 9, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

	for (i in 0..10) { // 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10
	for (i in 0..10) { // 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

		@@ -0,0 +1,151 @@
		processed 1 task

		task 0 'print-bytecode'. lines 1-25:

[compiler] More tests for "for" loops #11584

[compiler] More tests for "for" loops #11584

Conversation

brmataptos commented Jan 6, 2024

Description

Test Plan

trunk-io bot commented Jan 6, 2024 • edited Loading

codecov bot commented Jan 6, 2024 • edited Loading

Codecov Report

vineethk 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

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

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 Jan 18, 2024

✅ Forge suite compat success on aptos-node-v1.8.3 ==> a3418de924c811edd3703d2f8ab8f0e215fdc7c3

github-actions bot commented Jan 18, 2024

✅ Forge suite realistic_env_max_load success on a3418de924c811edd3703d2f8ab8f0e215fdc7c3

github-actions bot commented Jan 18, 2024

❌ Forge suite framework_upgrade failure on aptos-node-v1.8.3 ==> a3418de924c811edd3703d2f8ab8f0e215fdc7c3

trunk-io bot commented Jan 6, 2024 •

edited

Loading

codecov bot commented Jan 6, 2024 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.8.3` ==> `a3418de924c811edd3703d2f8ab8f0e215fdc7c3`

✅ Forge suite `realistic_env_max_load` success on `a3418de924c811edd3703d2f8ab8f0e215fdc7c3`

❌ Forge suite `framework_upgrade` failure on `aptos-node-v1.8.3` ==> `a3418de924c811edd3703d2f8ab8f0e215fdc7c3`