more efExample tests and eof exceptions #550
Conversation
winsvega
force-pushed
the
efExample
branch
2 times, most recently
from
2c68f7c
to
5b4320f
Compare
| @@ -209,6 +209,9 @@ def list_header(sections: List["Section"]) -> bytes: | |||
| Creates the single code header for all code sections contained in | |||
| the list. | |||
| """ | |||
| if sections[0].skip_header_listing: | |||
There was a problem hiding this comment.
this is to allow types section to use skip_header_listing flag
| @@ -384,6 +391,874 @@ | |||
| EOFException.UNDEFINED_INSTRUCTION, | |||
| id="jump_jumpdest_fails", | |||
| ), | |||
| pytest.param( | |||
There was a problem hiding this comment.
We discussed with @marioevz and @danceratopz that this array with tests is not great. I don't mind some much the array but maybe it should be a separate test?
But what is bad that we add a ton of sophisticated tests to "example" category.
You should also add test cases to the EOF tracker file.
There was a problem hiding this comment.
I personally find it confusing when such an array is stored in another file, but here it is quite transparent
There was a problem hiding this comment.
I think the problem is that tests/prague/eip7692_eof_v1/eip3540_eof_v1/test_example_valid_invalid.py contains too many unrelated test cases.
For example it tries to cover many of the deprecated opcodes but not all of them.
We need to break up test_example_valid_invalid into different coherent test functions testing similar things, it's too big, and it makes it difficult to find a specific test case (also no longer an "example", it contains multiple valid tests).
| @@ -247,6 +247,10 @@ class EOFException(ExceptionBase): | |||
| """ | |||
| EOF container missing code section | |||
| """ | |||
| MISSING_DATA_SECTION = auto() | |||
There was a problem hiding this comment.
I think this is already in the main branch, so rebasing should remove this.
| name="EOF1I0015", | ||
| sections=[ | ||
| Section.Code( | ||
| code=Op.SELFDESTRUCT(1) + Op.STOP, |
There was a problem hiding this comment.
I don't understand the reason why we have two SELFDESTRUCT opcode tests here.
| "ef000102000100030400010000800001305000ef", | ||
| EOFException.MISSING_TYPE_HEADER, | ||
| id="bad_type_section_order_3", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_example_valid_invalid( |
There was a problem hiding this comment.
I think we should break this function up into different ones to make it easier to find implemented tests, and we can create different functions that really test similar things. E.g. we could make a test_deprecated_opcodes functions and parameterize it to each opcode that is deprecated in EOF (see V1_EOF_DEPRECATED_OPCODES in tests/prague/eip7692_eof_v1/eip3540_eof_v1/opcodes.py, which actually needs an update), and another one for header failures, etc.
Also, I think there's definitely duplicates between this file and file tests/prague/eip7692_eof_v1/eip3540_eof_v1/container.py, which also should be cleaned up, but perhaps in a follow up PR.
| ) | ||
| from ethereum_test_tools.eof.v1.constants import NON_RETURNING_SECTION | ||
|
|
||
| from .spec import EOF_FORK_NAME |
There was a problem hiding this comment.
| from .spec import EOF_FORK_NAME | |
| from .. import EOF_FORK_NAME |
On latest commit in main, this is where the EOF_FORK_NAME is actually defined.
| @@ -287,6 +291,10 @@ class EOFException(ExceptionBase): | |||
| """ | |||
| EOF container header does not have types section first | |||
| """ | |||
| INVALID_CODE_SECTION_INDEX = auto() | |||
There was a problem hiding this comment.
This is ok, but I think that now some tests in the tests/prague/eip7692_eof_v1/eip6206_jumpf/test_jumpf_execution.py should be updated because they no longer fit the EOFException.UNDEFINED_EXCEPTION definition (because their exception is now defined with this change).
🗒️ Description
add more eof exceptions and tests from efExample by Ori
🔗 Related Issues
✅ Checklist
mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.