feat(test): Check that nargo::ops::transform_program is idempotent#6694
Merged
feat(test): Check that nargo::ops::transform_program is idempotent#6694
nargo::ops::transform_program is idempotent#6694Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem*
Resolves #6670
Summary*
Added a test in the
compile_cmdmodule to run through all tests intest_programs/execution_success, compile them in-memory, then run them through the optimisation transformation twice, comparing the hashes after the first and the second pass, which should be equal unless the optimiser is not idempotent for one of these examples.The idempotency test is currently failing for most test programs.
Testing
An example of a failing run:
To see why a test fails, we can pass its name as an extra argument:
cargo test -q -p nargo_cli --bins -- test_transform_program_is_idempotent slice_loopWhen an argument is given, the test compares the actual
Programs so we can see the difference in opcodes, otherwise it just compares hashes to have a list of failing tests that we can potentially look at. Some have really long bytecode which would produce walls of text if the detailed output was always shown.Applied various fixes in #6695
Additional Context
I added the test in
compile_cmdbecause this is where the transformation is applied, where we are defending against idempotency problems; it is a defensive test to verify that we're right to assume this property, even though we're cautious and avoid running it on cached programs twice. The module also contained code to read manifests and parse files into a workspace, which could be conveniently reused in the test.I thought about trying to generate a separate
#[test]per directory inbuild.rs, but instead of running them as an integration test, splice them into a unit test module to get access to the utility functions for parsing the compiled artefacts. However, in the end I thought this test is less about the test programs themselves as just a sanity check that the optimiser produces stable results, ie. we expect it to be true for all programs, therefore having dozens of tests doing nothing that is really about the test program itself would be just noisy. One benefit of separate tests would have been that cargo would run them in parallel; I hope that using rayon reclaimed enough of this efficiency to make this a non-issue.I am only testing on
execution_success, but we could add the other test directories which are supposed to compile, and potentially even the stdlib tests I suppose. Let me know if you think more coverage is needed.Documentation*
Check one:
PR Checklist*
cargo fmton default settings.