Skip to content

test(cli/mev): add integration tests for MEV test command#329

Open
varex83agent wants to merge 22 commits intomainfrom
feat/mev-tests
Open

test(cli/mev): add integration tests for MEV test command#329
varex83agent wants to merge 22 commits intomainfrom
feat/mev-tests

Conversation

@varex83agent
Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #300. Adds the integration tests missing from the initial MEV test implementation, as noted in the review thread.

  • default scenario: healthy mocked MEV node → Ping OK, PingMeasure Good, CreateBlock Skip
  • connection refused: two unreachable endpoints → Ping Fail, PingMeasure Fail, CreateBlock Skip with error messages
  • timeout: 100ns timeout → all network tests fail with timeout/interrupted verdict
  • quiet mode: --quiet produces no stdout output
  • unsupported test case: unknown test name returns "test case not supported" error
  • custom test cases: --test-cases=Ping runs only Ping per endpoint
  • write to file: --output-json produces valid JSON with mev key

Tests ported from testmev_internal_test.go. The default load scenario and flag-parsing tests are deferred to a further follow-up since they require a live beacon node or deeper CLI wiring.

Test plan

  • cargo test --package pluto-cli --bin pluto -- commands::test::mev::tests passes (9/9)
  • cargo clippy clean
  • cargo +nightly fmt clean
  • cargo deny check clean

🤖 Generated with Claude Code

PoulavBhowmick03 and others added 16 commits March 28, 2026 13:09
- Follow the `test validator` approach (enum + impl block)
- Simplify error handling for timeouts
- Test is already cancelled by caller appropriately
- Workaround for false positive
- Use tokio::time::Instant instead of std::time::Instant in async code
- Replace CliError::Other string literals with typed variants
  TimeoutInterrupted and TestCaseNotSupported
- Remove TestResultError from imports; use CliError::TimeoutInterrupted directly
- Simplify latest_beacon_block and fetch_proposers_for_epoch to use ?
  operator, leveraging existing From impls on CliError
- Simplify get_block_header error conversions with .map_err(MevError::from)
  and .map_err(|e| MevError::Cli(e.into()))

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
RUSTSEC-2026-0097: rand 0.8.x unsoundness (transitive dep)
RUSTSEC-2026-0098: rustls-webpki URI name constraint bypass (transitive dep)

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
These features are absent from Charon v1.7.1:
- Remove x-timeout-ms CLI flag and x_timeout_ms field from TestMevArgs
- Remove X-Timeout-Ms and Date-Milliseconds headers from get_block_header
- Remove credential parsing (parse_endpoint_credentials / apply_basic_auth)
  from mev_ping_test, latest_beacon_block, fetch_proposers_for_epoch,
  and get_block_header
- Replace x_timeout_ms-based thresholds in mev_create_block_test with
  fixed 500ms/800ms constants matching Go reference thresholds

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
Resolve conflicts:
- deny.toml: adopt main's detailed reason comment for RUSTSEC-2026-0097, keep RUSTSEC-2026-0098
- crates/cli/Cargo.toml: add chrono from main
- crates/cli/src/commands/test/mod.rs: accept main's refactor (helpers/constants modules)
- crates/cli/src/commands/test/mev.rs: update imports for helpers/constants split,
  use SLOTS_IN_EPOCH.get() for NonZero<u64>

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
Port tests from testmev_internal_test.go covering:
- default scenario: healthy MEV node returns Ping OK, PingMeasure Good, CreateBlock Skip
- connection refused: Ping and PingMeasure fail with error, CreateBlock skips
- timeout: very short timeout causes Fail verdicts for network tests
- quiet mode: no stdout output when quiet=true
- unsupported test case: returns "test case not supported" error
- custom test cases filter: only specified tests are run
- write to file: output_json path produces valid JSON with mev key

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
rustls-webpki wildcard name constraint bypass — transitive dep,
cannot upgrade immediately.

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
Base automatically changed from poulav/mev_test to main April 16, 2026 15:23
Comment thread crates/cli/src/commands/test/mev.rs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can silently fail, would be better to return an error.

Comment thread crates/cli/src/commands/test/mev.rs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

Comment thread crates/cli/src/commands/test/mev.rs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

those two as well, why not return an error?

Comment thread crates/cli/src/commands/test/mev.rs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

Comment thread crates/cli/src/commands/test/mev.rs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

Comment thread crates/cli/src/commands/test/mev.rs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Contributor

@mskrzypkows mskrzypkows left a comment

Choose a reason for hiding this comment

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

LGTM, I would just change some unwraps to returning errors in case this code would be used in different context.

@varex83
Copy link
Copy Markdown
Collaborator

varex83 commented Apr 28, 2026

@claude merge main and fix comments

@claude
Copy link
Copy Markdown

claude Bot commented Apr 28, 2026

Claude finished @varex83's task in 4m 36s —— View job


Tasks

  • Gather context and understand request
  • Read crates/cli/src/commands/test/mev.rs to identify the flagged unwrap_or calls
  • Fix review comments (replace unwrap_or fallbacks with proper error returns)
  • Commit and push changes
  • [⚠️] Merge maingit merge requires explicit approval in this environment; please run git merge origin/main locally or re-trigger with that permission granted.

What was done

Replaced all six unwrap_or/unwrap_or_default fallbacks flagged by the reviewer with explicit error returns:

Location Old New
mev_create_block_test L308–310 .unwrap_or(UNIX_EPOCH) match … { None => return test_res.fail(…) }
mev_create_block_test L311–313 .unwrap_or(latest_block_ts) match … { None => return test_res.fail(…) }
mev_create_block_test L325–326 try_from(…).unwrap_or(i64::MAX) / checked_div(…).unwrap_or(0) two match arms returning test_res.fail(…)
mev_create_block_test L366 try_from(…).unwrap_or(u64::MAX) match … { Err(e) => return test_res.fail(…) }
mev_create_block_test L400 try_from(…).unwrap_or(u32::MAX) match … { Err(e) => return test_res.fail(…) }
create_mev_block L529–530 unwrap_or(i64::MAX) / unwrap_or(0) .map_err(…)? / .ok_or_else(…)?

Commit: 89d39d2
· Branch

github-actions Bot and others added 3 commits April 28, 2026 10:26
Replace silent unwrap_or fallbacks on checked arithmetic and numeric
conversions with explicit error returns, so callers observe failures
instead of silently continuing with sentinel values.

Co-authored-by: Bohdan Ohorodnii <varex83@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
- Adopt CancellationToken by-value signature from main
- Keep proper error propagation from PR (replace unwrap_or fallbacks)
- Update test call sites to CancellationToken::new() by value
- Adopt main's test naming convention

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
Move the local MevError out of mev.rs into the public MevTestError type
in error.rs, and add CliError::MevTest(#[from] MevTestError). The Cli
wrapper variant is dropped — get_block_header now returns Result<_, CliError>
directly, letting ? propagate reqwest/serde errors and using
MevTestError::StatusCodeNot200 for the non-200 retry path.

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
…r variants

Add 13 new MevTestError variants covering every CliError::Other site in mev.rs:
HttpStatus, BeaconNodeEndpointRequired, BeaconNodeEndpointNotAllowed,
ParseTimestamp, TimestampOverflow, NextBlockTimestampOverflow, ParseSlot,
SlotsInEpochConversion, EpochCalculationOverflow, ElapsedNanosConversion,
BlockCountConversion, SlotNotFound, PayloadMarshal,
UnsupportedVersionOrMissingHeader. Remove the now-dead http_status_error helper.

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
@varex83
Copy link
Copy Markdown
Collaborator

varex83 commented Apr 29, 2026

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 29, 2026

Claude finished @varex83's task in 3m 3s —— View job


Code Review

Tasks

  • Gather context
  • Read changed files and diff
  • Review crates/cli/src/commands/test/mev.rs
  • Review crates/cli/src/error.rs
  • Review deny.toml
  • Post final review

Overall the PR is solid — good test coverage for the non-load-test paths, clean MevTestError extraction, and the error-handling improvements from the previous review are well done. A few issues worth addressing before merge:


Bugs / Issues

1. test_mev_timeout doesn't test timeout — it tests connection refused

mev.rs:803-827

With StdDuration::from_nanos(100) and unreachable ports, the reqwest calls fail with "connection refused" before the cancellation token can fire. Both paths happen to produce TestVerdict::Fail, so the test passes — but it never actually exercises the token.cancelled() branch or the TimeoutInterrupted code path.

To actually test timeout you'd need an endpoint that accepts the TCP handshake but delays sending a response (e.g., a MockServer with a ResponseTemplate::new(200).set_delay(Duration::from_secs(10))). As-is this is a duplicate of test_mev_connection_refused with a misleading name.

Fix this →


2. Hardcoded ports are fragile

mev.rs:775, 806, 834, 848, 863, 886

Eleven consecutive ports (19950–19960) are hardcoded under the assumption nothing is listening on them. If any service happens to occupy those ports (other test processes, CI environment services), tests expecting Fail will instead see a successful connection and fail non-deterministically.

The standard fix is to let the OS pick: bind 127.0.0.1:0, record the assigned port, then drop the listener before the test runs (ensuring connection refused).

fn refused_addr() -> String {
    let l = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
    let addr = l.local_addr().unwrap();
    drop(l); // port is now free but OS won't have reused it immediately
    format!("http://{addr}")
}

Fix this →


Test Quality

3. test_mev_default_scenarioPingMeasure::Good can be flaky

mev.rs:766

("PingMeasure", TestVerdict::Good),

Good requires avg RTT ≤ 40ms (THRESHOLD_MEV_MEASURE_AVG). On a loaded CI runner this could return Poor instead. Consider asserting verdict != TestVerdict::Fail (or at least verdict != TestVerdict::Skip), since the goal is just to confirm the mock server is reachable and the path succeeds.

Fix this →

4. test_mev_quiet — JSON file not verified

mev.rs:829-843

The test sets output_json to a file path but only checks buf.is_empty(). It never asserts the file was written or has valid content, which means the write_result_to_file path isn't tested here at all (it's covered by test_mev_write_to_file anyway, but the quiet-mode file write isn't explicitly verified).


Minor Issues

5. deny.toml — missing reason field on new entries

deny.toml:34, 36

The existing entry uses { id = "...", reason = "..." }. The two new entries have a comment above but no inline reason:

# existing pattern:
{ id = "RUSTSEC-2026-0097", reason = "transitive rand <0.9.3 via cuckoofilter ..." },

# new entries (inconsistent):
{ id = "RUSTSEC-2026-0098" },
{ id = "RUSTSEC-2026-0099" },

Add reason fields for consistency and machine-readability.

Fix this →

6. Double blank line before #[cfg(test)]

mev.rs:689-692

}


#[cfg(test)]

Two blank lines instead of one — cargo +nightly fmt --check will flag this.

Fix this →

7. Remaining unwrap_or calls in production code

mev.rs:357, 368, 369, 372, 405, 600

Several unwrap_or calls were not converted. Most are infallible in practice (e.g. u32 → usize on 64-bit, SLOT_TIME constant, divisors guaranteed non-zero), so they won't silently fail. But they're inconsistent with the stated goal of this PR and the reviewer's original ask. Worth converting the non-obvious ones, particularly:

  • L357: usize::try_from(conf.number_of_payloads).unwrap_or(usize::MAX)u32 → usize can't fail on any platform Pluto targets, but an explicit assertion or comment would clarify the intent.
  • L368: u64::try_from(SLOT_TIME.as_nanos()).unwrap_or(1)SLOT_TIME is a constant; unwrap() or a compile-time assertion would be more honest.

What's Good

  • MevTestError moved to error.rs alongside other domain errors — clean.
  • get_block_header return type simplified to Result<...> — the From<reqwest::Error> for CliError chain makes this clean.
  • assert_verdict helper is reusable and the tests are well-structured.
  • The match on Err(CliError::MevTest(MevTestError::StatusCodeNot200)) in create_mev_block correctly threads the error variant through the new type hierarchy.
    · Branch

- test_mev_timeout: use MockServer with 500ms delay and 10ms timeout to
  actually exercise the token.cancelled() / TimeoutInterrupted path
- Replace hardcoded ports 19950-19960 with refused_addr() helper that
  binds :0 and drops the listener, giving OS-assigned ports that are
  guaranteed available
- test_mev_default_scenario: assert PingMeasure is Good|Poor instead of
  exact Good to avoid latency-sensitive flakiness on loaded CI runners
- Replace infallible u32→usize and SLOT_TIME→u64 try_from/unwrap_or with
  direct as casts
- deny.toml: add reason fields to RUSTSEC-2026-0098 and RUSTSEC-2026-0099

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
@varex83
Copy link
Copy Markdown
Collaborator

varex83 commented Apr 29, 2026

@claude re-review please, check if all issues were addressed

@claude
Copy link
Copy Markdown

claude Bot commented Apr 29, 2026

Re-review: Checking if all issues were addressed

Tasks

View job run

- Revert SLOT_TIME as-cast to try_from/unwrap_or to satisfy
  clippy::cast_possible_truncation
- Rename test functions to drop redundant `test_` prefix
  (clippy::redundant_test_prefix)

Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@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.

5 participants