feat(consume): allow geth to validate eof test vectors via direct (#1232) #1246
Conversation
felix314159
changed the title
felix314159
force-pushed
the
consume-eof-geth
branch
from
52140ef to
d08cdd7
Compare
felix314159
force-pushed
the
consume-eof-geth
branch
from
0724176 to
5e0bd80
Compare
danceratopz
added
type:feat
There was a problem hiding this comment.
Thanks for adding this @felix314159!
This needs to be tested against some test fixtures that are expected to fail, as
INFO [02-25|10:26:56.473] Executed tests passed=3 "total executed"=4
still leads to all fixtures in the file passing the test.
Two (obvious) comments:
- We're forced into hacky methods as the
eofparsetool doesn't provide any structured output. Perhaps we should add this to geth'seofparseinstead of writing brittle code in EEST. I looked at Besu's tool yesterday and they do return structured output. We could adopt this:
{
"fileName": "/home/dtopz/.cache/ethereum-execution-spec-tests/cached_downloads/v4.0.0/fixtures_eip7692/fixtures/eof_tests/osaka/eip7692_eof_v1/eip3540_eof_v1/eof_example/eof_example_custom_fields.json",
"group": "tests/osaka/eip7692_eof_v1/eip3540_eof_v1/test_eof_example.py::test_eof_example_custom_fields[fork_Osaka-eof_test]",
"name": "0",
"fork": "Osaka",
"pass": true,
"expectedError": null,
"actualError": null
}- Regardless of whether we improve
eofparseor not, better testing ofconsume directis a must. We can usepytester-level tests (which can test the behavior of entire pytest test sessions) to catch fails like the one above. I can start adding some in a separate PR.
Let's discuss 1. in our meeting today.
| ): | ||
| """Geth's implementation of the fixture consumer.""" | ||
|
|
||
| # ------------------------------------ state test --------------------------------------------- |
There was a problem hiding this comment.
I would prefer to remove these separators. Perhaps we need to organize the code into more logical groups if this class is getting out of hand.
| # ------------------------------------ state test --------------------------------------------- |
| ): | ||
| """Geth's implementation of the fixture consumer.""" | ||
|
|
||
| # ------------------------------------ state test --------------------------------------------- | ||
| @cache # noqa: B019 |
There was a problem hiding this comment.
Thanks for adding the code (B019) here. Did you see the pop-up (from the ruff extension)? Very helpful:
As we only have one instance of GethFixtureConsumer per consume run, I think we can leave it as-is. Open to other thoughts though.
There was a problem hiding this comment.
I think the memory leaks should not be a problem for consume since we have a single instance per run as you mention, and also consume is not a long-running process anyway, so it should be ok for the sake of code simplicity.
| @cache # noqa | ||
| def consume_state_test_file( | ||
| # ------------------------------------ eof test ----------------------------------------------- | ||
| def extract_int_from_subprocess_string( |
There was a problem hiding this comment.
How about?
| def extract_int_from_subprocess_string( | |
| def extract_test_count_from_eofparse_summary( |
| def extract_int_from_subprocess_string( | ||
| self, s: str, substring_of_interest_prefix: str | ||
| ) -> int | None: | ||
| """Hacky method to extract relevant substring from string returned by subprocess execution.""" # noqa: E501 |
There was a problem hiding this comment.
Typically one-liners should be concise and fit within our ruff-line-length (99). If clarification is needed, then it should be added as a separate block below. I think this is easy to fix in this case :)
| """Hacky method to extract relevant substring from string returned by subprocess execution.""" # noqa: E501 | |
| """Hacky method to extract test counts from `eofparse` console output.""" |
| ) -> int | None: | ||
| """Hacky method to extract relevant substring from string returned by subprocess execution.""" # noqa: E501 | ||
| # get everything after this substring occurrence, then remove everything not in [0,9] | ||
| relevant_substring = re.sub(r"\D", "", s.split('total executed"=', 1)[1]) |
There was a problem hiding this comment.
I think we need to raise an exception if the Executed tests line isn't found in the eofparse output.
There was a problem hiding this comment.
I think this function could be made more explicit by re-writing it to match 2 regex groups, one for the "passed" and one for the "total" count and return them both in a single call.
| all_tests_passed: bool = False | ||
| if total_tests_amount is not None and passed_tests_amount is not None: | ||
| if total_tests_amount == passed_tests_amount: | ||
| all_tests_passed = True |
There was a problem hiding this comment.
I think this will result in fixture file level pass/fail resolution and not give a detailed per test case (test fixture) resolution?
| ): | ||
| """Geth's implementation of the fixture consumer.""" | ||
|
|
||
| # ------------------------------------ state test --------------------------------------------- | ||
| @cache # noqa: B019 |
There was a problem hiding this comment.
I think the memory leaks should not be a problem for consume since we have a single instance per run as you mention, and also consume is not a long-running process anyway, so it should be ok for the sake of code simplicity.
| ) -> int | None: | ||
| """Hacky method to extract relevant substring from string returned by subprocess execution.""" # noqa: E501 | ||
| # get everything after this substring occurrence, then remove everything not in [0,9] | ||
| relevant_substring = re.sub(r"\D", "", s.split('total executed"=', 1)[1]) |
There was a problem hiding this comment.
I think it should be:
| relevant_substring = re.sub(r"\D", "", s.split('total executed"=', 1)[1]) | |
| relevant_substring = re.sub(r"\D", "", s.split(substring_of_interest_prefix, 1)[1]) |
There was a problem hiding this comment.
This one is failing after the change for me:
For s equal to 'INFO [03-17|17:33:22.741] Executed tests passed=4 "total executed"=4', relevant_substring becomes 44 when substring_of_interest_prefix=='tests passed='.
I think this function can be replaced with:
m = re.search(r'tests passed=(\d+) "total executed"=(\d+)', s)and then tests passed is int(m.group(1)) and tests executed is int(m.group(2)).
| def extract_int_from_subprocess_string( | ||
| self, s: str, substring_of_interest_prefix: str | ||
| ) -> int | None: |
There was a problem hiding this comment.
| def extract_int_from_subprocess_string( | |
| self, s: str, substring_of_interest_prefix: str | |
| ) -> int | None: | |
| @staticmethod | |
| def extract_int_from_subprocess_string( | |
| s: str, substring_of_interest_prefix: str | |
| ) -> int | None: |
This should be static since it doesn't access any instance properties, then you can call it with GethFixtureConsumer.extract_int_from_subprocess_string.
| def eofparse_subprocess_output_to_dict( | ||
| self, process_result: str, fixture_name: str | None, fork_name: str | ||
| ) -> Dict: |
There was a problem hiding this comment.
| def eofparse_subprocess_output_to_dict( | |
| self, process_result: str, fixture_name: str | None, fork_name: str | |
| ) -> Dict: | |
| @staticmethod | |
| def eofparse_subprocess_output_to_dict( | |
| process_result: str, fixture_name: str | None, fork_name: str | |
| ) -> Dict: |
Also can be an static method, because self.extract_int_from_subprocess_string can become GethFixtureConsumer.extract_int_from_subprocess_string if that method is also static, and then to call this one we would do GethFixtureConsumer.eofparse_subprocess_output_to_dict.
🗒️ Description
Geth supports eofparse and can be used to validate a given eof test (code valid yes/no without running the code). The command I used for fill was
uv run fill -vv -m "not slow" -m eof_test --fork=osaka --evm-bin=$HOME/Documents/evmone/build/bin/evmone-t8nand the consume command I tested the code with wasuv run consume direct --input=./fixtures/eof_tests --bin=$HOME/Documents/go-ethereum/build/bin/evm -s(I added-sto see the output of some print statements I put intest_via_direct.pyto see more detailed info about what is going on). It shows all 3895 tests passed in less than a minute, lmk what can be improved. Next I would try to do the same for consume direct via evmone.🔗 Related Issues
✅ Checklist
mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.