fix(spec-tools,testing): move evm_tools into the testing package - #3307
Conversation
69b0300 to
356f1c3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #3307 +/- ##
===================================================
+ Coverage 93.49% 93.53% +0.04%
===================================================
Files 624 624
Lines 37056 37074 +18
Branches 3394 3394
===================================================
+ Hits 34647 34679 +32
+ Misses 1653 1645 -8
+ Partials 756 750 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
One small thing (the diff view hides renamed file). The Evm docstring in evm_trace/protocols.py cross references the old module path, and now that the file lives in the testing package it is rendered by mkdocstrings rather than docc, here
A larger thing, can we remove b11r? I remember there was talk of security using it for fuzzing but I think they should or infact do use geth's now :)
Fixed the docstrings. Nice catch!!
We could do this but I'd defer to a follow-up PR |
There was a problem hiding this comment.
Thanks @gurukamath, the move looks clean and (bar 2. below) complete!!!
Details are in the inline comments. Two larger changes that could/should go in this PR:
- Move the new packaging job into a
just test-packagingrecipe (see the comment on the job). - Let
loaders/andutils.pyfollowevm_toolsinto the testing package (see the comment ontransaction_loader.py). This PR already breaks their import path, so one break beats two.
Follow-up: opened #3326 for the unrelated but never-runnable test_count_opcodes.py that was spotted during review.
| hex_to_uint, | ||
| ) | ||
| from ethereum_spec_tools.evm_tools.utils import parse_hex_or_int | ||
| from ethereum_spec_tools.utils import parse_hex_or_int |
There was a problem hiding this comment.
Suggestion (anchored here because the rest of the loader move is a pure rename in the diff): let loaders/ and utils.py follow evm_tools all the way into the testing package, in this PR, rather than stopping at ethereum_spec_tools top level.
Justification: After the move they have zero consumers left in the spec package: every user of loaders/ and utils.py is in packages/testing (11 import sites) or tests/json_loader (one import, Load in helpers/load_blockchain_tests.py). The only spec-internal use is this file importing utils.parse_hex_or_int, and it moves with them. forks.py is different and should stay, it is real spec API.
Why now? The import path already breaks in this PR either way: on base these lived at ethereum_spec_tools.evm_tools.loaders and evm_tools/utils.py, so any external consumer has to update regardless. Landing them at execution_testing.evm_tools.loaders and evm_tools/utils.py now means one break instead of two.
What the full move buys, beyond the cleaner boundary:
- 13 of the 16 new vulture whitelist entries go away outright (
Load, the sevenForkLoad.has_*, the fiveutilshelpers leavesrc/); the remaining three are covered by extending the deadcode scan (other comment). - The
"ethereum_spec_tools.loaders"line added to the setuptools packages list can be dropped again. - The new fork-selection branch in
tests/json_loader/helpers/select_tests.py:79-84can be reverted; it exists only because these files stayed behind. - The PR's story simplifies from "move
evm_tools, but split out its loaders and utils" to "moveevm_toolswholesale".
Cost is the same mechanical class as the rest of the PR: five files moved plus roughly a dozen import-site rewrites.
There was a problem hiding this comment.
The layout in the three states, for the visual argument.
- Base (
forks/amsterdam), everything under one roof:
src/ethereum_spec_tools/
├── __init__.py, forks.py, patch_tool.py, sync.py, py.typed
└── evm_tools/
├── __init__.py, __main__.py, daemon.py
├── utils.py ◄ inside evm_tools
├── b11r/ (__init__.py, b11r_types.py)
├── loaders/ (__init__.py, fixture_loader.py, ◄ inside evm_tools
│ fork_loader.py, transaction_loader.py)
├── statetest/ (__init__.py)
└── t8n/ (__init__.py, block_environment.py, cli.py,
result.py, evm_trace/ ×5)
- This PR as it stands,
evm_toolsmoves butloaders/andutils.pyare split out and promoted to spec-tools top level:
src/ethereum_spec_tools/
├── __init__.py, forks.py, patch_tool.py, sync.py, py.typed
├── loaders/ (__init__.py, fixture_loader.py, ◄ promoted, stayed behind
│ fork_loader.py, transaction_loader.py)
└── utils.py ◄ promoted, stayed behind
packages/testing/src/execution_testing/
└── evm_tools/
├── __init__.py, __main__.py, daemon.py
├── b11r/ (__init__.py, b11r_types.py)
├── statetest/ (__init__.py)
├── t8n/ (__init__.py, block_environment.py, cli.py,
│ result.py, evm_trace/ ×5)
└── tests/ (×3, moved from tests/evm_tools/)
- With the suggested move,
evm_toolsmoves wholesale and keeps its base-internal structure at the new address:
src/ethereum_spec_tools/
├── __init__.py
├── forks.py ◄ stays: real spec API
└── patch_tool.py, sync.py, py.typed
packages/testing/src/execution_testing/
└── evm_tools/
├── __init__.py, __main__.py, daemon.py
├── utils.py ◄ back where it was
├── b11r/ (__init__.py, b11r_types.py)
├── loaders/ (__init__.py, fixture_loader.py, ◄ back where it was
│ fork_loader.py, transaction_loader.py)
├── statetest/ (__init__.py)
├── t8n/ (__init__.py, block_environment.py, cli.py,
│ result.py, evm_trace/ ×5)
└── tests/ (×3)
Why the picture matters: state 3 is not a new reorganization on top of the PR; it is the PR's own move applied uniformly. evm_tools in state 3 has the identical internal shape it had on base (utils.py and loaders/ in their old positions relative to t8n/, b11r/, statetest/); only the package prefix changed. State 2 invents a layout that never existed before, loaders/ and utils.py at spec-tools top level. The leftover src/ethereum_spec_tools/ in state 3 is pure spec-serving tooling: fork enumeration (forks.py), lint, new-fork scaffolding, docs, sync.
There was a problem hiding this comment.
I think this one is genuinely a spec_tool which the t8n happens to use. tests/json_loader imports Load, and having that depend on the testing framework seems not right to me. Also, if we move this, that import would first execute execution_testing/__init__, which eagerly imports the whole framework.
There was a problem hiding this comment.
The only part of loaders I can chime in is fixture_loader.py, since the ideal solution is that both execution_testing and the specs import what currently is execution_testing.fixtures and work from there, but I think this requires a bigger refactoring than what we have in hand just now.
- add `just build-wheels`/`test-packaging`; the packaging CI job now delegates to it and the import walk lives in `.github/scripts/` - add `just check-testing-imports`: grep `src/` for execution_testing references, catching the function-scoped imports that a module-level import walk cannot see - move the t8n_build fixtures into the testing package's tests/fixtures - rename `tests/evm_tools` to `tests/spec_tools` - repoint the stale import-cycle comment pointers in `t8n/cli.py` at the surviving note in `result.py` - drop the broken `whitelist` entry point; `just whitelist` now runs the module via `python -m` - extend `just deadcode` to the moved evm_tools code and refresh the vulture whitelist accordingly - SHA-pin the `ethereum-spec-evm` docs link; drop the archived-EEST reference from the testing README - exclude `packages/testing/build/` from mypy: local wheel builds leave a setuptools tree there that shadows `execution_testing`
marioevz
left a comment
There was a problem hiding this comment.
LGTM, thanks for this effort!
I would just push myself a minor fix to enable packages/testing/src/execution_testing/evm_tools/tests/test_count_opcodes.py and then I will proceed to merge.
Description
pip install-ing this repository produces a brokenethereum-spec-evm: the t8nand statetest subcommands import
execution_testing(since #2924), which is auv workspace member — not a dependency of
ethereum-execution— so standaloneinstalls fail with
ModuleNotFoundErroron first use.This PR fixes the dependency inversion by moving
evm_tools(t8n, b11r,statetest, daemon) into the
ethereum-execution-testingpackage, so allimports point one way: testing → spec. The
ethereum-spec-evmentry pointmoves with it;
loaders/andutils.pystay inethereum_spec_tools.into a clean venv, and runs a real Frontier transition — the class of
regression that shipped No module named 'execution_testing' #3236 is now caught before merge.
packages/testing/README.md(also fixes its danglingreadme =metadata) with verified standalone install recipes, plus updatedrepo README, packaging docs, and an
evm_toolsreference page.Note for consumers: the CLI is no longer installable from the repo root alone —
install both packages from the same clone, e.g.
pip install ./execution-specs ./execution-specs/packages/testing.Related Issues or PRs
Fixes #3236.
Checklist
just static<type>(<area>): <title>, where<type>and<area>come from an appropriateC-<type>, respectivelyA-<area>, label. The title should match the target squash commit message.Cute Animal Picture