test: add command and option contract gate for e2e#39
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a deterministic “CLI help contract” test suite to guard the rc command tree and option surface (via --help output), and wires it into the integration workflow as a dedicated CI gate.
Changes:
- Added
crates/cli/tests/help_contract.rsto validate help/usage text across top-level commands and nested subcommands. - Added a
cli-contractjob to.github/workflows/integration.ymlto run the new contract tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/cli/tests/help_contract.rs | New e2e-style test harness that runs rc ... --help and asserts expected command/option tokens exist. |
| .github/workflows/integration.yml | Adds a CI job that runs only the CLI help contract test suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for option in GLOBAL_OPTIONS { | ||
| assert!( | ||
| stdout.contains(option), | ||
| "global option `{option}` missing in help for {command_label}\nstdout:\n{}", | ||
| stdout | ||
| ); | ||
| } |
There was a problem hiding this comment.
stdout.contains(option) can produce false positives for some global flags. In particular, --version will match --versions / --version-id, so this loop can pass even if the actual --version flag is missing from help output. Consider matching whole flags (e.g., boundary-aware search/regex, or checking for the exact rendered option line) so the contract gate enforces the intended surface.
| "mirror", | ||
| "tree", | ||
| "share", | ||
| "version", |
There was a problem hiding this comment.
The top-level help contract checks for tokens like "version", but that substring will also be present due to the built-in --version flag, so this assertion can still pass even if the version command is removed/renamed. To make this a strict command-tree contract, match command names in the Commands section more precisely (e.g., line-start/column matching) rather than a raw substring search.
| "version", | |
| " version", |
| if let Ok(path) = std::env::var("CARGO_BIN_EXE_rc") { | ||
| return PathBuf::from(path); | ||
| } | ||
|
|
||
| let workspace_root = PathBuf::from(env!("CARGO_MANIFEST_DIR")) | ||
| .parent() | ||
| .expect("cli crate has parent directory") | ||
| .parent() | ||
| .expect("workspace root exists") | ||
| .to_path_buf(); | ||
|
|
||
| let debug_binary = workspace_root.join("target/debug/rc"); | ||
| if debug_binary.exists() { | ||
| return debug_binary; | ||
| } | ||
|
|
||
| workspace_root.join("target/release/rc") |
There was a problem hiding this comment.
The fallback to target/debug/rc / target/release/rc makes the test potentially non-deterministic when CARGO_BIN_EXE_rc is not set (it could execute a stale binary from a previous build). For a strict contract gate, it’s safer to require CARGO_BIN_EXE_rc (and fail fast if missing) or otherwise ensure the fallback path is freshly built as part of the test setup.
| if let Ok(path) = std::env::var("CARGO_BIN_EXE_rc") { | |
| return PathBuf::from(path); | |
| } | |
| let workspace_root = PathBuf::from(env!("CARGO_MANIFEST_DIR")) | |
| .parent() | |
| .expect("cli crate has parent directory") | |
| .parent() | |
| .expect("workspace root exists") | |
| .to_path_buf(); | |
| let debug_binary = workspace_root.join("target/debug/rc"); | |
| if debug_binary.exists() { | |
| return debug_binary; | |
| } | |
| workspace_root.join("target/release/rc") | |
| match std::env::var("CARGO_BIN_EXE_rc") { | |
| Ok(path) if !path.is_empty() => PathBuf::from(path), | |
| _ => panic!( | |
| "CARGO_BIN_EXE_rc is not set or empty; \ | |
| run these tests via `cargo test` so Cargo provides the rc binary path." | |
| ), | |
| } |
Summary
This PR continues the test hardening work by adding a command/option contract layer and wiring it into integration CI.
What Changed
crates/cli/tests/help_contract.rs--helpoutput for:CLI Contract (Commands and Options)job in.github/workflows/integration.ymlcargo test --package rustfs-cli --test help_contractWhy
Current integration tests focus on runtime behavior with S3/RustFS, but they do not provide a strict contract guard for command tree and option surface. This PR adds a fast, deterministic safety net that catches regressions in command/option UX before runtime integration phases.
Validation
All required checks were run locally and passed:
cargo fmt --all --checkcargo clippy --workspace -- -D warningscargo test --workspacecargo test --package rustfs-cli --test help_contractScope