Skip to content
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

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Conversation

pdobacz
Copy link
Collaborator

@pdobacz pdobacz commented Apr 10, 2024

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

Copy link

codecov bot commented Apr 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.37%. Comparing base (ad99657) to head (65bbf0a).

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     
Flag Coverage Δ
statetests 27.77% <20.00%> (-0.01%) ⬇️
statetests-silkpre 18.81% <20.00%> (+<0.01%) ⬆️
unittests 94.17% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
test/statetest/statetest_runner.cpp 96.87% <100.00%> (+0.32%) ⬆️
test/unittests/state_transition.cpp 98.38% <100.00%> (+0.03%) ⬆️
test/unittests/state_transition.hpp 0.00% <ø> (ø)
...est/unittests/state_transition_eof_create_test.cpp 100.00% <100.00%> (ø)
test/unittests/state_transition_tx_test.cpp 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@pdobacz pdobacz force-pushed the invalid-tx-test-export branch 2 times, most recently from e49819f to 5afb89e Compare April 10, 2024 11:39
@gumb0 gumb0 force-pushed the eof-create4 branch 4 times, most recently from cb87439 to 9b4d832 Compare April 17, 2024 13:46
@chfast chfast added the tests Testing infrastructure label Apr 17, 2024
@gumb0 gumb0 force-pushed the eof-create4 branch 2 times, most recently from febfe66 to 1b2835f Compare April 18, 2024 13:15
Base automatically changed from eof-create4 to master April 18, 2024 13:35
@pdobacz
Copy link
Collaborator Author

pdobacz commented Apr 18, 2024

Ready to review after rebase

test/unittests/state_transition.cpp Show resolved Hide resolved
test/unittests/state_transition.cpp Outdated Show resolved Hide resolved
test/unittests/state_transition.cpp Show resolved Hide resolved
test/unittests/state_transition_tx_test.cpp Outdated Show resolved Hide resolved
@pdobacz pdobacz merged commit 9b3ee17 into master Apr 19, 2024
24 checks passed
@pdobacz pdobacz deleted the invalid-tx-test-export branch April 19, 2024 15:23

if (holds_alternative<std::error_code>(res))
{
jpost["expectException"] = std::get<std::error_code>(res).message();
Copy link
Member

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

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

Copy link
Collaborator Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Testing infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants