From 816494ac6807f9b6f35ce421bef3ba31c8360e0c Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 15 Nov 2024 07:56:30 -0800 Subject: [PATCH] refactor: Options holds values by reference Performance of it doesn't matter but this seems to make construction a bit simpler. --- src/cargo.rs | 38 ++++++++------ src/lab.rs | 4 +- src/main.rs | 9 ++-- src/options.rs | 132 ++++++++++++++++++++++++++++-------------------- src/timeouts.rs | 30 +++++++---- 5 files changed, 126 insertions(+), 87 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index 5360017d..ae5fea09 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -161,9 +161,14 @@ fn cargo_argv(packages: &PackageSelection, phase: Phase, options: &Options) -> V .iter() .map(|f| format!("--features={}", f)), ); - cargo_args.extend(options.additional_cargo_args.iter().cloned()); + cargo_args.extend(options.additional_cargo_args.iter().map(|s| s.to_string())); if phase == Phase::Test { - cargo_args.extend(options.additional_cargo_test_args.iter().cloned()); + cargo_args.extend( + options + .additional_cargo_test_args + .iter() + .map(|s| s.to_string()), + ); } cargo_args } @@ -212,6 +217,7 @@ mod test { use pretty_assertions::assert_eq; use rusty_fork::rusty_fork_test; + use crate::config::Config; use crate::Args; use super::*; @@ -238,9 +244,7 @@ mod test { let mut options = Options::default(); let package_name = "cargo-mutants-testdata-something"; // let relative_manifest_path = Utf8PathBuf::from("testdata/something/Cargo.toml"); - options - .additional_cargo_test_args - .extend(["--lib", "--no-fail-fast"].iter().map(|s| s.to_string())); + options.additional_cargo_test_args = vec!["--lib", "--no-fail-fast"]; // TODO: It wolud be a bit better to use `--manifest-path` here, to get // the fix for // but it's temporarily regressed. @@ -292,10 +296,8 @@ mod test { let mut options = Options::default(); options .additional_cargo_test_args - .extend(["--lib", "--no-fail-fast"].iter().map(|s| s.to_string())); - options - .additional_cargo_args - .extend(["--release".to_owned()]); + .extend(["--lib", "--no-fail-fast"]); + options.additional_cargo_args.extend(["--release"]); assert_eq!( cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], ["check", "--tests", "--verbose", "--workspace", "--release"] @@ -320,7 +322,8 @@ mod test { #[test] fn no_default_features_args_passed_to_cargo() { let args = Args::try_parse_from(["mutants", "--no-default-features"].as_slice()).unwrap(); - let options = Options::from_args(&args).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], [ @@ -336,7 +339,8 @@ mod test { #[test] fn all_features_args_passed_to_cargo() { let args = Args::try_parse_from(["mutants", "--all-features"].as_slice()).unwrap(); - let options = Options::from_args(&args).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], [ @@ -352,7 +356,8 @@ mod test { #[test] fn cap_lints_passed_to_cargo() { let args = Args::try_parse_from(["mutants", "--cap-lints=true"].as_slice()).unwrap(); - let options = Options::from_args(&args).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], ["check", "--tests", "--verbose", "--workspace",] @@ -365,7 +370,8 @@ mod test { ["mutants", "--features", "foo", "--features", "bar,baz"].as_slice(), ) .unwrap(); - let options = Options::from_args(&args).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], [ @@ -381,8 +387,9 @@ mod test { #[test] fn profile_arg_passed_to_cargo() { + let config = Config::default(); let args = Args::try_parse_from(["mutants", "--profile", "mutants"].as_slice()).unwrap(); - let options = Options::from_args(&args).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], [ @@ -401,7 +408,8 @@ mod test { ["mutants", "--test-tool=nextest", "--profile", "mutants"].as_slice(), ) .unwrap(); - let options = Options::from_args(&args).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( cargo_argv(&PackageSelection::All, Phase::Build, &options)[1..], [ diff --git a/src/lab.rs b/src/lab.rs index cd487a3e..b10ecf57 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -163,7 +163,7 @@ fn join_threads(threads: Vec>>) -> Resul struct Lab<'a> { output_mutex: Mutex, jobserver: Option, - options: &'a Options, + options: &'a Options<'a>, console: &'a Console, } @@ -218,7 +218,7 @@ struct Worker<'a> { build_dir: &'a BuildDir, output_mutex: &'a Mutex, jobserver: &'a Option, - options: &'a Options, + options: &'a Options<'a>, console: &'a Console, } diff --git a/src/main.rs b/src/main.rs index e89f1b2a..f73c99f4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -440,15 +440,12 @@ fn main() -> Result<()> { PackageFilter::Auto(start_dir.to_owned()) }; - let output_parent_dir = options - .output_in_dir - .clone() - .unwrap_or_else(|| workspace.root().to_owned()); + let output_parent_dir = options.output_in_dir.unwrap_or_else(|| workspace.root()); let mut discovered = workspace.discover(&package_filter, &options, &console)?; let previously_caught = if args.iterate { - let previously_caught = load_previously_caught(&output_parent_dir)?; + let previously_caught = load_previously_caught(output_parent_dir)?; info!( "Iteration excludes {} previously caught or unviable mutants", previously_caught.len() @@ -477,7 +474,7 @@ fn main() -> Result<()> { if args.list { print!("{}", list_mutants(&mutants, &options)); } else { - let output_dir = OutputDir::new(&output_parent_dir)?; + let output_dir = OutputDir::new(output_parent_dir)?; if let Some(previously_caught) = previously_caught { output_dir.write_previously_caught(&previously_caught)?; } diff --git a/src/options.rs b/src/options.rs index 67c1ccd0..e1445014 100644 --- a/src/options.rs +++ b/src/options.rs @@ -21,7 +21,7 @@ use crate::*; /// Options for mutation testing, based on both command-line arguments and the /// config file. #[derive(Default, Debug, Clone)] -pub struct Options { +pub struct Options<'a> { /// Run tests in an unmutated tree? pub baseline: BaselineStrategy, @@ -92,13 +92,13 @@ pub struct Options { pub shuffle: bool, /// Cargo profile. - pub profile: Option, + pub profile: Option<&'a str>, /// Additional arguments for every cargo invocation. - pub additional_cargo_args: Vec, + pub additional_cargo_args: Vec<&'a str>, /// Additional arguments to `cargo test`. - pub additional_cargo_test_args: Vec, + pub additional_cargo_test_args: Vec<&'a str>, /// Selection of features for cargo. pub features: super::Features, @@ -116,13 +116,13 @@ pub struct Options { pub exclude_names: RegexSet, /// Create `mutants.out` within this directory (by default, the source directory). - pub output_in_dir: Option, + pub output_in_dir: Option<&'a Utf8Path>, /// Run this many `cargo build` or `cargo test` tasks in parallel. pub jobs: Option, /// Insert these values as errors from functions returning `Result`. - pub error_values: Vec, + pub error_values: Vec<&'a str>, /// Show ANSI colors. pub colors: Colors, @@ -164,11 +164,6 @@ pub enum TestTool { Nextest, } -/// Join two slices into a new vector. -fn join_slices(a: &[String], b: &[String]) -> Vec { - a.iter().chain(b).cloned().collect() -} - /// Should ANSI colors be drawn? #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Display, Deserialize, ValueEnum)] #[strum(serialize_all = "snake_case")] @@ -208,9 +203,9 @@ impl Colors { } } -impl Options { +impl<'a> Options<'a> { /// Build options by merging command-line args and config file. - pub(crate) fn new(args: &Args, config: &Config) -> Result { + pub(crate) fn new(args: &'a Args, config: &'a Config) -> Result> { if args.no_copy_target { warn!("--no-copy-target is deprecated and has no effect; target/ is never copied"); } @@ -241,11 +236,18 @@ impl Options { }; let options = Options { - additional_cargo_args: join_slices(&args.cargo_arg, &config.additional_cargo_args), - additional_cargo_test_args: join_slices( - &args.cargo_test_args, - &config.additional_cargo_test_args, - ), + additional_cargo_args: args + .cargo_arg + .iter() + .chain(&config.additional_cargo_args) + .map(String::as_str) + .collect(), + additional_cargo_test_args: args + .cargo_test_args + .iter() + .chain(&config.additional_cargo_test_args) + .map(String::as_str) + .collect(), baseline: args.baseline, build_timeout: args.build_timeout.map(Duration::from_secs_f64), build_timeout_multiplier: args @@ -256,7 +258,12 @@ impl Options { colors: args.colors, emit_json: args.json, emit_diffs: args.diff, - error_values: join_slices(&args.error, &config.error_values), + error_values: args + .error + .iter() + .chain(&config.error_values) + .map(String::as_str) + .collect(), examine_names: RegexSet::new(or_slices(&args.examine_re, &config.examine_re)) .context("Failed to compile examine_re regex")?, exclude_names: RegexSet::new(or_slices(&args.exclude_re, &config.exclude_re)) @@ -271,10 +278,14 @@ impl Options { jobserver_tasks: args.jobserver_tasks, leak_dirs: args.leak_dirs, minimum_test_timeout, - output_in_dir: args.output.clone(), + output_in_dir: args.output.as_deref(), print_caught: args.caught, print_unviable: args.unviable, - profile: args.profile.as_ref().or(config.profile.as_ref()).cloned(), + profile: args + .profile + .as_ref() + .or(config.profile.as_ref()) + .map(String::as_str), shuffle: !args.no_shuffle, show_line_col: args.line_col, show_times: !args.no_times, @@ -295,11 +306,6 @@ impl Options { Ok(options) } - #[cfg(test)] - pub fn from_args(args: &Args) -> Result { - Options::new(args, &Config::default()) - } - /// Which phases to run for each mutant. pub fn phases(&self) -> &[Phase] { if self.check_only { @@ -344,7 +350,8 @@ mod test { #[test] fn default_options() { let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert!(!options.check_only); assert_eq!(options.test_tool, TestTool::Cargo); assert!(!options.cap_lints); @@ -353,45 +360,50 @@ mod test { #[test] fn options_from_test_tool_arg() { let args = Args::parse_from(["mutants", "--test-tool", "nextest"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_tool, TestTool::Nextest); } #[test] fn options_from_baseline_arg() { let args = Args::parse_from(["mutants", "--baseline", "skip"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.baseline, BaselineStrategy::Skip); let args = Args::parse_from(["mutants", "--baseline", "run"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.baseline, BaselineStrategy::Run); let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.baseline, BaselineStrategy::Run); } #[test] fn options_from_timeout_args() { let args = Args::parse_from(["mutants", "--timeout=2.0"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_timeout, Some(Duration::from_secs(2))); let args = Args::parse_from(["mutants", "--timeout-multiplier=2.5"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_timeout_multiplier, Some(2.5)); let args = Args::parse_from(["mutants", "--minimum-test-timeout=60.0"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.minimum_test_timeout, Duration::from_secs(60)); let args = Args::parse_from(["mutants", "--build-timeout=3.0"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.build_timeout, Some(Duration::from_secs(3))); let args = Args::parse_from(["mutants", "--build-timeout-multiplier=3.5"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.build_timeout_multiplier, Some(3.5)); } @@ -453,6 +465,7 @@ mod test { #[test] fn features_arg() { let args = Args::try_parse_from(["mutants", "--features", "nice,shiny features"]).unwrap(); + let config = Config::default(); assert_eq!( args.features.features.iter().as_ref(), ["nice,shiny features"] @@ -460,7 +473,7 @@ mod test { assert!(!args.features.no_default_features); assert!(!args.features.all_features); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( options.features.features.iter().as_ref(), ["nice,shiny features"] @@ -478,8 +491,9 @@ mod test { "nice,shiny features", ]) .unwrap(); + let config = Config::default(); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( options.features.features.iter().as_ref(), ["nice,shiny features"] @@ -491,7 +505,8 @@ mod test { #[test] fn default_jobserver_settings() { let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert!(options.jobserver); assert_eq!(options.jobserver_tasks, None); } @@ -499,7 +514,8 @@ mod test { #[test] fn disable_jobserver() { let args = Args::parse_from(["mutants", "--jobserver=false"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert!(!options.jobserver); assert_eq!(options.jobserver_tasks, None); } @@ -507,7 +523,8 @@ mod test { #[test] fn jobserver_tasks() { let args = Args::parse_from(["mutants", "--jobserver-tasks=13"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert!(options.jobserver); assert_eq!(options.jobserver_tasks, Some(13)); } @@ -521,8 +538,9 @@ mod test { "nice,shiny features", ]) .unwrap(); + let config = Config::default(); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( options.features.features.iter().as_ref(), ["nice,shiny features"] @@ -540,22 +558,23 @@ mod test { remove_var("CLICOLOR_FORCE"); remove_var("NO_COLOR"); let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.colors.forced_value(), Some(true)); set_var("CARGO_TERM_COLOR", "never"); let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.colors.forced_value(), Some(false)); set_var("CARGO_TERM_COLOR", "auto"); let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.colors.forced_value(), None); remove_var("CARGO_TERM_COLOR"); let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.colors.forced_value(), None); } @@ -567,22 +586,23 @@ mod test { remove_var("CLICOLOR_FORCE"); remove_var("NO_COLOR"); let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.colors.forced_value(), None); remove_var("CLICOLOR_FORCE"); set_var("NO_COLOR", "1"); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.colors.forced_value(), Some(false)); remove_var("NO_COLOR"); set_var("CLICOLOR_FORCE", "1"); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.colors.forced_value(), Some(true)); remove_var("CLICOLOR_FORCE"); remove_var("NO_COLOR"); - let options = Options::new(&args, &Config::default()).unwrap(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.colors.forced_value(), None); } } @@ -590,7 +610,8 @@ mod test { #[test] fn profile_option_from_args() { let args = Args::parse_from(["mutants", "--profile=mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.profile.unwrap(), "mutants"); } @@ -612,14 +633,16 @@ mod test { #[test] fn test_workspace_arg_true() { let args = Args::parse_from(["mutants", "--test-workspace=true"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_package, TestPackages::Workspace); } #[test] fn test_workspace_arg_false() { let args = Args::parse_from(["mutants", "--test-workspace=false"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_package, TestPackages::Mutated); } @@ -686,7 +709,8 @@ mod test { #[test] fn test_package_arg_with_commas() { let args = Args::parse_from(["mutants", "--test-package=foo,bar"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( options.test_package, TestPackages::Named(vec!["foo".to_string(), "bar".to_string()]) diff --git a/src/timeouts.rs b/src/timeouts.rs index 2f4fa743..5b668eb4 100644 --- a/src/timeouts.rs +++ b/src/timeouts.rs @@ -109,7 +109,8 @@ mod test { #[test] fn timeout_multiplier_from_option() { let args = Args::parse_from(["mutants", "--timeout-multiplier", "1.5"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_timeout_multiplier, Some(1.5)); assert_eq!( @@ -121,7 +122,8 @@ mod test { #[test] fn test_timeout_unaffected_by_in_place_build() { let args = Args::parse_from(["mutants", "--timeout-multiplier", "1.5", "--in-place"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( test_timeout(Some(Duration::from_secs(40)), &options), @@ -132,7 +134,8 @@ mod test { #[test] fn build_timeout_multiplier_from_option() { let args = Args::parse_from(["mutants", "--build-timeout-multiplier", "1.5"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.build_timeout_multiplier, Some(1.5)); assert_eq!( @@ -144,7 +147,8 @@ mod test { #[test] fn build_timeout_is_affected_by_in_place_build() { let args = Args::parse_from(["mutants", "--build-timeout-multiplier", "5", "--in-place"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!( build_timeout(Some(Duration::from_secs(40)), &options), @@ -187,7 +191,8 @@ mod test { #[test] fn timeout_multiplier_default() { let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_timeout_multiplier, None); assert_eq!( @@ -199,7 +204,8 @@ mod test { #[test] fn build_timeout_multiplier_default() { let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.build_timeout_multiplier, None); assert_eq!(build_timeout(Some(Duration::from_secs(42)), &options), None,); @@ -208,7 +214,8 @@ mod test { #[test] fn timeout_from_option() { let args = Args::parse_from(["mutants", "--timeout=8"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_timeout, Some(Duration::from_secs(8))); } @@ -216,7 +223,8 @@ mod test { #[test] fn build_timeout_from_option() { let args = Args::parse_from(["mutants", "--build-timeout=4"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.build_timeout, Some(Duration::from_secs(4))); } @@ -224,7 +232,8 @@ mod test { #[test] fn no_default_build_timeout() { let args = Args::parse_from(["mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.build_timeout, None); } @@ -234,7 +243,8 @@ mod test { // The --baseline option is not used to set the timeout but it's // indicative of the realistic situation. let args = Args::parse_from(["mutants", "--baseline", "skip"]); - let options = Options::new(&args, &Config::default()).unwrap(); + let config = Config::default(); + let options = Options::new(&args, &config).unwrap(); assert_eq!(options.test_timeout_multiplier, None); assert_eq!(test_timeout(None, &options), Some(Duration::from_secs(300)));