-
Notifications
You must be signed in to change notification settings - Fork 82
refactor(levm): rewrite of state EF tests runner first iteration #3642
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
base: main
Are you sure you want to change the base?
Conversation
Lines of code reportTotal lines added: Detailed view
|
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.
Really nice work! I like how you distributed the modules, having runner separated from result_check makes things feel less crowded and easier to navigate. I made a couple of comments, one you can consider moving on (it's about writing reports) the rest are rather subjective and totally skippable.
FailedToDeserializeField(String), | ||
FailedToCreateReportFile(String), | ||
FailedToGetIndexValue(String), | ||
} |
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.
Maybe the runner errors can be distributed at lest in two categories, each with its own enum, so you can distinguish between parsing errors and LEVM errors. I'm not sure about this though because I also think that it could escalate so you end up having too many nested enums and to me that also overcomplicates debugging... Maybe pin this though and consider it as you continue with the runner.
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.
I think this might be a good suggestion, for now I think it could be considered as out of scope as the new runner is still in a quite rustic state, but I will keep in mind for later iterations to improve the Errors.
format!( | ||
"Test checks failed for test: {:?}, with fork: {:?}, in path: {:?}.\n", | ||
test.name, test_case.fork, test.path, | ||
) |
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.
Maybe it could be useful to have the chance to write only the failed tests in the report. I think this could be regulated with a flag you set when running the tests. That being said, I like the idea of having both the succesful and failed tests on the report so you can have a better overview of what is working and what isn't. When you run all tests it's probably too much, but if you are running per directory or per tests it could be useful.
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.
I think this is a good idea that can be done when I get to writing the execution flags parts. Thank you!
Suggestion, leave in PR description the necessary steps for running the tests |
Parsing test files... |
Now that each test is one struct we can filter out repeated tests, a problem in the other runner that we have was that we were running some tests twice because some files were duplicated |
Motivation
Related issue: #3496.
The idea is to incrementally develop a new EF Test runner (for state tests) that can eventually replace the current one. The main goal of the new runner is to be easy to understand and as straightforward as possible, also making it possible to easily add any new requirement.
Considerations
This first iteration was developed using the following test files as reference:
vectors/state_tests/prague/eip2537_bls_12_381_precompiles/bls12_g1add/gas.json
vectors/GeneralStateTests/Cancun/stEIP4844-blobtransactions/blobhashListBounds9.json
vectors/LegacyTests/Cancun/GeneralStateTests/Cancun/stEIP1153-transientStorage/
directory.The main changes are:
Test
andTestCase
structures in types.Files that should not be reviewed as they are full or partial copies of the original files:
runner_v2/deserialize.rs
runner_v2/utils.rs
This iteration excludes report-related code, option flags and other possible test case errors to be considered that will be included later.