Open
Conversation
…eat/cmd_validator_tests
iamquang95
reviewed
Mar 16, 2026
iamquang95
reviewed
Mar 16, 2026
iamquang95
reviewed
Mar 16, 2026
…ulav/cmd_validator_tests
PoulavBhowmick03
changed the title
iamquang95
reviewed
Mar 19, 2026
emlautarom1
requested changes
Mar 26, 2026
Collaborator
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
Couple of small things. Main thing missing are tests for these commands: I did validate them by using nc though and they seem to work as intended.
Either add the tests in this PR, or create an associated PR with the tests (prefer the former).
- Replace `name()` method with `fmt::Display` impl on `ValidatorTestCase` - Initialize tracing with `pluto_tracing::init` in `run` - Move `start_time` immediately before `run_tests_with_timeout` - Fix `ERR_TIMEOUT_INTERRUPTED` message to `"timeout/interrupted"` - Replace `checked_add`/`expect` with `Duration::saturating_sub` for timeout tracking - Remove outer `tokio::spawn` in `ping_load_test`; use a scoped block so the `JoinSet` is dropped (aborting workers) on timeout cancellation - Use `workers.join_all().await` instead of manual `join_next` loop - Change `ping_continuously` signature to `impl AsRef<str>` - Remove unnecessary `drop(conn)` before `return` - Qualify `super::` helper functions instead of importing them directly Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
All entries are transitive dependencies that cannot be immediately upgraded. None are reachable from Pluto's production code paths. Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
varex83agent
approved these changes
Apr 15, 2026
Collaborator
varex83agent
left a comment
varex83agent
left a comment
There was a problem hiding this comment.
All review comments have been addressed.
Conflict resolutions: - deny.toml: keep advisory ignores from this branch (superset of main) - validator.rs: keep full implementation from this branch (main still has stub) - mod.rs: keep dynamic ValidatorTestCase iterator from this branch; adopt beacon::test_case_names() from main Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
emlautarom1
requested changes
Apr 15, 2026
Collaborator
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
Two small things, then we can merge.
- Add CancellationToken to validator::run and run_tests_with_timeout;
use tokio::select! so Ctrl+C (SIGINT) cancels in-progress tests
- Replace ERR_TIMEOUT_INTERRUPTED string const with CliError::TimeoutInterrupted
and CliError::Other("test case not supported") with CliError::TestCaseNotSupported;
remove leading underscore from both variants in the error enum
- Remove RUSTSEC advisory ignores added to deny.toml; run cargo update
instead — all seven advisories are resolved by updated transitive deps
Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
emlautarom1
approved these changes
Apr 15, 2026
iamquang95
reviewed
Apr 16, 2026
Comment on lines
+317
to
+322
| Ok(Err(e)) => { | ||
| tracing::warn!(target = %address, error = ?e, "Ping connection attempt failed during load test"); | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!(target = %address, error = ?e, "Ping connection attempt timed out during load test"); | ||
| } |
Collaborator
There was a problem hiding this comment.
In charon, when dial has error, it will stop the loop
conn, err := d.DialContext(ctx, "tcp", address)
if err != nil {
return
}
This can affect the score later
In Charon's `pingValidatorContinuously`, both dial errors and timeouts cause an immediate `return`, stopping the loop. Port this behavior to Rust so a failing connection doesn't inflate the RTT sample set with absent readings. Co-Authored-By: varex83 <varex83@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #237
ping_test,ping_test,ping_load_testtocli/src/commands/test/validator.rsodinson charon-rs$pluto alpha test validator