Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
@chfast chfast force-pushed the state_statetest branch from 48d216a to 828e058 Compare June 20, 2022 08:24
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 added 3 commits July 7, 2022 00:57
@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 force-pushed the state_statetest branch from 88c21d3 to de64e67 Compare July 15, 2022 16:31
@chfast chfast merged commit 99d8773 into master Jul 15, 2022
@chfast chfast deleted the state_statetest branch July 15, 2022 16:43
# for free to join this conversation on GitHub. Already have an account? # 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