-
Notifications
You must be signed in to change notification settings - Fork 70
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
new(tests): Exported Evmone EOF unit tests #786
base: main
Are you sure you want to change the base?
Conversation
a5afd73
to
606bf05
Compare
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.
Very nice work overall!
I haven't finished the review but I thought of sharing the comments I have so far in order to start the rework earlier.
Overall general comments:
- We should reduce the number of files, since the generated test case names already contain the test file name and the test function name, many of the test cases could be placed in existing files (primarily
tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_container_validation.py
) without risking name collisions. - On the new files, we should skip the
test_eof1_
prefix and instead simply usetest_
. - Linter is currently complaining about the format of many of the files. One simple solution is to run the
black
python formatting tool (seeuv run black --help
) on the new files.
[ | ||
pytest.param( | ||
Container( | ||
name="EOFV1_0001", |
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 file can be removed since it's very similar to tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_container_validation.py::test_invalid_containers[fork_Osaka-eof_test-single_code_section_no_data_section]
with only difference being that this one uses Op.INVALID
instead of Op.STOP
.
@@ -0,0 +1,225 @@ | |||
"""" |
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.
The linter is complaining about this.
"""" | |
""" |
|
||
REFERENCE_SPEC_GIT_PATH = "EIPS/eip-3540.md" | ||
REFERENCE_SPEC_VERSION = "12ca2f0bd2f7380100e154aaaa0995c46cbb2710" | ||
|
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 could be done in a more pythonic way by doing a pytest.mark.parametrize
of only the Opcode, then in the test function we construct the container, and that way the test's id should be automatically obtained from the Op.__name__
.
Another thing is that we could (also within the test function) inspect the opcode's min_stack_height
property and add some Op.PUSH0
before the opcode in order to try to trick a faulty implementation into passing the stack check (max_stack_height
should be automatically calculated also, even with these opcodes that are unsupported).
Also we have this list of opcodes in tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_all_opcodes_in_container.py
, it's named invalid_eof_opcodes
, so we could simply use that by placing this test in that file.
] | ||
) | ||
|
||
def test_example_valid_invalid( |
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.
Once refactored, this can be renamed to something like test_deprecated_instructions
.
0x02, | ||
0x00, | ||
0x00, | ||
0x00, |
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 could add another flavor to this test where the data section header is not missing, and also we have the actual body of the type section.
]), | ||
) | ||
, | ||
"ef000101000403000102000100010000800000aafe", |
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.
This one is very similar to the container generated by tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_section_order.py::test_section_order[fork_Osaka-eof_test-test_position_CasePosition.BODY_AND_HEADER-section_test_SectionTest.WRONG_ORDER-section_kind_CODE]
:
ef0001 010004 04 0001 02 0001 0003 00 00800001 ef3050 00
I think it should be ok to just keep that one since it forms part of the parametrization of a function already, we could remove this file.
]), | ||
) | ||
, | ||
"ef0001040001010004020001000100aa00800000fe", |
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 previous comment, seems to be covered by tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_section_order.py::test_section_order[fork_Osaka-eof_test-test_position_CasePosition.BODY_AND_HEADER-section_test_SectionTest.WRONG_ORDER-section_kind_DATA]
.
"ef00010100040200010001040001feaa", | ||
EOFException.MISSING_TERMINATOR, | ||
id="eof1_header_not_terminated_6" | ||
), |
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.
These are very similar to no_section_terminator*
tests in tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_container_validation.py
, I think we can simply add these two cases in that same file as parametrization values to test_invalid_containers
] | ||
) | ||
|
||
def test_example_valid_invalid( |
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.
These are in the same category as the invalid_first_code_section_*
tests in tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_container_validation.py
, but in this case I think it's enough amount of test cases to move the ones in that file into this new file.
A couple of things:
- This file could be renamed to
test_first_code_section_type.py
- We could rename this test function to
test_invalid_containers
- The docstring of this test function could be a bit more descriptive to account for the type of testing that is being performed.
] | ||
) | ||
|
||
def test_example_valid_invalid( |
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.
Very nice test scenarios.
Similar to previous comment, we could:
- Rename file to
test_type_section_size.py
- Rename test function to
test_invalid_containers
- Improve the docstring description
In general I think the test_eof1
prefix for file names in this folder is redundant since the version of EOF containers we are testing here can be implied from the parent folders' names.
ποΈ Description
π Related Issues
β Checklist
mkdocs serve
locally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.