-
Notifications
You must be signed in to change notification settings - Fork 285
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
Export state tests with invalid txs #858
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #858 +/- ##
==========================================
+ Coverage 98.32% 98.37% +0.05%
==========================================
Files 129 129
Lines 15525 15536 +11
==========================================
+ Hits 15265 15284 +19
+ Misses 260 252 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
e49819f
to
5afb89e
Compare
cb87439
to
9b4d832
Compare
5afb89e
to
5e1b365
Compare
febfe66
to
1b2835f
Compare
5e1b365
to
b6b7a0a
Compare
Ready to review after rebase |
b6b7a0a
to
65bbf0a
Compare
|
||
if (holds_alternative<std::error_code>(res)) | ||
{ | ||
jpost["expectException"] = std::get<std::error_code>(res).message(); |
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 not the right error messages that are normal in tests.
Example of valid one: https://github.com/ethereum/tests/blob/1c23e3c27ac53b794de0844d2d5e19cd2495b9d8/GeneralStateTests/stExample/invalidTr.json#L26
Perhaps some conversion needed similar to
evmone/test/unittests/eof_validation.cpp
Line 14 in 9b3ee17
std::string_view get_tests_error_message(EOFValidationError err) noexcept |
The good place to find test error messages is probably retesteth config https://github.com/ethereum/retesteth/blob/fa5fe40627b0b4914295d5d0fe120dd0b053cd15/retesteth/configs/clientconfigs/evmone.cpp#L231
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.
@gumb0 Thanks for flagging this! So to be sure, what I need to do is:
- PR
retesteth
to add the additional entries - in this spot, call a new function like
get_tests_error_message(std::error_code)
, instead of just.message()
which has clauses like:
case INIT_CODE_EMPTY:
return "TR_InitcodeEmpty"; // <<<<<- one of the new `TR_` entries
to translate the code into retesteth-compatible value?
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.
yes, sounds good
I'm branching off of eof-create4, as this is where a bunch of exportable invalid tx tests were added.
I also tackled a "// TODO: Compare states carefully, they should be identical." for invalid txs - the same state comparison logic is executed now. This required a tiny amount of shuffling in how the default expectations on accounts like sender and coinbase work, but I think this is acceptable