Move the exception handling logic from K to kevm-pyk#2816
Move the exception handling logic from K to kevm-pyk#2816
Conversation
juliankuners
left a comment
There was a problem hiding this comment.
see comments. Also, I did not deep dive into the testing mechanisms, so a second review might be nice.
| kore_pattern_list = [ | ||
| (name, kore) | ||
| for (name, kore) in iterate_gst( | ||
| (name, kore, exception_metadata) | ||
| for (name, kore, exception_metadata) in iterate_gst( | ||
| json_read, options.mode, options.chainid, options.usegas, schedule=options.schedule | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Can't this instead be re-written as this?
| kore_pattern_list = [ | |
| (name, kore) | |
| for (name, kore) in iterate_gst( | |
| (name, kore, exception_metadata) | |
| for (name, kore, exception_metadata) in iterate_gst( | |
| json_read, options.mode, options.chainid, options.usegas, schedule=options.schedule | |
| ) | |
| ] | |
| kore_pattern_list = list( | |
| iterate_gst( | |
| json_read, options.mode, options.chainid, options.usegas, schedule=options.schedule | |
| ) | |
| ) |
| except CalledProcessError: | ||
| if exception_expected: | ||
| _LOGGER.info(f'Test {name} failed as expected') | ||
| continue | ||
| else: | ||
| raise |
There was a problem hiding this comment.
If an exception is expected, but no CalledProcessError is thrown, this check will be skipped. If this needs to be enforced, it should, e.g., check whether an exception happened outside of the except block.
| kore_pattern_list = [ | ||
| (name, kore) | ||
| for (name, kore) in iterate_gst( | ||
| (name, kore, exception_metadata) | ||
| for (name, kore, exception_metadata) in iterate_gst( | ||
| json_read, options.mode, options.chainid, options.usegas, schedule=options.schedule | ||
| ) | ||
| ] |
There was a problem hiding this comment.
The same list comprehension as above can be applied here as well
| ethereum_cell = kevm_cell.args[5] # type: ignore[attr-defined] | ||
| evm_cell = ethereum_cell.args[0] # type: ignore[attr-defined] | ||
| status_code_cell = evm_cell.args[1] # type: ignore[attr-defined] | ||
| status_code = status_code_cell.args[0] # type: ignore[attr-defined] |
There was a problem hiding this comment.
These magic number indices look very susceptible to semantic changes to me. Can you replace them with a more reproducible mechanism like labels or something?
| elif exception_expected: | ||
| _assert_exit_code_exception(kevm_cell) | ||
| return |
There was a problem hiding this comment.
The function name _assert_exit_code_zero is not appropriate anymore when exception_expected = True.
Refactoring the logic that handles the exceptions in conformance tests and moving it from driver.md to the kevm-pyk test harness.
changes in driver.md:
exceptionproduction fromEthereumCommandsort and its associated rules from driver.md.statusproduction from driver.md#allPostKeyssetchanges to the GST iterator in kevm-pyk/src/kevm_pyk/interpreter.py:
added a
_resolve_exceptionfunction that reads theexpectExceptionandhasBigIntfields from the GST test json.the iterator now returns if the test is expected to fail.
changes in kevm-pyk/src/tests/utils.py:
optimizing the
_testfunction:adding new functions to fetch the cell value.
updating the
_assert_exit_code_zerofunction to check if the test should throw an exception when the interpreter returns a non-zero exit code.Removing the
expectExceptionfrom a test that is supposed to fail is caught.