Skip to content

test(evm-core): fix stale delegation.rs test module#405

Open
manuelmauro wants to merge 1 commit into
rust-ethereum:v0.xfrom
moonbeam-foundation:fix/delegation-test-compile
Open

test(evm-core): fix stale delegation.rs test module#405
manuelmauro wants to merge 1 commit into
rust-ethereum:v0.xfrom
moonbeam-foundation:fix/delegation-test-compile

Conversation

@manuelmauro
Copy link
Copy Markdown
Contributor

Problem

evm-core's core/src/delegation.rs has a #[cfg(test)] mod tests that does not compile:

  • Delegation::try_from(&bytes) is called with bytes: Vec<u8> (so the argument is &Vec<u8>), but the impl is TryFrom<&[u8]> — trait selection does not deref-coerce &Vec<u8> to &[u8].
  • The assertions compare against Some(..) / None, but try_from returns Result<Delegation, DelegationError>.

The module has been broken since #367 first added the file — the test was written against an Option-returning draft of the Delegation API, while the merged impl returns Result via TryFrom.

Why CI didn't catch it

The root Cargo.toml is both a [package] (evm) and a [workspace], with no default-members. Per Cargo's rules, commands run from the root then default to the root package only — so cargo test (the workflow's only test step) never descends into evm-core, and cargo build compiles evm-core only as a dependency (a dependency's #[cfg(test)] modules are never built). cargo clippy --all lints lib/bin targets but not test targets.

It surfaces with:

cargo check --workspace --tests

Fix

  • Pass a &[u8] (bytes.as_slice()) so the TryFrom<&[u8]> impl is selected.
  • Compare against Ok(..) / Err(DelegationError::InvalidFormat).

Test-only change; no library code is touched.

Optional follow-up

Adding --workspace to the cargo test step in .github/workflows/rust.yml would catch this whole class of never-compiled test code across core/gasometer/runtime/fuzzer.

The `Delegation` API was refactored from an `Option`-returning conversion
to a `Result`-based `TryFrom<&[u8]>`, but the test module was never
updated to match:

- `try_from` was called with `&Vec<u8>`; the impl is `TryFrom<&[u8]>`,
  and trait selection does not deref-coerce `&Vec<u8>` to `&[u8]`. Pass
  `bytes.as_slice()` instead.
- assertions compared against `Some(..)`/`None`; `try_from` returns
  `Result`. Compare against `Ok(..)`/`Err(DelegationError::InvalidFormat)`.

The library itself compiles regardless, so `cargo check` without
`--tests` never caught this. Surfaced by `cargo check --workspace
--tests`.
manuelmauro added a commit to moonbeam-foundation/moonbeam that referenced this pull request May 18, 2026
- Add an evm row for the `delegation.rs` test-module fix
  (moonbeam-foundation/evm@a122857), upstreamed as rust-ethereum/evm#405.
- Correct the EIP-7939 row: PR #400 merged into `rust-ethereum/evm:v0.x`,
  so it is inherited from the upstream base, not a moonbeam cherry-pick
  (`Included` -> `Dropped`). Fix the matching "v1.0 only" claim in the
  Phase 1.2 plan.
manuelmauro added a commit to Moonsong-Labs/knowledge-work-plugins that referenced this pull request May 18, 2026
…k verification (#52)

## What

Adds a **"Verify the branch compiles"** step to the `qa-cherry-picks`
skill's `verify-cherry-picks.md`, requiring `cargo check --workspace
--tests` during fork-branch QA.

## Why

The verification flow confirmed cherry-picks via git but never checked
that the fork branch still built. Plain `cargo check --workspace` skips
`#[cfg(test)]` modules and integration tests, so a cherry-pick — or an
upstream refactor it lands on top of — can leave a test module that no
longer compiles while the check still reports success.

This was hit during the stable2603 cycle: evm's `evm-core`
`delegation.rs` test module had been broken since an `Option`→`Result`
API refactor, and a `--tests`-less check let it ride along undetected
from one stable branch to the next. (Since fixed upstream:
rust-ethereum/evm#405.)

---------

Co-authored-by: Rodrigo Quelhas <22591718+RomarQ@users.noreply.github.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