charles/ws-rpc-test
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(cli): clap-based CLI + parse_cli_args() helper (#15) #34

Merged
charles merged 2 commits from feat/15-cli into main 2026-04-11 18:44:10 +00:00
Conversation 1 Commits 2 Files changed 3 +219
charles commented 2026-04-11 13:07:05 +00:00
Owner
Copy link

Closes #15. Stacked on #33.

Summary

Adds the cli module behind the default-on cli feature declared in #1. Gives test binaries a one-liner (runner.parse_cli_args()) that wires every spec §7 flag into RunConfig.

Flags (all from spec §7)

  • --binary <PATH>
  • --host <HOST> (default 127.0.0.1)
  • --port <PORT> (default 9820)
  • --health-url <URL>
  • --ws-url <URL>
  • --token <TOKEN>
  • --filter <PATTERN>
  • --tag <TAG>
  • --fail-fast
  • --timeout <SECS> (default 60)
  • --format <FORMAT> (default console; json and junit accepted)
  • --no-color
  • --startup-timeout <SECS> (default 15)
  • -v / --verbose
  • --parallel
  • --parallel-workers <N>

API

let mut runner = TestRunner::new(harness);
runner.parse_cli_args(); // reads argv into RunConfig
runner.test("health", |c| async move { /* ... */ Ok(()) });
runner.run().await?;

Apply rules (Cli::apply_to(&mut RunConfig))

  • filter / tag only override when Some (builder-configured values win if the user didn't pass flags)
  • fail_fast / no_color / parallel only override when true
  • timeout is always applied (clap default is 60 s, matches RunConfig::with_defaults)
  • format goes through OutputFormat::parse — invalid values leave the config's format untouched (graceful degrade rather than clap-level error)
  • parallel_workers only overrides when Some

Checklist (from issue #15)

  • Cli struct with clap derive
  • Every spec §7 flag represented
  • runner.parse_cli_args() one-liner applies to RunConfig
  • --filter / --tag populate filter / tag
  • Invalid --format is handled cleanly (no panic)
  • Each flag has a descriptive --help line (from clap #[arg] docs)
  • clap gated behind the default-on cli feature — consumers can disable via default-features = false
  • Unit tests cover: spec's example invocation, defaults, round-trip into RunConfig, invalid format

Test plan

  • 4 unit tests in src/cli.rs
  • just qa green locally — 102 unit + 5 integration = 107/107
  • CI green on Forgejo

Notes for the reviewer

  • Harness overrides (--binary, --health-url, --ws-url, --token) are parsed into the Cli struct but not applied to the harness by parse_cli_args. Consumers own the ProcessHarness::builder() chain and should set those fields there. The issue's §9 example applies them manually via ProcessHarness::builder().command(cli.binary.unwrap_or("target/debug/my-app")).... We can add a convenience helper later.
  • RunConfig::timeout is overwritten unconditionally because clap's default (60 s) already matches with_defaults. If a user calls runner.timeout(Duration::from_secs(30)) before parse_cli_args(), their builder value will be overwritten when the CLI uses the default 60. I weighed making this Option<u64> to preserve the builder value, but "CLI wins" is the simpler mental model and matches how most tools behave.
  • The cli feature is on by default. Consumers who want to use clap-less builds can opt out via default-features = false in their Cargo.toml.
  • Tests use Cli::try_parse_from(["ws-rpc-test", ...]) so they don't read std::env::args. No process state is leaked.
Closes #15. **Stacked on #33.** ## Summary Adds the `cli` module behind the default-on `cli` feature declared in #1. Gives test binaries a one-liner (`runner.parse_cli_args()`) that wires every spec §7 flag into `RunConfig`. ### Flags (all from spec §7) - `--binary <PATH>` - `--host <HOST>` (default `127.0.0.1`) - `--port <PORT>` (default `9820`) - `--health-url <URL>` - `--ws-url <URL>` - `--token <TOKEN>` - `--filter <PATTERN>` - `--tag <TAG>` - `--fail-fast` - `--timeout <SECS>` (default 60) - `--format <FORMAT>` (default `console`; `json` and `junit` accepted) - `--no-color` - `--startup-timeout <SECS>` (default 15) - `-v / --verbose` - `--parallel` - `--parallel-workers <N>` ### API ```rust let mut runner = TestRunner::new(harness); runner.parse_cli_args(); // reads argv into RunConfig runner.test("health", |c| async move { /* ... */ Ok(()) }); runner.run().await?; ``` ### Apply rules (`Cli::apply_to(&mut RunConfig)`) - `filter` / `tag` only override when `Some` (builder-configured values win if the user didn't pass flags) - `fail_fast` / `no_color` / `parallel` only override when `true` - `timeout` is always applied (clap default is 60 s, matches `RunConfig::with_defaults`) - `format` goes through `OutputFormat::parse` — invalid values leave the config's format untouched (graceful degrade rather than clap-level error) - `parallel_workers` only overrides when `Some` ## Checklist (from issue #15) - [x] `Cli` struct with clap derive - [x] Every spec §7 flag represented - [x] `runner.parse_cli_args()` one-liner applies to `RunConfig` - [x] `--filter` / `--tag` populate `filter` / `tag` - [x] Invalid `--format` is handled cleanly (no panic) - [x] Each flag has a descriptive `--help` line (from clap `#[arg]` docs) - [x] `clap` gated behind the default-on `cli` feature — consumers can disable via `default-features = false` - [x] Unit tests cover: spec's example invocation, defaults, round-trip into `RunConfig`, invalid format ## Test plan - [x] 4 unit tests in `src/cli.rs` - [x] `just qa` green locally — 102 unit + 5 integration = 107/107 - [ ] CI green on Forgejo ## Notes for the reviewer - Harness overrides (`--binary`, `--health-url`, `--ws-url`, `--token`) are parsed into the `Cli` struct but **not** applied to the harness by `parse_cli_args`. Consumers own the `ProcessHarness::builder()` chain and should set those fields there. The issue's §9 example applies them manually via `ProcessHarness::builder().command(cli.binary.unwrap_or("target/debug/my-app"))...`. We can add a convenience helper later. - `RunConfig::timeout` is overwritten unconditionally because clap's default (60 s) already matches `with_defaults`. If a user calls `runner.timeout(Duration::from_secs(30))` before `parse_cli_args()`, their builder value will be overwritten when the CLI uses the default 60. I weighed making this `Option<u64>` to preserve the builder value, but "CLI wins" is the simpler mental model and matches how most tools behave. - The `cli` feature is on by default. Consumers who want to use `clap`-less builds can opt out via `default-features = false` in their `Cargo.toml`. - Tests use `Cli::try_parse_from(["ws-rpc-test", ...])` so they don't read `std::env::args`. No process state is leaked. 
Implements ticket #15 behind the default-on `cli` feature flag that
#1 declared.

src/cli.rs (new, gated on feature = "cli")
- `Cli` struct with clap derive covering every flag from spec §7:
    --binary, --host, --port, --health-url, --ws-url, --token,
    --filter, --tag, --fail-fast, --timeout, --format, --no-color,
    --startup-timeout, -v/--verbose, --parallel, --parallel-workers
- `Cli::parse_args()` wraps `<Cli as Parser>::parse()`.
- `Cli::apply_to(&self, cfg: &mut RunConfig)` maps the parsed struct
  into the runner's config:
    * filter / tag only override when Some
    * fail_fast / no_color / parallel only override when true
    * timeout is always applied (default 60 s matches the runner's own)
    * format goes through OutputFormat::parse — invalid values leave
      the config untouched (graceful degrade rather than clap error)
    * parallel_workers only overrides when Some

src/runner.rs
- New `TestRunner::parse_cli_args()` (#[cfg(feature = "cli")]) parses
  argv and applies the result to self.config in one call — the
  shortcut documented in spec §9.

src/lib.rs
- `pub mod cli;` declared behind `#[cfg(feature = "cli")]`.

Tests (4 new in src/cli.rs)
- cli_parses_example_invocation_from_spec: --binary, --filter,
  --tag, --fail-fast, --timeout, --format=junit, --no-color,
  --parallel, -v all come through correctly.
- cli_defaults_match_spec: spec §7 defaults verified
  (host=127.0.0.1, port=9820, timeout=60, format=console,
  startup_timeout=15, everything else false).
- cli_applies_filter_and_timeout_to_run_config: round-trips into
  RunConfig via apply_to and asserts each field.
- cli_invalid_format_leaves_config_unchanged: --format=xml-v2
  parses but OutputFormat::parse returns None, so the config keeps
  OutputFormat::Console.

just qa green: 102 unit + 5 integration = 107/107, fmt + clippy clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
charles reviewed 2026-04-11 17:58:52 +00:00
charles left a comment
Copy link

Review PR #34 — CLI clap

Feature propre et bien délimitée. L'intégration derrière le feature flag cli est correcte, les tests couvrent bien les cas prévus. Un point mérite attention.

🐛 Bug de priorité : timeout écrase toujours la valeur du runner

pub fn apply_to(&self, config: &mut RunConfig) {
    ...
    config.timeout = Duration::from_secs(self.timeout);  // toujours exécuté
    ...
}

--timeout a default_value_t = 60 dans clap. Cela signifie que self.timeout vaut toujours 60 — que l'utilisateur ait passé --timeout ou non. En conséquence, apply_to() écrase systématiquement le timeout configuré dans le runner, même si l'appelant avait positionné config.timeout = Duration::from_secs(300) avant d'appeler parse_cli_args().

Le commentaire de la méthode dit : "Only fields set by the user (Some / non-default) override the runner's defaults", mais ce n'est pas vrai pour timeout : la valeur par défaut de clap écrase la valeur programmatique.

Fix minimal — utiliser un Option<u64> dans le struct :

#[arg(long)]
pub timeout: Option<u64>,
// ...
if let Some(t) = self.timeout {
    config.timeout = Duration::from_secs(t);
}

Même traitement conseillé pour startup_timeout.

Champs exposés mais non appliqués par apply_to()

binary, host, port, health_url, ws_url, token, startup_timeout, verbose sont parsés mais pas appliqués. Le doc-comment de parse_cli_args() l'explique (les champs harness appartiennent au HarnessBuilder), c'est un choix de design valide. Mais verbose et startup_timeout ne correspondent pas à des champs harness — s'ils ne font rien pour l'instant, un commentaire // TODO: wire into RunConfig once the field exists éviterait la confusion.

Format invalide silencieux

if let Some(format) = OutputFormat::parse(&self.format) {
    config.format = format;
}

--format xml-v2 est accepté par clap mais ignoré silencieusement. La valeur reste Console. Le test cli_invalid_format_leaves_config_unchanged valide ce comportement. Une mise en garde sur eprintln! améliorerait l'UX, mais ce n'est pas un bug.

Points positifs

  • Feature gate #[cfg(feature = "cli")] correctement propagée dans lib.rs, runner.rs, et les exemples.
  • apply_to() correctement one-directional pour les booléens (on peut activer fail_fast via CLI, pas le désactiver).
  • Tests couvrent : invocation spec §9, defaults, filter+timeout+format, format invalide. Bonne couverture.
  • parse_cli_args() retourne &mut Self → chaînable. ✓

⚠️ Le bug timeout (écrasement systématique par la valeur par défaut clap 60 s) peut causer des surprises si le runner est configuré avec un timeout plus long. À corriger avant merge.

## Review PR #34 — CLI clap Feature propre et bien délimitée. L'intégration derrière le feature flag `cli` est correcte, les tests couvrent bien les cas prévus. Un point mérite attention. --- ### 🐛 Bug de priorité : `timeout` écrase toujours la valeur du runner ```rust pub fn apply_to(&self, config: &mut RunConfig) { ... config.timeout = Duration::from_secs(self.timeout); // toujours exécuté ... } ``` `--timeout` a `default_value_t = 60` dans clap. Cela signifie que `self.timeout` vaut **toujours** 60 — que l'utilisateur ait passé `--timeout` ou non. En conséquence, `apply_to()` écrase systématiquement le timeout configuré dans le runner, même si l'appelant avait positionné `config.timeout = Duration::from_secs(300)` avant d'appeler `parse_cli_args()`. Le commentaire de la méthode dit : _"Only fields set by the user (Some / non-default) override the runner's defaults"_, mais ce n'est pas vrai pour `timeout` : la valeur par défaut de clap écrase la valeur programmatique. Fix minimal — utiliser un `Option<u64>` dans le struct : ```rust #[arg(long)] pub timeout: Option<u64>, // ... if let Some(t) = self.timeout { config.timeout = Duration::from_secs(t); } ``` Même traitement conseillé pour `startup_timeout`. --- ### Champs exposés mais non appliqués par `apply_to()` `binary`, `host`, `port`, `health_url`, `ws_url`, `token`, `startup_timeout`, `verbose` sont parsés mais pas appliqués. Le doc-comment de `parse_cli_args()` l'explique (les champs harness appartiennent au `HarnessBuilder`), c'est un choix de design valide. Mais `verbose` et `startup_timeout` ne correspondent pas à des champs harness — s'ils ne font rien pour l'instant, un commentaire `// TODO: wire into RunConfig once the field exists` éviterait la confusion. --- ### Format invalide silencieux ```rust if let Some(format) = OutputFormat::parse(&self.format) { config.format = format; } ``` `--format xml-v2` est accepté par clap mais ignoré silencieusement. La valeur reste `Console`. Le test `cli_invalid_format_leaves_config_unchanged` valide ce comportement. Une mise en garde sur `eprintln!` améliorerait l'UX, mais ce n'est pas un bug. --- ### Points positifs - Feature gate `#[cfg(feature = "cli")]` correctement propagée dans `lib.rs`, `runner.rs`, et les exemples. - `apply_to()` correctement one-directional pour les booléens (on peut activer `fail_fast` via CLI, pas le désactiver). - Tests couvrent : invocation spec §9, defaults, filter+timeout+format, format invalide. Bonne couverture. - `parse_cli_args()` retourne `&mut Self` → chaînable. ✓ ⚠️ Le bug `timeout` (écrasement systématique par la valeur par défaut clap 60 s) peut causer des surprises si le runner est configuré avec un timeout plus long. À corriger avant merge.
charles changed target branch from feat/14-reporters-json-junit to main 2026-04-11 18:44:03 +00:00
charles deleted branch feat/15-cli 2026-04-11 18:44:10 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
area:assertions
Assertion macros and helpers

  
area:cli
CLI argument parsing

  
area:client
RpcClient (WebSocket, JSON-RPC, events)

  
area:harness
Process harness (spawn, health, shutdown)

  
area:meta
Project setup, examples, self-tests

  
area:reporting
Console, JSON, JUnit reporters

  
area:runner
Test runner (registration, execution, hooks)

  
type:user-story
User story

No labels
area:assertions
area:cli
area:client
area:harness
area:meta
area:reporting
area:runner
type:user-story
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
charles/ws-rpc-test!34
No description provided.