Skip to content

chore: switch ethereum state tests from ethereum/tests to EEST#501

Open
hai-rise wants to merge 3 commits intomainfrom
new-ethereum-tests
Open

chore: switch ethereum state tests from ethereum/tests to EEST#501
hai-rise wants to merge 3 commits intomainfrom
new-ethereum-tests

Conversation

@hai-rise
Copy link
Copy Markdown
Contributor

Replaces the deprecated ethereum/tests git submodule with fixtures downloaded from ethereum/execution-spec-tests (the current standard, now maintained in ethereum/execution-specs). Run once before testing:

bash scripts/fetch-eest-fixtures.sh

Key changes:

  • Remove the ethereum/tests submodule
  • Add scripts/fetch-eest-fixtures.sh to download fixtures_stable.tar.gz
  • Rewrite main.rs to walk state_tests/ instead of GeneralStateTests/, map all EEST exception strings (with pipe-separated OR support), and skip the same create-collision tests revm can't handle correctly
  • Add fixtures directory to .gitignore

Replaces the deprecated ethereum/tests git submodule with fixtures
downloaded from ethereum/execution-spec-tests (the current standard,
now maintained in ethereum/execution-specs). Run once before testing:

  bash scripts/fetch-eest-fixtures.sh

Key changes:
- Remove the ethereum/tests submodule
- Add scripts/fetch-eest-fixtures.sh to download fixtures_stable.tar.gz
- Rewrite main.rs to walk state_tests/ instead of GeneralStateTests/,
  map all EEST exception strings (with pipe-separated OR support), and
  skip the same create-collision tests revm can't handle correctly
- Add fixtures directory to .gitignore

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request replaces the Ethereum tests submodule with a script-based fixture download mechanism (fetch-eest-fixtures.sh) and refactors the test runner in crates/pevm/tests/ethereum/main.rs to map EEST exception strings to internal transaction errors. The changes also include updated test filtering and documentation. Feedback suggests improving error handling by returning a boolean instead of panicking on unknown exceptions to provide better assertion context, and removing an unsafe unwrap() call.

| InvalidTransaction::CallerGasLimitMoreThanBlock
)
}
_ => panic!("Unknown EEST exception: {exception}"),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of panicking on an unknown exception, returning false allows the assertion in run_test_unit to fail with a descriptive message including the file path and the actual error received. This makes debugging much easier when EEST introduces new exception strings.

Suggested change
_ => panic!("Unknown EEST exception: {exception}"),
_ => false,

),
spec_id,
build_block_env(&unit.env, spec_id),
vec![tx_env.unwrap()],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

With the suggested change to use let else for tx_env above, this unwrap() can be removed.

Suggested change
vec![tx_env.unwrap()],
vec![tx_env],

hai-rise and others added 2 commits April 26, 2026 23:14
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.

1 participant