feat(test-consume): initial implementation of the enginex simulator#1964
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #1964 +/- ##
===================================================
+ Coverage 84.93% 85.97% +1.04%
===================================================
Files 599 599
Lines 36916 36916
Branches 3771 3771
===================================================
+ Hits 31354 31738 +384
+ Misses 4931 4560 -371
+ Partials 631 618 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
* chore(fw): remove fill warnings. * chore(fw): refactor.
953c821 to
97cf5e7
Compare
pytest-xdist is supported, but not optimised.
- Add commented-out enginex dev-mode matrix entry. - No release with enginex containing the new pre-alloc builder format (cf ethereum#2140).
b5a1811 to
0e87751
Compare
|
As-is this branch can be tested against hive master, but hiveview does not report the tests correctly. If you'd like to test end-to-end with hiveview, use this execution-specs branch (which adds two small commits to this PR's branch) and hive branch:
The hive branch needs a little clean-up, so not ready for review yet. But to test, need to checkout the hive The release is a minimal release just for testing, you can also use Then rebuild hiveview; each client log link now only highlights the relevant part of the log from that test: Example test summary and client logs in screenshots. Summary screenshot:
Log screenshot; only the lines corresponding to that test are show. |
There was a problem hiding this comment.
Review — consume enginex simulator
Reviewed by Claude (Opus 4.6) + Codex (GPT-5.3). Overall architecture is solid — the grouping-by-pre_hash, test sorting, xdist loadgroup integration, and engine_rpc extraction are all well done. Below are the findings, ordered by severity.
🔴 High
1. Cross-client reuse bug when multiple client types are parametrized (Codex)
MultiTestClientManager keys clients by pre_hash alone (enginex/conftest.py:154, multi_test_client.py:34). Since client_type is parametrized independently in consume.py:628-634, tests for client B could reuse client A's instance if they share the same pre_hash.
In practice Hive CI typically runs one client, but the code doesn't enforce that. Suggested fix:
# enginex/conftest.py:154
group_identifier = f"{client_type.name}:{fixture.pre_hash}"2. multi_test_hive_test always reports test_pass=True to Hive (Both)
<- Seems fine as is!pytest_hive.py:296-300 unconditionally sends test_pass=True at module teardown. The existing hive_test fixture inspects result_call.passed — this module-scoped fixture should track failure state similarly (e.g., via a pytest_runtest_makereport hook that sets a flag on the test object).
🟡 Medium
3. No resilience to client crash mid-group (Both)
get_client() (multi_test_client.py:46) returns the cached Client object without any liveness check. If a client dies, all remaining tests in that group cascade-fail with opaque connection errors. A lightweight TCP connect check (or even just a try/except around the first RPC call with a clear error message) would improve debuggability.
4. Unbounded session caches (Both)
pre_alloc_group_cache, client_genesis_cache, environment_cache (multi_test_client.py:158-173) are session-scoped dicts with no eviction. For large fixture sets, these retain heavy PreAllocGroup objects until session end. Could evict entries in mark_test_completed when a group completes.
5. assert used for runtime validation of external data (Both)
Several assert statements guard external fixture data — these are stripped by python -O:
consume.py:612—pre_hashpresence checkenginex/conftest.py:190— client startup checkmulti_test_client.py:271— ruleset key check
Suggest replacing with if not ...: raise ValueError(...) / raise RuntimeError(...).
🟢 Low / Nits
6. HIVE_GROUP_ID uses truncated hash (Both)
format_group_identifier() truncates to 16 chars (test_tracker.py:14), which is fine for log messages but is also used for the HIVE_GROUP_ID env var (multi_test_client.py:280). If this is used downstream as a unique identifier, consider using the full hash for the env var.
7. PR description says shared_hive_test, code says multi_test_hive_test (Claude)
Minor doc inconsistency in the PR description's "Modified" table.
8. Dual-yield in client fixture (Claude)
The client fixture (enginex/conftest.py:137-207) has two yield paths (reuse vs new). Works correctly but a single-yield refactor would be clearer — extract the resolution to a local variable, then have one try/yield/finally block.
9. isinstance branching in unified test function (Claude)
test_via_engine.py:78 and :217 use isinstance(fixture, BlockchainEngineFixture) to branch FCU behavior. A fixture-provided perform_fcu: bool flag would decouple the test logic from concrete fixture types. Not blocking, but worth considering if more modes are added.
10. Emoji in log messages (Claude)
♻️, 🚀, 🛑, ✓ in log messages (enginex/conftest.py, multi_test_client.py, test_tracker.py) can cause encoding issues in some CI log collectors. Consider plain text markers like [reuse], [start], [stop], [done].
|
I wanted to try claude/codex review skill here (thought it was a good candidate, not sure if I like it 😓)! I ran So the simple fix for 1) above wont work I believe. The grouping is by |
- Build combined `test_case`×`client_type` parametrization in `pytest_generate_tests`, attaching `xdist_group` marks inline so xdist's `loadgroup` scheduler reads them natively. - Simplify enginex `pytest_collection_modifyitems` to only read existing markers for counting and sorting (largest groups first). - Add `make_group_identifier()` helper for composite key construction (`pre_hash` + `client_name`).
…P_ID` - Remove `format_group_identifier()` which silently truncated the 18-char `pre_hash` (and lost the client name entirely from composite keys). - Use full group identifiers in all log messages. - Remove unused `HIVE_GROUP_ID` env var (nothing in hive reads it). - Collapse single-value f-strings where possible.
- Consolidate dual-yield paths (reuse vs new) into a single resolve-or-create branch followed by one `try`/`yield`/`finally`.
The enginex conftest both imported `MultiTestClientManager` and `TimingData` at module level and listed their modules in `pytest_plugins`. Python's import ran before pytest could register the modules for assert rewriting, producing: PytestAssertRewriteWarning: Module already imported so cannot be rewritten Fix by moving the imports behind `TYPE_CHECKING` and using string annotations in fixture signatures. Pytest resolves fixtures by parameter name, not type annotation, so the string literals work at runtime. Mypy/ruff still see the types via the `TYPE_CHECKING` block.
I think it's quite helpful, just a little noisy. I addressed what I thought was worthwhile in this PR - rationale below! I think they should only augment reviews; human reviews still catch the most important issues atm, imo. I additionally fixed the
1. Cross-client reuse bug when multiple client types are parametrized - FixedNice catch! The group identifier should have been a composition of pre-alloc group and client type. Fixed! There is another fix in this commit. The tests were already ordered in the enginex simulator by pre-alloc group (this ordering is likely present anyway, but it's essential in sequential mode to prevent the simulator from trying to switch between pre-alloc groups - spoiler, it can't, so it would just hang), but this change also revealed that the scheduling was extremely poor in non-sequential mode. So the commit addressing this also ordered the tests so that the largest pre-alloc groups come first. This increases our chances of splitting larger groups across workers with Worker 0: 1-test-group (geth); 1-test-group (reth) I.e., workers have imbalanced load leading to starvation. With group size ordering: Worker 0: 100-test-group (geth); 1-test-group (geth) This isn't the end solution though, as imbalances can still occur. xdist optimizations are coming in a follow-up PR. 2. multi_test_hive_test always reports test_pass=True to Hive (Both) - Retracted by reviewerN/A 3. No resilience to client crash mid-group - won't fixCould be more elegant, but the connection error from the first failed RPC call already points to the right cause. Let's see if this ever becomes an issue in practice. 4. Unbounded session caches - deferDeferring as more xdist optimization will come in subsequent PRs. But I think it's a fair concern! 5.
|
spencer-tb
left a comment
There was a problem hiding this comment.
Only thing I could spot! Previous issue looks solved from my side. Lets get this merged! 🚀
Co-authored-by: spencer <spencer.tb@ethereum.org>


🗒️ Description
Adds a
consume enginexsimulator that reuses client instances across tests sharing the same pre-allocation state. This simulator significantly reduces test execution time compared toconsume engineby avoiding per-test client startup overhead.Includes basic
pytest-xdistsupport via--dist=loadgroup, with further optimizations planned in subsequent PRs.Architecture
The
enginexsimulator groups tests bypre_hash(hash of genesis + pre-state). Tests in the same group run against a single client instance instead of starting/stopping per test.New Files
simulators/enginex/conftest.pysimulators/multi_test_client.pyMultiTestClientManagerfor client lifecyclesimulators/helpers/test_tracker.pyModified
consume.pyxdist_groupmarker based onpre_hashfor enginex fixturestest_via_engine.pyengineandenginexmodespytest_hive.pymulti_test_hive_testfixture for cross-test client sharingprocessors.py--dist=loadgroupdefault for enginex with xdistExecution Flow
pytest_collection_modifyitemscounts tests per group, sorts bypre_hash.mark_test_completedtriggers client shutdown.Fixture Chain
flowchart LR subgraph "Per Group (cached)" PAG[pre_alloc_group] --> CG[client_genesis] PAG --> GH[genesis_header] CG --> BG[buffered_genesis] BG --> CF[client_files] end subgraph "Client (reused)" CF --> C[client] C --> ETH[eth_rpc] C --> ENG[engine_rpc] end subgraph "Per Test" TC[test_case] --> F[fixture] F --> P[payloads] end subgraph "Test Execution" GH --> TEST[test_blockchain_via_engine] ENG --> TEST P --> TEST endTesting
Due to the pre-alloc optimizations from #2140 and the fact there's no release generated with this change yet (except bal@v5*), it's easiest to fill locally and test using those fixtures.
For example, fill the benchmark tests:
Then consume them:
🔗 Related Issues or PRs
N/A.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.[ ] All: Considered adding an entry to CHANGELOG.md.skippedCute Animal Picture