-
Notifications
You must be signed in to change notification settings - Fork 2.2k
test: refactor testdata/ tests to be run in forge test
#12049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
this will be a game changer for CI! I'd suggest getting rid of default / Paris/ shanghai testdata / test runners stuff and using the newly added vm.setEvmVersion cheatcode to assert different outcomes in same tests instead
right, we can do that as a follow-up since the diff is going to be big already from just fixing tests and making it work in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should re-enable this one with AST seeded fuzz dictionary PR, should be much faster cc @0xrusowsky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as for #12049 (comment) with AST seeded fuzz dictionary we will collect values of 0x4446 and 0x5556 and use to fuzz selectors.
For the
counter += numToIncrement;
counterX2 += numToIncrement * 2;
case wonder if folding could help #12044 cc @0xrusowsky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in our current plans we only had constant folding. In this case how would we derive a value for numToIncrement?
at most we could use the += and the * 2 to create smarter mutators for dict values, but not totally sure how to leverage that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a note, pls lmk if I am wrong: we should still add sol files repros as before in testdata/default/repros but if there are repros that require check of output (fails or we want to check traces) we should add them here and assert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's also what i understood + it aligns with the PR desc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that's right, if the test can be implemented using foundry directly then it should go in testdata
only if it needs higher level assertions like console output should it go to forgetest!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we close #10577 with this PR? cc @zerosnacks
And probably #10744 not needed anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be cleaner to resolve them instead of ignoring them but I can quite easily create a follow-up PR to reduce the number of ignored error codes, especially 2319 and 2519 need to be addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! since a big diff waiting for other reviews before merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big diff, didn't spot anything that seemed wrong. lgtm
Uh oh!
There was an error while loading. Please reload this page.
Refactor manual integration tests to just run
forge teston thetestdatafolder project.We set up testdata/ to be a Foundry project, and a single test that runs
forge teston it.Not all current tests pass due to inability of asserting higher level things like console output, general test failure, etc. These tests are migrated to inline
forgetest!Rust snapshot tests.The bulk of the manual work was migrating fuzz and invariant tests that assert data structures like counterexample list into
str![[]]tests, making sure the config is also migrated from the relevant Rust test.The rest was mostly automated / claude-coded.
Followups:
Config::load_with_rootinupdate_config. We ignore remappings so we shouldnt try to resolve them at all