test: Move the state diff revert test to state_transition - #1717
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1717 +/- ##
==========================================
- Coverage 98.02% 98.02% -0.01%
==========================================
Files 180 179 -1
Lines 16379 16382 +3
Branches 3769 3769
==========================================
+ Hits 16056 16058 +2
- Misses 243 244 +1
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
e66a54c to
d36175a
Compare
d36175a to
be32086
Compare
The test added with the fix builds its own state, block and transaction instead of using the state_transition fixture the rest of the state tests use. Move it to state_transition_touch_test.cpp, next to the other touch-and-revert cases. The post state alone cannot express what the test checks: applying a diff entry for an account which does not exist is a no-op, so a spurious entry leaves the post state correct. The fixture gets an `in_diff` expectation for that, used in three places: the reverted cold access (false), an EIP-161 sweep of a pre-existing account (true, a deletion) and a value transfer (true, a modification).
be32086 to
5a2c928
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Add the direct <algorithm> include required by std::ranges::find.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Moves the state-diff regression test into the shared state_transition fixture and adds account-level diff expectations.
Changes:
- Adds optional
in_diffvalidation. - Migrates touch/revert and value-transfer coverage.
- Removes the superseded standalone test and build entry.
File summaries
| File | Summary |
|---|---|
test/unittests/state_transition.hpp |
Adds diff expectation metadata. |
test/unittests/state_transition.cpp |
Validates expected diff membership; requires a direct <algorithm> include. |
test/unittests/state_transition_touch_test.cpp |
Adds migrated regression and deletion coverage. |
test/unittests/state_transition_call_test.cpp |
Adds value-transfer diff coverage. |
test/unittests/state_diff_test.cpp |
Removes the superseded standalone test. |
test/unittests/CMakeLists.txt |
Removes the deleted test source. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follow-up to #1709, which added the regression test as a standalone
TEST()building its own state, block and transaction instead of using thestate_transitionfixture the rest of the state tests use.Moves it to
state_transition_touch_test.cpp, next to the other touch-and-revert cases.The post state alone cannot express what the test checks: applying a diff entry for an account which does not exist is a no-op, so a spurious entry leaves the post state correct. The fixture gets an
std::optional<bool> in_diffexpectation for that, used in three places:in_difftouch_revert_cold_access_nonexistent0x4efalsetouch_empty_sd0xeetruedeleted_accountsentrycall_value_to_empty0xbetruemodified_accountsentryThe first two make a matched pair: both end non-existent in the post state, and only the diff tells them apart.
in_diffis anoptional<bool>rather than a plain flag so the same shape can later cover storage entries.Verified by reverting the
host.cppchange from #1709: the migrated test fails with0x4e: unexpectedly in the state diffwhile every other touch test andcall_value_to_emptystill pass. Each of the three expectations was also flipped to the wrong value to confirm it asserts something.Note: the account-level diff is not consistent beyond this case — see #1718.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DV45qzgCspEBfY2es9c7SL