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.

Sign up for GitHub

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

Jump to bottom

evmone-statetest tool with JSON test loading #479

Merged
merged 5 commits into from
Jul 15, 2022
Merged

evmone-statetest tool with JSON test loading #479

merged 5 commits into from
Jul 15, 2022

Conversation

chfast
Copy link
Member

@chfast chfast commented Jun 17, 2022

No description provided.

@chfast chfast force-pushed the state_statetest branch 3 times, most recently from 9ec1223 to 48d216a Compare June 17, 2022 22:38
@chfast chfast requested review from axic and gumb0 June 17, 2022 22:38
Base automatically changed from state_mpt_hash to master July 6, 2022 11:30
@chfast chfast force-pushed the state_statetest branch 4 times, most recently from 32936b6 to 990b972 Compare July 6, 2022 22:01
@chfast chfast force-pushed the state_statetest branch 2 times, most recently from ba16bf7 to 0b3286d Compare July 7, 2022 14:53
gumb0
gumb0 reviewed Jul 14, 2022
View reviewed changes
test/statetest/statetest.hpp

struct TestCase
{
struct Case
Copy link
Member

Choose a reason for hiding this comment

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

Seems a bit inconsistent that TestCase has a vector of nested TestCase::Case, but StateTransitionTest has a vector of not-nested TestCase.

So maybe this shouln't be nested?
And renamin suggestion; Case => Expectation or CaseExpectation

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually moved structs into StateTransitionTest. Usually I don't do this but here this looks nice.

gumb0
gumb0 approved these changes Jul 14, 2022
View reviewed changes
Copy link
Member

@gumb0 gumb0 left a comment

Choose a reason for hiding this comment

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

Looks good overall.

Suprisingly not that many weird hacks required to parse a test.

@codecov
Copy link

codecov bot commented Jul 14, 2022 •
edited
Loading

Codecov Report

Merging #479 (88c21d3) into master (5742654) will decrease coverage by 0.10%.
The diff coverage is 96.72%.

❗ Current head 88c21d3 differs from pull request most recent head de64e67. Consider uploading reports for the commit de64e67 to get more accurate results

@@            Coverage Diff             @@
##           master     #479      +/-   ##
==========================================
- Coverage   99.55%   99.45%   -0.11%     
==========================================
  Files          46       51       +5     
  Lines        4992     5118     +126     
==========================================
+ Hits         4970     5090     +120     
- Misses         22       28       +6
Flag Coverage Δ
consensus 79.12% <ø> (ø)
statetests 7.67% <96.72%> (?)
unittests 99.59% <ø> (ø)

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

Impacted Files Coverage Δ
test/state/account.hpp 100.00% <ø> (ø)
test/statetest/statetest.cpp 90.47% <90.47%> (ø)
test/statetest/statetest_loader.cpp 97.91% <97.91%> (ø)
test/state/state.hpp 100.00% <100.00%> (ø)
test/statetest/statetest.hpp 100.00% <100.00%> (ø)
test/unittests/eof_test.cpp 97.05% <0.00%> (-2.95%) ⬇️
test/unittests/state_rlp_test.cpp 98.50% <0.00%> (-1.50%) ⬇️
test/utils/bytecode.hpp 97.05% <0.00%> (-0.58%) ⬇️
lib/evmone/vm.hpp 100.00% <0.00%> (+12.50%) ⬆️

@chfast chfast force-pushed the state_statetest branch 2 times, most recently from 53399fb to 88c21d3 Compare July 15, 2022 10:07
gumb0
gumb0 reviewed Jul 15, 2022
View reviewed changes
test/statetest/statetest_loader.cpp
o.block = from_json<state::BlockInfo>(j_t.at("env"));

for (const auto& [rev_name, expectations] : j_t.at("post").items())
o.cases.push_back({to_rev(rev_name),
Copy link
Member

Choose a reason for hiding this comment

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

emplace_back?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try, but this may not work because this class does not have explicit constructor. I believe this is fixed in C++20 or C++23 where S(a, b) is valid initialization for an aggregate S.

@chfast chfast merged commit 99d8773 into master Jul 15, 2022
@chfast chfast deleted the state_statetest branch July 15, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gumb0 gumb0 gumb0 approved these changes

@axic axic Awaiting requested review from axic

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chfast @gumb0