feat(test-benchmark,test-fill): organize fixtures into gas-limit subdirs#2134
feat(test-benchmark,test-fill): organize fixtures into gas-limit subdirs#2134danceratopz wants to merge 4 commits intoethereum:forks/amsterdamfrom
Conversation
57092f9 to
635b120
Compare
635b120 to
070ec15
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2134 +/- ##
================================================
Coverage 86.07% 86.07%
================================================
Files 599 599
Lines 39472 39472
Branches 3780 3780
================================================
Hits 33977 33977
Misses 4862 4862
Partials 633 633
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:
|
spencer-tb
left a comment
There was a problem hiding this comment.
Another alternative structure would be the following, which has the benefit of keeping the existing structure, the test types staying at the same dir level:
fixtures/
├── blockchain_tests
│ ├── gas_limit_0001M/...
│ └── gas_limit_0002M/...
├── blockchain_tests_engine
│ ├── gas_limit_0001M/...
│ └── gas_limit_0002M/...
└── blockchain_tests_engine_x
├── pre_alloc/
├── gas_limit_0001M/...
└── gas_limit_0002M/...
I don't see any other issues here. The hasher would still work. Consume enginex could need a tweak in the future but not being used atm.
Another point: are those using the benchmarking tests happy with the structure as it might be a breaking change on there end.
|
Thanks for the review @spencer-tb! Yes, I think this is a better structure! |
|
Done! Also flattened one-level in the structure so it's clean. Description updated with verbose before and after trees! |
|
I don't think we should make this change in benchmark release layouts without considering the corresponding change for forks. The linear growth of our new fixtures files with each new fork, is the real issue. Let's clear that up first and then come back to this PR (which could increase in scope?). I made a discussion with poll for that here: |
🗒️ Description
--gas-benchmark-values.blockchain_tests_engine_x/pre_allocshared at the output root and reject--output=stdoutfor gas benchmark runs.The subdir routing happens in the base test parametrizer, right before
fixture_collector.add_fixture(...)decides the output path: https://github.com/danceratopz/execution-specs/blob/ade21760e4eb9dccc687b91d623d404bc1ed5ecf/packages/testing/src/execution_testing/cli/pytest_commands/plugins/filler/filler.py#L1554-L1576The logic for the per‑gas subdir is only applied when both are true:
--gas-benchmark-valuesis provided:GasBenchmarkValues.from_config(request.config) is not None.benchmark,stateful, orrepricing.Before
All fixtures for multiple gas limits were in the same files:
E.g.,
fixtures/blockchain_tests/benchmark/compute/instruction/arithmetic/arithmetic.jsoncontains both:tests/benchmark/compute/instruction/test_arithmetic.py::test_arithmetic[benchmark-gas-value_1M-fork_Osaka-blockchain_test-opcode_ADD-]tests/benchmark/compute/instruction/test_arithmetic.py::test_arithmetic[benchmark-gas-value_2M-fork_Osaka-blockchain_test-opcode_ADD-]After
Fixtures with different gas-limit parameters are organized by sub-folder as:
Changes
fillnow writes benchmark fixtures undergas_limit_XXXXM/(zero-padded, stable width).🔗 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):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.Cute Animal Picture