-
Notifications
You must be signed in to change notification settings - Fork 302
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
test: Introduce TestState and TestAccount #811
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #811 +/- ##
=======================================
Coverage 95.14% 95.15%
=======================================
Files 138 140 +2
Lines 15766 15785 +19
=======================================
+ Hits 15001 15020 +19
Misses 765 765
Flags with carried forward coverage won't be shown. Click here to find out more.
|
78d4e78
to
9aa80be
Compare
43616e1
to
7b5c45d
Compare
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.
2 items to look at, otherwise makes sense to me
test/unittests/state_transition.cpp
Outdated
// Find unexpected storage keys. This will also report entries with value 0. | ||
EXPECT_TRUE(expected_acc.storage.contains(key)) | ||
<< "unexpected storage key " << key << "=" << value.current << " in " << addr; | ||
// FIXME: |
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.
is this fixme ok to be left behind? I'm staring at this and can't seem to figure it out, pls check
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.
No. I need to figure it out. This code has been changed since the start of this PR. The new types makes the comparison easier (e.g. TestState{} == TestState{}
) but we also want to have rich failure comments.
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.
Fixed. Mostly in #920.
This decouples the `state::State` for transaction execution from the state object for test definitions and asserts. The `state::State` (also called "intra state") has some constructs and data related to transaction execution only (like "current" and "original" storage values). These should not be exposed to tests and make test definitions more complicated. This separation should also help with adding new public API for EVM with transaction-level execution granularity.
This decouples the
state::State
for transaction execution from the state object for test definitions and asserts.The
state::State
(also called "intra state") has some constructs and data related to transaction execution only (like "current" and "original" storage values). These should not be exposed to tests and make test definitions more complicated.This separation should also help with adding new public API for EVM with transaction-level execution granularity.