Conversation
Added url parsing and basic auth for request_rrt function.
There was a problem hiding this comment.
Pull request overview
Ports the beacon-node CLI test suite into pluto-cli, implementing connectivity/latency/load/simulation checks against beacon endpoints (issue #233).
Changes:
- Implemented full
alpha test beaconcommand logic (test discovery, execution, scoring, JSON output, optional publish). - Added endpoint URL credential parsing + optional HTTP Basic Auth application for test requests.
- Extended
Durationwrapper to support ordering and nanosecond comparisons used by simulation evaluation.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cli/src/main.rs | Initializes tracing for alpha test command path. |
| crates/cli/src/duration.rs | Adds as_nanos, Ord, and conversion to std::time::Duration to support simulation metrics. |
| crates/cli/src/commands/test/mod.rs | Beacon test case listing; adds endpoint URL credential parsing + basic auth; adjusts test filtering API; updates request RTT path. |
| crates/cli/src/commands/test/beacon.rs | Replaces stub with full beacon test suite implementation (ping/measure/version/sync/peer-count/load/simulation). |
| crates/cli/Cargo.toml | Adds tracing-subscriber dependency needed by CLI tracing init. |
| Cargo.lock | Locks tracing-subscriber as a dependency of pluto-cli. |
Comments suppressed due to low confidence (1)
crates/cli/src/commands/test/mod.rs:756
request_rttreturnsOk(rtt)even whenstatus != expected_status(it only logs a warning). The beacon tests treatOk(_)as success (e.g.,Pingsets verdictOk), so endpoints returning the wrong status can still be marked as passing. Consider returning an error (or a richer result containing both RTT and status) when the status code doesn't matchexpected_status, and update callers accordingly.
let status = response.status();
if status != expected_status {
match response.text().await {
Ok(body) if !body.is_empty() => tracing::warn!(
status_code = status.as_u16(),
expected_status_code = expected_status.as_u16(),
endpoint = clean_url,
body = body,
"Unexpected status code"
),
_ => tracing::warn!(
status_code = status.as_u16(),
expected_status_code = expected_status.as_u16(),
endpoint = clean_url,
"Unexpected status code"
),
}
}
Ok(rtt)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for the PR! Can you fix the linter please? |
|
sure, I'm resolving rest copilot comments |
added NonZero for SLOT_TIME_SECS and SLOTS_IN_EPOCH
|
Addressed all @copilot comments |
|
@mskrzypkows I've opened a new pull request, #296, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Also added a docker compose to easier run beacon test, e.g.: |
|
Claude finished @varex83's task in 3m 15s —— View job Code Review — feat(cli): beacon CLI tests
Overall this is a solid port of the Charon beacon test suite. The structure mirrors Go well, test coverage is good (mock-server integration tests for all main paths), and the Findings[High]
|
| Component | Go | Rust | Match | Notes |
|---|---|---|---|---|
CLI flags --endpoints, --load-test, --simulation-* |
✅ | ✅ | yes | |
| Test case list & order | ✅ | ✅ | yes | |
SimulateCustom result name |
SimulateCustom |
Simulate{N} |
no | Bug — see finding above |
| Proposal slot advancement | / SLOT_TIME + 1 |
/ SLOT_TIME + 1 |
yes | Both have the +1; confirm this is intentional vs. Go |
| Basic auth credential stripping | ✅ | ✅ | yes | set_username/set_password errors are now properly propagated |
| JSON output file atomic write | ✅ | ✅ | yes | tempfile + persist pattern |
| Tracing init | conditional | unconditional .init() |
partial | Go does not panic; Rust can |
Tests
Unit tests in beacon.rs cover: healthy node (all test cases), connection refused, timeout, quiet mode, file output, custom test case selection, and basic auth. Good coverage of the happy and error paths. The Simulate* load tests are skipped in unit tests (correct, since load_test = false).
No unit test exercises SimulateCustom naming or the slot-advancement math — adding those would catch the two High findings above.
CLI beacon tests ported from Charon, fixes #233