Run MOVE_COMPILER_V2 with more tests and record outputs in .v2_exp files #13254
Conversation
⏱️ 2h 45m total CI duration on this PR
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13254 +/- ##
=========================================
- Coverage 33.0% 33.0% -0.1%
=========================================
Files 1760 1764 +4
Lines 338939 339033 +94
=========================================
+ Hits 111976 111996 +20
- Misses 226963 227037 +74 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
- I'm not seeing what this PR brings us in new quality of testing since most features we have v2 tests for now are rather generic Move code which very likely are already covered by our regular tests.
- Instead we are adding new technical debt by having the v1/v2 living side by side. When we remove v1, we have to pay for this.
- Some of the v2 tests (e.g. bytecode verifier) should have the same .exp file and not a new one. If the difference is in things like definition index we should better redact this from the baseline instead of creating new v2_exp.
| @@ -0,0 +1,1138 @@ | |||
| /* ======================================= | |||
There was a problem hiding this comment.
I think I said before we should keep the evm stuff alone. Why are we adding this additional complexity?
There was a problem hiding this comment.
It was easy enough to add it. Just a line in a script.
| @@ -0,0 +1,55 @@ | |||
| publishing Actor | |||
There was a problem hiding this comment.
Same here. Once we switch to v2 compiler as the default, those things can be updated. This does not give us relevant new test coverage, most test cases here and in the EVM code are not specific to Move features, but to the tool.
| @@ -0,0 +1,37 @@ | |||
| processed 8 tasks | |||
There was a problem hiding this comment.
How does that differ from the simple .exp? There shouldn't be any difference. Is it the definition indices?
| @@ -11,6 +11,14 @@ pub const EXP_EXT: &str = "exp"; | |||
| /// Extension for expected output files compiled by v2 | |||
| pub const EXP_EXT_V2: &str = "v2_exp"; | |||
|
|
|||
| pub fn get_compiler_exp_extension() -> &'static str { | |||
| if get_move_compiler_v2_from_env() { | |||
There was a problem hiding this comment.
Any tests which are dependent from env variables must be run in their own CI action and cannot run in parallel with other tests and different env vars. Its perhaps OK we do this for e2e tests (like we do already) but for unit tests it is not a good design. Where we have already different exp files (prover, compiler unit tests and transactional tests) we do not do this via env vars but via test configs.
I've already been able to somewhat reproduce Kevin's bug using move-docgen tests, because of the different build environment provided. In other cases, the added coverage is small, but it is non-zero. We can also test which tools/paths accidentally still use V1 under the covers by using the MOVE_COMPILER_DISABLE_V1 env var.
This gives us an easy way to get a bit more coverage to track compiler V2 correctness and avoiding regressions:
A few parallel output files which we can ignore (after merging this PR) unless they change is an insignificant debt to take on for the benefit, even if it is small. Trying more complex approaches to leverage the testing value of these other packages would involve a significant amount of wasted time or technical debt: Jumping through hoops to make outputs identical, or building complex redaction/comparison frameworks to try to enable V2 runs to use exactly the same .exp files as V1 is technical debt. In retrospect, the move-compiler-v2-transactional-tests comparison mechanism might have been better done this way: we would avoided wasteful duplicate runs of V1 on every test, and also simplified the testing code significantly. The cost is a one-time comparison of each .v2_exp file with each .exp file, and then we can forget about v1. We can rename .v2_exp to .exp when we get rid of V1 if it bothers you. |
|
Hi @brmataptos, sorry I don't look all code and new files in detail because there are so many. I am wondering how these tests are run against the compiler V2 on CI? Do we automatically run V1 and then V2 for all of them (like the prover test)? |
I think these will have to be run as a separate CI job with The problem is that these tests represent a number of different test frameworks, so doing something like what was done for the prover tests would require a lot of work of questionable long-term value. This is a quick hack that allows us to run tests in the following directories under V2 with minimal effort, gaining perhaps 637 files with very few changes to .rs files. It requires some scripting to manage this collection of tests, but it will be easy to rename and/or change the defaults once V2 becomes mainstream. Note that this PR introduces test outputs for the following directories: The two directories marked with Of the resulting 835 Of these files:
|
brmataptos
force-pushed
the
brm-issue-13216
branch
from
3ca51d2
to
9bd22c2
Compare
brmataptos
force-pushed
the
brm-issue-13216
branch
2 times, most recently
from
f112943
to
9e96207
Compare
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.
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.
brmataptos
force-pushed
the
brm-issue-13216
branch
from
9e96207
to
a911281
Compare
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.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Description
Updated various test harnesses and re-ran pretty much all .move tests with
MOVE_COMPILER_V2=trueto generate .v2_exp files to see what V2 does today. Diffed outputs with V1 outputs and filed bugs for differences (fortunately few).Tests were run manually. Perhaps there should be a script. Something like the following was used:
Note, did not yet set
MOVE_COMPILER_BLOCK_V1so some of these may be using V1 under the hood, but with the baseline outputs checked in we can see if things degrade when we force use of V2.Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
See above.
Key Areas to Review