test: Group the integration tests by the command they run - #1695
Merged
Conversation
The tests of every subcommand sat in one file, which reached sideways for its fixtures: the `evmone test` tests read `statetest/` and `blockchaintest/` while the `run` tests sat beside them. Each command now has a directory of its own under evmone-cli/, holding its fixtures, and integration/CMakeLists.txt names nothing but the directories. The 62 tests keep their names, so only the prefix moves. Two things fall out. t8n runs evmone-cli but was registered outside the `if(TARGET evmone-cli)` guard, which now stands where the decision is, around the directory itself. And the code file the @-prefix test reads is checked in beside that test rather than written at configure time, where nothing could read it. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
Contributor
Merging this PR will degrade performance by 3.57%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | baseline/analyse/main/sha1_shifts |
11.1 µs | 11.5 µs | -3.57% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing test/ctest-layout (c08ee00) with master (af66b3d)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1695 +/- ##
========================================
Coverage 97.98% 97.99%
========================================
Files 181 181
Lines 16199 16317 +118
Branches 3723 3739 +16
========================================
+ Hits 15873 15990 +117
Misses 248 248
- Partials 78 79 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
--histogram and --trace repeated in CMake what tracing_test.cpp already pins by calling the tracers with a stream of its own: the whole histogram table, and five lines of instruction trace. What only a run of the command shows is that the app-level flag reaches the VM, so that is all these two assert now. --version was the same test in two files. The app owns it. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
Four integration tests ran the command, then read the files it wrote to check a base fee, a rejected blob-create, and the empty requests hash. T8NArgs takes streams, so those assertions belong in the unit tests, where they need no output directory and no second process to cat it. The commands stay: what they still show is that --output.result reaches the file named on the command line, so each keeps a check that the file holds anything at all. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
Three more assertions needed only a library call: the post-state t8n writes to --output.alloc, what the state test runner reports for a transaction its fixture expected to be valid, and the loader's refusal of a block carrying expectException without rlp_decoded. The two transaction fixtures were a message and nothing else, so their commands go with them. The other two commands stay, thinned to what only running them shows: that the output file was written, and that the tool skips the whole file rather than failing it. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
The statetest --trace test spelled out six instruction lines and four regex escapes for them, which the tracing unit tests already pin. Its last line is the runner's own summary of the test, and nothing else reports that, so it is what the test keeps. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
The Prague fixture holds only the sender, so the block-end system calls found no code and the block was rejected with SYSTEM_CONTRACT_EMPTY. It still reported no requests and their hash, which is what both tests checked, so both passed for a reason neither named. The pre-state now holds the two request contracts, and the tests say the block is valid. The assertions along the way are parsed rather than matched: an exact key rather than a substring anywhere in the JSON, and the beacon-roots write inside the account whose storage it belongs to. The two txbytes fixtures were one case twice, and are now one function. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
Same as the Prague case beside it: the pre-state held only the sender, so the block-end system calls found no code and the block was rejected. The gas the test checks is reported either way, so it never noticed. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
Three assertions held whether or not their subject worked. Reading code from a file checked only that the run succeeded, which it does on the empty code an unread file gives; it now checks the gas that file's code costs. The EIP-7778 test asks for --trace and read nothing back, so the trace is checked, with the count standing in for the exit code a regex hides. The pre-state-only mode skips the block's system calls, and the case which names it borrowed the pre-state beside it, where the request contracts now make the block valid either way. It has a pre-state of its own again, holding only the sender, so the skipped calls are what keeps the block valid. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
There was a problem hiding this comment.
🟡 Changes recommended
Coverage-profile isolation and deterministic t8n fixture ordering must be restored before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Reorganizes integration tests by CLI command, relocates fixtures, and moves library-level assertions into focused unit tests.
Changes:
- Groups CLI tests under
run,t8n, andtest. - Adds targeted t8n, state-test, and blockchain-loader unit coverage.
- Strengthens previously ineffective integration checks.
- Requires fixes for lost coverage-profile settings and missing t8n fixture ordering.
File summaries
| File | Reviewed change / finding |
|---|---|
test/unittests/tooling_t8n_test.cpp |
Adds focused t8n unit tests. |
test/unittests/statetest_runner_test.cpp |
Tests invalid raw transactions. |
test/unittests/blockchaintest_loader_test.cpp |
Tests unsupported block loading. |
test/integration/t8n/prague_empty_requests/alloc.json |
Removes superseded fixture. |
test/integration/statetest/tx/invalid_signature.json |
Removes migrated fixture. |
test/integration/statetest/tx/invalid_encoding.json |
Removes migrated fixture. |
test/integration/evmone-cli/test/testcmd_fault/unrecognised_case.json |
Adds malformed-case fixture. |
test/integration/evmone-cli/test/testcmd_cases/case_after_exception.json |
Adds load-failure fixture. |
test/integration/evmone-cli/test/statetest/tests2/SuiteA/test1.json |
Relocates state-test fixture. |
test/integration/evmone-cli/test/statetest/tests1/SuiteA/test2_multi.json |
Adds multi-case fixture. |
test/integration/evmone-cli/test/statetest/tests1/SuiteA/test1.json |
Adds tracing fixture. |
test/integration/evmone-cli/test/statetest/tests1/SuiteA/notes.txt |
Adds ignored non-JSON fixture. |
test/integration/evmone-cli/test/statetest/tests1/SuiteA/index.json |
Adds ignored index fixture. |
test/integration/evmone-cli/test/statetest/tests1/B/T.json |
Adds empty fixture. |
test/integration/evmone-cli/test/statetest/filter/one_failing_of_two.json |
Adds filtering fixture. |
test/integration/evmone-cli/test/statetest/CMakeLists.txt |
Registers state-test integrations. |
test/integration/evmone-cli/test/CMakeLists.txt |
Groups test-driver cases; must restore per-process coverage profiles. |
test/integration/evmone-cli/test/blockchaintest/unsupported_rlp.json |
Adds unsupported-RLP fixture; metadata should describe loader rejection. |
test/integration/evmone-cli/test/blockchaintest/unrecovered_sender_deposit_log.json |
Tests deposit-log handling. |
test/integration/evmone-cli/test/blockchaintest/unrecovered_sender_blob_gas.json |
Tests blob-gas handling. |
test/integration/evmone-cli/test/blockchaintest/test.json |
Adds blockchain fixtures. |
test/integration/evmone-cli/test/blockchaintest/stops_at_one_test.json |
Tests per-case failure isolation. |
test/integration/evmone-cli/test/blockchaintest/not_json.txt |
Adds unloadable input. |
test/integration/evmone-cli/test/blockchaintest/eip7778_block_gas.json |
Covers EIP-7778 accounting. |
test/integration/evmone-cli/test/blockchaintest/CMakeLists.txt |
Registers blockchain integrations. |
test/integration/evmone-cli/t8n/prague_pre_state_only/txs.json |
Adds pre-state-only transactions. |
test/integration/evmone-cli/t8n/prague_pre_state_only/env.json |
Adds pre-state-only environment. |
test/integration/evmone-cli/t8n/prague_pre_state_only/alloc.json |
Adds pre-state-only allocation. |
test/integration/evmone-cli/t8n/prague_empty_requests/txs.json |
Relocates Prague transactions. |
test/integration/evmone-cli/t8n/prague_empty_requests/env.json |
Relocates Prague environment. |
test/integration/evmone-cli/t8n/prague_empty_requests/alloc.json |
Adds request-contract stubs. |
test/integration/evmone-cli/t8n/osaka_blob_schedule/txs.json |
Relocates Osaka transactions. |
test/integration/evmone-cli/t8n/osaka_blob_schedule/env.json |
Relocates Osaka environment. |
test/integration/evmone-cli/t8n/osaka_blob_schedule/blobParams.json |
Relocates blob parameters. |
test/integration/evmone-cli/t8n/osaka_blob_schedule/alloc.json |
Relocates Osaka allocation. |
test/integration/evmone-cli/t8n/CMakeLists.txt |
Registers t8n tests; one generated-output check needs fixture ordering. |
test/integration/evmone-cli/t8n/cancun_create_tx/txs.json |
Relocates Cancun transactions. |
test/integration/evmone-cli/t8n/cancun_create_tx/env.json |
Relocates Cancun environment. |
test/integration/evmone-cli/t8n/cancun_create_tx/alloc.json |
Relocates Cancun allocation. |
test/integration/evmone-cli/t8n/base_fee/txs.json |
Relocates empty transactions. |
test/integration/evmone-cli/t8n/base_fee/env.json |
Relocates fee environment. |
test/integration/evmone-cli/t8n/base_fee/alloc.json |
Relocates empty allocation. |
test/integration/evmone-cli/run/code.hex |
Checks in file-input bytecode. |
test/integration/evmone-cli/run/CMakeLists.txt |
Groups run tests; must restore per-process coverage profiles. |
test/integration/evmone-cli/CMakeLists.txt |
Defines CLI hierarchy; command-level tests lost coverage-profile isolation. |
test/integration/CMakeLists.txt |
Delegates CLI registration but drops coverage settings for relocated suites. |
Review details
Suppressed comments (1)
test/integration/evmone-cli/t8n/CMakeLists.txt:284
- This generated-output test is not attached to the
${TEST_CASE}fixture. With the repository's randomized parallel CTest run, it can execute before the producer above createsout.json(or consume a stale file). Add the same fixture ordering used bybody.rlpand every preceding generated-file check.
- Files reviewed: 18/46 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Letting the app take a second subcommand left the reversed exit-code test green: `run 6000 t8n` exits non-zero either way, because a t8n taken as a command is then rejected for the option it lacks. Giving it that option makes the exit code say which happened. The body of a block was checked for being hex, which the empty transaction list also is, and is what a run that never read the transactions writes. The encoding is fixed by the fixture, so it is what the test expects. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
The property naming a profile file per process reaches only the tests of the directory which sets it, so splitting the file left the fifteen tests which moved writing to one default file and overwriting each other's coverage. Each new directory sets it, as the ones beside them already did. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
Nothing removed the output files between runs, so a t8n which wrote nothing at all was checked against the previous run's: unbinding every output turned no test red on a build directory which had run before. The cases now wait on a step which removes those directories, and eight checks catch it. The check beside body.rlp never said it runs after the command which writes the file it reads, which a randomized parallel run is free to disregard. Claude-Session: https://claude.ai/code/session_016UHPAGwcwXMjqhTLpT31K7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The integration tests move to new locations, to reflect the current and future structure of the
evmone-clisubcommands.Some of the integration tests are converted to unit tests, which the current library design allows.