Repository navigation
fix(cli): reject unsupported go test flags - #67
not-matthias wants to merge 1 commit into
Conversation
Merging this PR will not alter performance2 warnings
Comparing Footnotes
|
The parser warned about unsupported flags but dropped only the flag, so a separate value was collected as a package. In `go test -p 1 ./... -bench .`, `1` became a package and the run failed with `package 1 is not in std` after the benchmarks completed. Go flag arity is not known to the runner, so after an unsupported flag the next token is dropped too unless it looks like a package pattern (relative or absolute path, `...` wildcard, reserved word, or domain-style import path). Inline `-flag=value` forms still drop a single token. The run keeps going, as before, and never fails before Go is invoked. Closes COD-4005
52a0d05 to
0458739
Compare
[Medium impact] The change affects command-line parsing across Go runner benchmark runs. Fix unsupported flag rejection and use tracing for the added warning before merging.
|
| if s.contains('=') { | ||
| continue; | ||
| } | ||
| // The flag's arity is unknown. A bare token that does not look like a | ||
| // package is the flag's value, and must not be collected as a package. | ||
| let is_flag_value = |a: &String| !a.starts_with('-') && !looks_like_package(a); | ||
| if let Some(value) = args.next_if(is_flag_value) { |
There was a problem hiding this comment.
Flags change package selection
With test -v fmt, the new code consumes fmt as a flag value and runs the default ./... instead of the requested package. Conversely, test -cpuprofile ./cpu.out ./... keeps ./cpu.out as a package, leaving the original failure unfixed.
Return CliExit::UnknownFlag before Go runs, as the PR description requires, rather than guessing which arguments belong to unsupported flags. Update the tests to expect rejection.
Prompt To Fix With AI
This is a comment left during a code review.
Path: go-runner/src/cli.rs
Line: 133-139
Comment:
**Flags change package selection**
With `test -v fmt`, the new code consumes `fmt` as a flag value and runs the default `./...` instead of the requested package. Conversely, `test -cpuprofile ./cpu.out ./...` keeps `./cpu.out` as a package, leaving the original failure unfixed.
Return `CliExit::UnknownFlag` before Go runs, as the PR description requires, rather than guessing which arguments belong to unsupported flags. Update the tests to expect rejection.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| // package is the flag's value, and must not be collected as a package. | ||
| let is_flag_value = |a: &String| !a.starts_with('-') && !looks_like_package(a); | ||
| if let Some(value) = args.next_if(is_flag_value) { | ||
| eprintln!("warning: ignoring value '{value}' of unsupported flag '{s}'"); |
There was a problem hiding this comment.
The new warning uses eprintln!, but the repository rule prohibits println! and eprintln! in Rust and requires tracing macros instead. Use tracing::warn! here to satisfy that requirement before merging.
| eprintln!("warning: ignoring value '{value}' of unsupported flag '{s}'"); | |
| tracing::warn!("ignoring value '{value}' of unsupported flag '{s}'"); |
Rule Used: Never use println!/eprintln! in Rust; use tracing macros (info!/debug!/warn!/error!). (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: go-runner/src/cli.rs
Line: 140
Comment:
**Warning bypasses tracing**
The new warning uses `eprintln!`, but the repository rule prohibits `println!` and `eprintln!` in Rust and requires tracing macros instead. Use `tracing::warn!` here to satisfy that requirement before merging.
```suggestion
tracing::warn!("ignoring value '{value}' of unsupported flag '{s}'");
```
**Rule Used:** Never use `println!`/`eprintln!` in Rust; use tracing macros (`info!`/`debug!`/`warn!`/`error!`). ([source](https://app.greptile.com/codspeed/-/custom-context?memory=f04f734b-e556-482b-98b9-64c7ce663663))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reject unsupported
go testflags instead of skipping only the flag.The parser warned and continued for unknown flags, but their values were left behind and parsed as packages. With
go test -p 1 ./... -bench ., the leftover1reached Go as a package, so the run failed withpackage 1 is not in stdonly after every benchmark had passed.This is a user-visible behavior change: every flag the runner does not support now aborts with exit 1 before Go runs, instead of being warned about and ignored. This includes common flags such as
-count,-benchmem, and-v, not just-p. The help text also no longer advertises--dry-run, which the runner never implemented.The arity of an arbitrary unsupported flag is unknown, so the runner cannot safely remove a separate value. It now fails before running Go with an error naming the flag. Flag parsing no longer stops at the first package, so unsupported flags are caught wherever they appear; supported flags after package arguments remain valid.
Regression tests cover separate and
=-attached values, flags before and after packages, and supported flags after packages.cargo test --lib -- --skip integration_tests,cargo check --all-targets, andcargo clippy --all-targets -- -D warningspass. The integration cases need the testdata submodules initialized; locally they fail because those submodules are absent.Closes COD-4005