Skip to content

fix(testing): remove ignored StateTest engine_api_error_code#2740

Open
junbyjun1238 wants to merge 1 commit into
ethereum:forks/amsterdamfrom
junbyjun1238:fix/remove-ignored-statetest-engine-api-error-code
Open

fix(testing): remove ignored StateTest engine_api_error_code#2740
junbyjun1238 wants to merge 1 commit into
ethereum:forks/amsterdamfrom
junbyjun1238:fix/remove-ignored-statetest-engine-api-error-code

Conversation

@junbyjun1238
Copy link
Copy Markdown
Contributor

🗒️ Description

Remove engine_api_error_code from StateTest.

StateTest currently exposes engine_api_error_code, but the value is silently dropped during the StateTest -> Block conversion in state.py. The native state-fixture path does not consume it either, so the current API advertises a field that is effectively ignored.

This field no longer matches the current codebase's abstraction:

  • engine_api_error_code is modeled as block/payload metadata in blockchain.py and fixtures/blockchain.py
  • all current in-repo usages attach it to Block(...), not StateTest(...)
  • unlike block_exception, it is not wired through _generate_blockchain_blocks() and has no in-tree StateTest(...) call sites

Removing the field makes the API honest and avoids a footgun where a test author can specify an Engine API error expectation that is silently ignored. With BaseTest configured as extra="forbid", unsupported usage will fail fast at model construction rather than being silently ignored.

🔗 Related Issues or PRs

N/A.

✅ Checklist

  • All: Ran fast static checks to avoid unnecessary CI fails:
    just static
  • All: PR title adheres to the repo standard and starts with type(scope):.
  • All: Considered updating the online docs in ./docs/.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).

Cute Animal Picture

Cute rabbit

@felix314159
Copy link
Copy Markdown
Contributor

yes StateTest.engine_api_error_code is currently ineffective, but removing it changes the public API and may break downstream clients relying on it. i would prefer to just keep it as is for compatibility and close this PR, unless you can explain your motives for this change?

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.47%. Comparing base (40461a6) to head (b17b695).
⚠️ Report is 14 commits behind head on forks/amsterdam.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           forks/amsterdam    #2740      +/-   ##
===================================================
+ Coverage            86.26%   86.47%   +0.21%     
===================================================
  Files                  599      599              
  Lines                37038    37632     +594     
  Branches              3795     3795              
===================================================
+ Hits                 31949    32542     +593     
- Misses                4525     4526       +1     
  Partials               564      564              
Flag Coverage Δ
unittests 86.47% <ø> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@felix314159
Copy link
Copy Markdown
Contributor

interesting, CI passed. maybe it is fine to go ahead with this after all

@junbyjun1238
Copy link
Copy Markdown
Contributor Author

I've reviewed it again, and I now think the upside is mostly API cleanup while the downside is downstream compatibility risk — so I lean toward not making the change. The benefit seems smaller than the risk.
This PR wasn't driven by anything specific either. I noticed it while studying the codebase and opened a cleanup PR around it. I'll review the tradeoffs more carefully before opening similar PRs in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants