chore: remove backwards-compat test cruft#1702
Conversation
Remove three tests that only existed to document rejection of old/removed interfaces. Too early in the project for this kind of compat baggage: - clap_rejects_unknown_old_top_level_flags (old --config top-level flag) - clap_rejects_removed_dump_config_alias (removed dump-config subcommand) - legacy_output_aliases_are_rejected (removed file_out/tcp_out/udp_out) Closes #1599 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
WalkthroughThis PR removes test coverage without modifying production code. It eliminates the Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Pull request overview
Removes backwards-compatibility tests that asserted rejection of CLI/config interfaces the project no longer wants to carry forward (and that reportedly never shipped publicly).
Changes:
- Deleted CLI tests that documented/validated rejection of removed/legacy flags and subcommand aliases.
- Deleted config parsing test that documented rejection of legacy output type aliases (
file_out,tcp_out,udp_out).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/logfwd/src/main.rs |
Removes CLI parser tests for old top-level flags / removed subcommand alias rejection. |
crates/logfwd-config/src/lib.rs |
Removes test covering rejection of legacy output type aliases. |
| #[test] | ||
| fn clap_parses_validate_subcommand() { | ||
| let cli = Cli::try_parse_from(["logfwd", "validate", "--config", "foo.yaml"]) | ||
| .expect("parser should accept validate subcommand"); | ||
| match cli.command.expect("command") { | ||
| Commands::Validate { config } => assert_eq!(config.as_deref(), Some("foo.yaml")), | ||
| other => panic!("expected validate command, got {other:?}"), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn clap_rejects_unknown_old_top_level_flags() { | ||
| let err = Cli::try_parse_from(["logfwd", "--config", "foo.yaml"]) | ||
| .expect_err("parser should reject old top-level flags"); | ||
| assert_eq!(err.kind(), ErrorKind::UnknownArgument); | ||
| } | ||
|
|
||
| #[test] | ||
| fn clap_parses_effective_config_with_optional_config_flag() { | ||
| let with_path = Cli::try_parse_from(["logfwd", "effective-config", "--config", "foo.yaml"]) |
There was a problem hiding this comment.
The PR description closes #1599 (misleading logfwd --validate error), but the code changes here only remove backwards-compat CLI tests and don’t appear to address that behavior. If #1599 is already fixed elsewhere, consider removing the closing keyword here (or update the PR/issue link to the relevant issue).
ApprovabilityVerdict: Approved This PR only removes three test functions that verified rejection of legacy CLI options. No production code is changed, so there is no runtime impact. The unresolved comment concerns PR/issue linkage documentation, not code correctness. You can customize Macroscope's approvability policy. Learn more. |
Summary
clap_rejects_unknown_old_top_level_flagstest (documented old--configtop-level flag)clap_rejects_removed_dump_config_aliastest (documented removeddump-configsubcommand)legacy_output_aliases_are_rejectedtest (documented removedfile_out/tcp_out/udp_outoutput types)Too early in the project for backwards-compat baggage. These tests only documented rejection of interfaces that never shipped publicly.
Closes #1599
Test plan
cargo test -p logfwd --bin logfwd -- cli_tests— 22 passedcargo test -p logfwd-config --lib— 123 passed🤖 Generated with Claude Code
Note
Remove backwards-compatibility tests for legacy CLI flags and config aliases
Deletes three tests that verified rejection of legacy/removed inputs: the
legacy_output_aliases_are_rejectedtest in lib.rs and two CLI tests (clap_rejects_unknown_old_top_level_flags,clap_rejects_removed_dump_config_alias) in main.rs. These tests were backwards-compatibility cruft no longer needed.Macroscope summarized f03d2a2.