diff --git a/go-runner/src/cli.rs b/go-runner/src/cli.rs index c10a589..2712704 100644 --- a/go-runner/src/cli.rs +++ b/go-runner/src/cli.rs @@ -1,3 +1,5 @@ +use std::iter::Peekable; + #[derive(Debug)] pub enum CliExit { Help, @@ -6,6 +8,159 @@ pub enum CliExit { UnknownFlag, } +/// A `go test` flag, classified by whether it takes a value. +/// +/// Flag names follow cmd/go/internal/{base/flag.go, work/build.go, test/testflag.go}. +enum Flag<'a> { + /// Takes no value. + Boolean, + /// Takes one value, attached (`-p=1`) or as the next argument (`-p 1`). + Value(Option<&'a str>), + /// Not a `go test` flag, e.g. a custom flag of the test binary. + Unknown(Option<&'a str>), +} + +impl<'a> Flag<'a> { + /// Classifies `arg`, or returns `None` if it is not a flag. + fn from_arg(arg: &'a str) -> Option { + if Self::parse_bool_flag(arg).is_some() { + return Some(Self::Boolean); + } + if let Some(value) = Self::parse_value_flag(arg) { + return Some(Self::Value(value)); + } + let (_, value) = Self::split(arg)?; + Some(Self::Unknown(value)) + } + + /// Parses a `go test` flag that takes a value and returns its attached value (`-p=1`), or + /// `Some(None)` when the value is the next argument (`-p 1`). Returns `None` if `arg` is + /// not a known value flag. Like [`Self::parse_bool_flag`], it accepts `--` and `test.`. + fn parse_value_flag(arg: &str) -> Option> { + const VALUE_FLAGS: &[&str] = &[ + "asmflags", + "bench", + "benchtime", + "blockprofile", + "blockprofilerate", + "buildmode", + "C", + "compiler", + "count", + "covermode", + "coverpkg", + "coverprofile", + "cpu", + "cpuprofile", + "debug-actiongraph", + "debug-runtime-trace", + "debug-trace", + "exec", + "fuzz", + "fuzzminimizetime", + "fuzztime", + "gccgoflags", + "gcflags", + "installsuffix", + "ldflags", + "list", + "memprofile", + "memprofilerate", + "mod", + "modfile", + "mutexprofile", + "mutexprofilefraction", + "o", + "outputdir", + "overlay", + "p", + "parallel", + "pgo", + "pkgdir", + "run", + "shuffle", + "skip", + "tags", + "timeout", + "toolexec", + "trace", + "vet", + ]; + let (name, value) = Self::split(arg)?; + VALUE_FLAGS.contains(&name).then_some(value) + } + + /// Parses a boolean `go test` flag and returns its value, or `None` if `arg` is not a + /// known boolean flag or has an invalid value. Accepted spellings: + /// + /// - `-v`, `--v`: bare, means true + /// - `-v=true`, `--v=true`: also `1`, `t`, `T`, `TRUE`, `True` + /// - `-v=false`, `--v=false`: also `0`, `f`, `F`, `FALSE`, `False` + /// - `-test.v`, `--test.v`, `-test.v=true`: the same flag under the test binary's name + /// + /// `-v true` is not a form: Go reads `true` as a separate argument, not as the value. + fn parse_bool_flag(arg: &str) -> Option { + const BOOL_FLAGS: &[&str] = &[ + "a", + "artifacts", + "asan", + "benchmem", + "buildvcs", + "c", + "cover", + "failfast", + "fullpath", + "json", + "linkshared", + "modcacherw", + "msan", + "n", + "race", + "short", + "trimpath", + "v", + "work", + "x", + ]; + // Spellings accepted by Go's `strconv.ParseBool`. + const TRUE_VALUES: &[&str] = &["1", "t", "T", "TRUE", "true", "True"]; + const FALSE_VALUES: &[&str] = &["0", "f", "F", "FALSE", "false", "False"]; + + let (name, value) = Self::split(arg)?; + if !BOOL_FLAGS.contains(&name) { + return None; + } + match value { + None => Some(true), + Some(value) if TRUE_VALUES.contains(&value) => Some(true), + Some(value) if FALSE_VALUES.contains(&value) => Some(false), + Some(_) => None, + } + } + + /// Consumes the flag's value from `args` when it is a separate argument. + fn consume_value(self, args: &mut Peekable>) -> Option { + match self { + Self::Boolean | Self::Value(Some(_)) | Self::Unknown(Some(_)) => None, + Self::Value(None) => args.next(), + // Like Go, treat a dash-leading argument after an unknown flag as a flag. + Self::Unknown(None) => args.next_if(|arg| !arg.starts_with('-')), + } + } + + /// Splits `-name`, `--name` or `-test.name`, with an optional `=value`, into name and value. + fn split(arg: &str) -> Option<(&str, Option<&str>)> { + let name = arg.strip_prefix('-')?; + let name = name.strip_prefix('-').unwrap_or(name); + let (name, value) = match name.split_once('=') { + Some((name, value)) => (name, Some(value)), + None => (name, None), + }; + let name = name.strip_prefix("test.").unwrap_or(name); + Some((name, value)) + } +} + #[derive(Debug)] pub struct Cli { /// Run only benchmarks matching regexp @@ -48,7 +203,8 @@ impl Cli { /// /// We can't do this with clap/structopt, because they only support single dashes for single-letter /// flags (e.g., `-h`), and double dashes for multi-letter flags (e.g., `--help`). - fn parse_args(mut args: impl Iterator) -> Result { + fn parse_args(args: impl Iterator) -> Result { + let mut args = args.peekable(); let mut instance = Self::default(); // We currently only support the `test` subcommand. @@ -106,9 +262,11 @@ UNSUPPORTED FLAGS (will be warned about): instance.benchtime = s.split_once('=').unwrap().1.to_string(); } s if s.starts_with('-') => { - eprintln!( - "warning: flag '{s}' is not supported by CodSpeed Go runner, ignoring" - ); + log::warn!("flag '{s}' is not supported by CodSpeed Go runner, ignoring"); + let value = Flag::from_arg(s).and_then(|flag| flag.consume_value(&mut args)); + if let Some(value) = value { + log::warn!("ignoring value '{value}' of unsupported flag '{s}'"); + } } _ => { // Collect package arguments for filtering @@ -128,6 +286,7 @@ UNSUPPORTED FLAGS (will be warned about): #[cfg(test)] mod tests { use super::*; + use rstest::rstest; fn str_to_iter(cmd: &str) -> Result { let args: Vec = if cmd.trim().is_empty() { @@ -215,4 +374,53 @@ mod tests { let result = str_to_iter("go-runner test -unknown"); assert!(result.is_ok()); } + + #[rstest] + #[case::separate_p("-p 1 ./... -bench Foo", &["./...", "-bench", "Foo"])] + #[case::attached_p("-p=1 ./...", &["./..."])] + #[case::path_value("-cpuprofile ./cpu.out ./...", &["./..."])] + #[case::subtest_value("-run TestX/sub ./...", &["./..."])] + #[case::bool_then_package("-v fmt", &["fmt"])] + #[case::test_prefixed_bool("-test.v fmt", &["fmt"])] + #[case::bool_then_flag("-v -bench Foo", &["./..."])] + #[case::trailing_flag("-p", &["./..."])] + #[case::unknown_then_flag("-custom -timeout 5m ./pkg", &["./pkg"])] + fn test_cli_drops_unsupported_flags(#[case] args: &str, #[case] packages: &[&str]) { + let cli = str_to_iter(&format!("go-runner test {args}")).unwrap(); + assert_eq!(cli.packages, packages, "{args}"); + } + + #[rstest] + #[case::bare("-v", Some(true))] + #[case::double_dash("--v", Some(true))] + #[case::true_value("-v=true", Some(true))] + #[case::one_value("-v=1", Some(true))] + #[case::upper_true_value("-v=TRUE", Some(true))] + #[case::false_value("--v=false", Some(false))] + #[case::short_false_value("-short=f", Some(false))] + #[case::test_prefix("-test.v", Some(true))] + #[case::test_prefix_double_dash("--test.v", Some(true))] + #[case::test_prefix_value("-test.short=false", Some(false))] + #[case::invalid_value("-v=maybe", None)] + #[case::empty_value("-v=", None)] + #[case::triple_dash("---v", None)] + #[case::value_flag("-p", None)] + #[case::value_flag_with_value("-p=1", None)] + #[case::unknown("-integration", None)] + #[case::not_a_flag("v", None)] + fn test_parse_bool_flag(#[case] arg: &str, #[case] expected: Option) { + assert_eq!(Flag::parse_bool_flag(arg), expected, "{arg}"); + } + + // Whole-token arguments: `str_to_iter` would split "-s -w" in two. + #[rstest] + #[case::ldflags("-ldflags", "-s -w")] + #[case::double_dash_gcflags("--gcflags", "-N")] + #[case::run("-run", "-x")] + #[case::test_prefixed_run("-test.run", "-bench")] + fn test_cli_drops_dash_leading_value(#[case] flag: &str, #[case] value: &str) { + let args = ["test", flag, value, "./pkg"].map(String::from); + let cli = Cli::parse_args(args.into_iter()).unwrap(); + assert_eq!(cli.packages, ["./pkg"], "{flag} {value}"); + } }