From 933abc0ee01cb673681d5d75c9203a86543c8e00 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 13 Oct 2024 20:13:11 -0700 Subject: [PATCH 01/37] WIP add test_workspace --- src/config.rs | 2 ++ src/lab.rs | 17 ++++++++++++----- src/main.rs | 6 ++++++ src/options.rs | 7 +++++++ 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/config.rs b/src/config.rs index 205041de..69300bff 100644 --- a/src/config.rs +++ b/src/config.rs @@ -53,6 +53,8 @@ pub struct Config { pub timeout_multiplier: Option, /// Build timeout multiplier, relative to the baseline 'cargo build'. pub build_timeout_multiplier: Option, + /// Run tests from all packages in the workspace, not just the mutated package. + pub test_workspace: Option, } impl Config { diff --git a/src/lab.rs b/src/lab.rs index e7e6c2ab..1b1e50e3 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -69,7 +69,7 @@ pub fn test_mutants( &output_mutex, &jobserver, &Scenario::Baseline, - &mutant_packages, + Some(&mutant_packages), Timeouts::for_baseline(&options), &options, console, @@ -129,15 +129,22 @@ pub fn test_mutants( return Err(anyhow!("Lock pending work queue: {}", err)); } Ok(Some(mutant)) => { + let scenario = Scenario::Mutant(mutant.clone()); let _span = debug_span!("mutant", name = mutant.name(false, false)).entered(); let package = mutant.package().clone(); // hold + let packages = [&package]; + let test_packages: Option<&[&Package]> = if options.test_workspace { + None + } else { + Some(&packages) + }; test_scenario( &build_dir, &output_mutex, &jobserver, - &Scenario::Mutant(mutant), - &[&package], + &scenario, + test_packages, timeouts, &options, console, @@ -203,7 +210,7 @@ fn test_scenario( output_mutex: &Mutex, jobserver: &Option, scenario: &Scenario, - test_packages: &[&Package], + test_packages: Option<&[&Package]>, timeouts: Timeouts, options: &Options, console: &Console, @@ -237,7 +244,7 @@ fn test_scenario( match run_cargo( build_dir, jobserver, - Some(test_packages), + test_packages, phase, timeout, &mut scenario_output, diff --git a/src/main.rs b/src/main.rs index e192fb4a..4711acfc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -309,6 +309,12 @@ pub struct Args { #[arg(long, help_heading = "Execution")] test_tool: Option, + /// Run all tests in the workspace. + /// + /// If false, only the tests in the mutated package are run. + #[arg(long, help_heading = "Tests")] + test_workspace: Option, + /// Maximum run time for all cargo commands, in seconds. #[arg(long, short = 't', help_heading = "Execution")] timeout: Option, diff --git a/src/options.rs b/src/options.rs index eca47ad3..95d8e6cf 100644 --- a/src/options.rs +++ b/src/options.rs @@ -55,6 +55,9 @@ pub struct Options { /// The time multiplier for test tasks, if set (relative to baseline test duration). pub test_timeout_multiplier: Option, + /// Run all tests in the workspace, not just the current package. + pub test_workspace: bool, + /// The time limit for build tasks, if set. /// /// If this is not set by the user it's None, in which case there is no time limit @@ -243,6 +246,10 @@ impl Options { test_timeout: args.timeout.map(Duration::from_secs_f64), test_timeout_multiplier: args.timeout_multiplier.or(config.timeout_multiplier), test_tool: args.test_tool.or(config.test_tool).unwrap_or_default(), + test_workspace: args + .test_workspace + .or(config.test_workspace) + .unwrap_or(false), }; options.error_values.iter().for_each(|e| { if e.starts_with("Err(") { From 18f57d6cb015f5707680af5e44554440ea80e859 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Mon, 28 Oct 2024 07:53:19 -0700 Subject: [PATCH 02/37] Be clearer that --workspace configures what to mutate --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 4711acfc..15e60f93 100644 --- a/src/main.rs +++ b/src/main.rs @@ -339,7 +339,7 @@ pub struct Args { #[arg(long, action = clap::ArgAction::SetTrue)] version: bool, - /// Test every package in the workspace. + /// Generate mutations in every package in the workspace. #[arg(long, help_heading = "Filters")] workspace: bool, From 9ca6e010463429f242f9697ed9adf5d9bdfcb87b Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Mon, 28 Oct 2024 07:53:33 -0700 Subject: [PATCH 03/37] doc: new --test-workspace etc --- book/src/workspaces.md | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/book/src/workspaces.md b/book/src/workspaces.md index b944093f..8e22afe6 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -1,21 +1,41 @@ # Workspace and package support -cargo-mutants supports testing Cargo workspaces that contain multiple packages. The entire workspace tree is copied. +cargo-mutants supports testing Cargo workspaces that contain multiple packages. -By default, cargo-mutants has [the same behavior as Cargo](https://doc.rust-lang.org/cargo/reference/workspaces.html): +The entire workspace tree is copied to the temporary directory (unless `--in-place` is used). -* If `--workspace` is given, all packages in the workspace are tested. -* If `--package` is given, the named packages are tested. -* If the starting directory (or `-d` directory) is in a package, that package is tested. -* Otherwise, the starting directory must be in a virtual workspace. If it specifies default members, they are tested. Otherwise, all packages are tested. +In workspaces with multiple packages, there are two considerations: -For each mutant, only the containing package's tests are run, on the theory that -each package's tests are responsible for testing the package's code. +1. Which packages to generate mutants in, and +2. Which tests to run on those mutants. -The baseline tests exercise all and only the packages for which mutants will -be generated. +## Selecting packages to mutate + +By default, cargo-mutants selects packages to mutate using [similar heuristics to other Cargo commands](https://doc.rust-lang.org/cargo/reference/workspaces.html). + +These rules work from the "starting directory", which is the directory selected by `--dir` or the current working directory. + +* If `--workspace` is given, all packages in the workspace are mutated. +* If `--package` is given, the named packages are mutated. +* If the starting directory is in a package, that package is mutated. Concretely, this means: if the starting directory or its parents contain a `Cargo.toml` containing a `[package]` section. +* If the starting directory's parents contain a `Cargo.toml` with a `[workspace]` section but no `[package]` section, then the directory is said to be in a "virtual workspace". If the `[workspace]` section has a `default-members` key then these packages are mutated. Otherwise, all packages are mutated. + +Selection of packages can be combined with [`--file`](skip_files.md) and other filters. You can also use the `--file` options to restrict cargo-mutants to testing only files from some subdirectory, e.g. with `-f "utils/**/*.rs"`. (Remember to quote globs on the command line, so that the shell doesn't expand them.) You can use `--list` or `--list-files` to preview the effect of filters. + +## Selecting tests to run + +For each baseline and mutant scenario, cargo-mutants selects some tests to see if the mutant is caught. +These selections turn into `--package` or `--workspace` arguments to `cargo test`. + +By default, the baseline runs tests from all and only the packages for which mutants will be generated. That is, if the whole workspace is being tested, then it runs `cargo test --workspace`, and otherwise it selects all the packages. + +By default, each mutant runs only the tests from the package that's being mutated. + +If the `--test-workspace` arguments or `test_workspace` configuration key is set, then all tests from the workspace are run against each mutant. + +If the `--test-package` argument or `test_package` configuration key is set then the specified packages are tested for all mutants. From 9078a3d50dd22fc2bfd951bed2457b5d2c467216 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Mon, 4 Nov 2024 08:20:59 -0800 Subject: [PATCH 04/37] Move book gitignore pattern VS Code seems to think the pattern 'book' in the directory 'book' means that the directory should be ignored, which seems inconsistent with git itself, and probably buggy. https://github.com/microsoft/vscode/issues/231304 --- .gitignore | 1 + book/.gitignore | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 book/.gitignore diff --git a/.gitignore b/.gitignore index c2966ec9..a665e829 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ mutants.out.old .cargo/config.toml wiki .vscode/ +book/book diff --git a/book/.gitignore b/book/.gitignore deleted file mode 100644 index 7585238e..00000000 --- a/book/.gitignore +++ /dev/null @@ -1 +0,0 @@ -book From f4bdac7de15a04dded817e84da374fe24011a6d2 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Mon, 4 Nov 2024 08:50:14 -0800 Subject: [PATCH 05/37] Parse --test-options and --test-packages --- book/src/workspaces.md | 12 ++- src/config.rs | 6 +- src/lab.rs | 21 +++--- src/main.rs | 6 ++ src/options.rs | 162 +++++++++++++++++++++++++++++++++++------ 5 files changed, 170 insertions(+), 37 deletions(-) diff --git a/book/src/workspaces.md b/book/src/workspaces.md index 8e22afe6..2de95d58 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -32,10 +32,16 @@ on the command line, so that the shell doesn't expand them.) You can use `--list For each baseline and mutant scenario, cargo-mutants selects some tests to see if the mutant is caught. These selections turn into `--package` or `--workspace` arguments to `cargo test`. -By default, the baseline runs tests from all and only the packages for which mutants will be generated. That is, if the whole workspace is being tested, then it runs `cargo test --workspace`, and otherwise it selects all the packages. +There are different behaviors for the baseline tests (before mutation), which run once for all packages, and then for the tests applied to each mutant. + +These behaviors can be controlled by the `--test-workspace` and `--test-package` command line options and the corresponding configuration options. + +By default, the baseline runs the tests from all and only the packages for which mutants will be generated. That is, if the whole workspace is being tested, then it runs `cargo test --workspace`, and otherwise runs tests for each selected package. By default, each mutant runs only the tests from the package that's being mutated. -If the `--test-workspace` arguments or `test_workspace` configuration key is set, then all tests from the workspace are run against each mutant. +If the `--test-workspace` arguments or `test_workspace` configuration key is set, then all tests from the workspace are run for the baseline and against each mutant. + +If the `--test-package` argument or `test_package` configuration key is set then the specified packages are tested for the baseline and all mutants. -If the `--test-package` argument or `test_package` configuration key is set then the specified packages are tested for all mutants. +As for other options, the command line arguments have priority over the configuration file. diff --git a/src/config.rs b/src/config.rs index 69300bff..d85271f8 100644 --- a/src/config.rs +++ b/src/config.rs @@ -47,6 +47,8 @@ pub struct Config { pub minimum_test_timeout: Option, /// Cargo profile. pub profile: Option, + /// Run tests from these packages for all mutants. + pub test_packages: Vec, /// Choice of test tool: cargo or nextest. pub test_tool: Option, /// Timeout multiplier, relative to the baseline 'cargo test'. @@ -54,13 +56,15 @@ pub struct Config { /// Build timeout multiplier, relative to the baseline 'cargo build'. pub build_timeout_multiplier: Option, /// Run tests from all packages in the workspace, not just the mutated package. + /// + /// Overrides `test_packages`. pub test_workspace: Option, } impl Config { pub fn read_file(path: &Path) -> Result { let toml = read_to_string(path).with_context(|| format!("read config {path:?}"))?; - toml::de::from_str(&toml).with_context(|| format!("parse toml from {path:?}")) + Config::from_str(&toml).with_context(|| format!("parse toml from {path:?}")) } /// Read the config from a tree's `.cargo/mutants.toml`, and return a default (empty) diff --git a/src/lab.rs b/src/lab.rs index 1b1e50e3..3789d3e4 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -12,12 +12,11 @@ use std::time::Instant; use itertools::Itertools; use tracing::{debug, debug_span, error, trace, warn}; -use crate::cargo::run_cargo; -use crate::outcome::LabOutcome; -use crate::output::OutputDir; -use crate::package::Package; -use crate::timeouts::Timeouts; use crate::*; +use crate::{ + cargo::run_cargo, options::TestPackages, outcome::LabOutcome, output::OutputDir, + package::Package, timeouts::Timeouts, +}; /// Run all possible mutation experiments. /// @@ -126,7 +125,7 @@ pub fn test_mutants( match next { Err(err) => { // PoisonError is not Send so we can't pass it directly. - return Err(anyhow!("Lock pending work queue: {}", err)); + return Err(anyhow!("Failed to lock pending work queue: {}", err)); } Ok(Some(mutant)) => { let scenario = Scenario::Mutant(mutant.clone()); @@ -134,10 +133,12 @@ pub fn test_mutants( debug_span!("mutant", name = mutant.name(false, false)).entered(); let package = mutant.package().clone(); // hold let packages = [&package]; - let test_packages: Option<&[&Package]> = if options.test_workspace { - None - } else { - Some(&packages) + let test_packages: Option<&[&Package]> = match &options.test_packages { + TestPackages::Workspace => None, + TestPackages::Mutated => Some(&packages), + TestPackages::Named(_named) => { + unimplemented!("get packages by name") + } }; test_scenario( &build_dir, diff --git a/src/main.rs b/src/main.rs index 15e60f93..1f38ab08 100644 --- a/src/main.rs +++ b/src/main.rs @@ -305,6 +305,10 @@ pub struct Args { #[arg(long, help_heading = "Execution")] shard: Option, + /// Run tests from these packages for all mutants. + #[arg(long, help_heading = "Tests")] + test_packages: Vec, + /// Tool used to run test suites: cargo or nextest. #[arg(long, help_heading = "Execution")] test_tool: Option, @@ -312,6 +316,8 @@ pub struct Args { /// Run all tests in the workspace. /// /// If false, only the tests in the mutated package are run. + /// + /// Overrides `--test_packages`. #[arg(long, help_heading = "Tests")] test_workspace: Option, diff --git a/src/options.rs b/src/options.rs index 95d8e6cf..8af66136 100644 --- a/src/options.rs +++ b/src/options.rs @@ -55,8 +55,10 @@ pub struct Options { /// The time multiplier for test tasks, if set (relative to baseline test duration). pub test_timeout_multiplier: Option, - /// Run all tests in the workspace, not just the current package. - pub test_workspace: bool, + /// Which packages to test for a given mutant. + /// + /// Comes from `--test-workspace` etc. + pub test_packages: TestPackages, /// The time limit for build tasks, if set. /// @@ -134,6 +136,20 @@ pub struct Options { pub test_tool: TestTool, } +/// Which packages should be tested for a given mutant? +#[derive(Debug, Default, Clone, PartialEq, Eq, EnumString, Display, Deserialize)] +pub enum TestPackages { + /// Only the package containing the mutated file. + #[default] + Mutated, + + /// All packages in the workspace. + Workspace, + + /// Certain packages, specified by name. + Named(Vec), +} + /// Choice of tool to use to run tests. #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, EnumString, Display, Deserialize)] #[strum(serialize_all = "snake_case")] @@ -204,6 +220,25 @@ impl Options { .unwrap_or(20f64), ); + // If either command line argument is set, it overrides the config. + let test_packages = if args.test_workspace == Some(true) { + TestPackages::Workspace + } else if !args.test_packages.is_empty() { + TestPackages::Named( + args.test_packages + .iter() + .flat_map(|s| s.split(',')) + .map(|s| s.to_string()) + .collect(), + ) + } else if args.test_workspace.is_none() && config.test_workspace == Some(true) { + TestPackages::Workspace + } else if !config.test_packages.is_empty() { + TestPackages::Named(config.test_packages.clone()) + } else { + TestPackages::Mutated + }; + let options = Options { additional_cargo_args: join_slices(&args.cargo_arg, &config.additional_cargo_args), additional_cargo_test_args: join_slices( @@ -243,13 +278,10 @@ impl Options { show_line_col: args.line_col, show_times: !args.no_times, show_all_logs: args.all_logs, + test_packages, test_timeout: args.timeout.map(Duration::from_secs_f64), test_timeout_multiplier: args.timeout_multiplier.or(config.timeout_multiplier), test_tool: args.test_tool.or(config.test_tool).unwrap_or_default(), - test_workspace: args - .test_workspace - .or(config.test_workspace) - .unwrap_or(false), }; options.error_values.iter().for_each(|e| { if e.starts_with("Err(") { @@ -280,6 +312,7 @@ fn or_slices<'a: 'c, 'b: 'c, 'c, T>(a: &'a [T], b: &'b [T]) -> &'c [T] { #[cfg(test)] mod test { use std::io::Write; + use std::str::FromStr; use indoc::indoc; use rusty_fork::rusty_fork_test; @@ -531,28 +564,111 @@ mod test { let options = Options::new(&args, &Config::default()).unwrap(); assert_eq!(options.colors.forced_value(), None); } + } - #[test] - fn profile_option_from_args() { - let args = Args::parse_from(["mutants", "--profile=mutants"]); - let options = Options::new(&args, &Config::default()).unwrap(); - assert_eq!(options.profile.unwrap(), "mutants"); - } - + #[test] + fn profile_option_from_args() { + let args = Args::parse_from(["mutants", "--profile=mutants"]); + let options = Options::new(&args, &Config::default()).unwrap(); + assert_eq!(options.profile.unwrap(), "mutants"); + } - #[test] - fn profile_from_config() { - let args = Args::parse_from(["mutants", "-j3"]); - let config = indoc! { r#" + #[test] + fn profile_from_config() { + let args = Args::parse_from(["mutants", "-j3"]); + let config = indoc! { r#" profile = "mutants" timeout_multiplier = 1.0 build_timeout_multiplier = 2.0 "#}; - let mut config_file = NamedTempFile::new().unwrap(); - config_file.write_all(config.as_bytes()).unwrap(); - let config = Config::read_file(config_file.path()).unwrap(); - let options = Options::new(&args, &config).unwrap(); - assert_eq!(options.profile.unwrap(), "mutants"); - } + let mut config_file = NamedTempFile::new().unwrap(); + config_file.write_all(config.as_bytes()).unwrap(); + let config = Config::read_file(config_file.path()).unwrap(); + let options = Options::new(&args, &config).unwrap(); + assert_eq!(options.profile.unwrap(), "mutants"); + } + + #[test] + fn test_workspace_arg_true() { + let args = Args::parse_from(["mutants", "--test-workspace=true"]); + let options = Options::new(&args, &Config::default()).unwrap(); + assert_eq!(options.test_packages, 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(); + assert_eq!(options.test_packages, TestPackages::Mutated); + } + + #[test] + fn test_workspace_config_true() { + let args = Args::parse_from(["mutants"]); + let config = indoc! { r#" + test_workspace = true + "#}; + let config = Config::from_str(config).unwrap(); + let options = Options::new(&args, &config).unwrap(); + assert_eq!(options.test_packages, TestPackages::Workspace); + } + + #[test] + fn test_workspace_config_false() { + let args = Args::parse_from(["mutants"]); + let config = indoc! { r#" + test_workspace = false + "#}; + let config = Config::from_str(config).unwrap(); + let options = Options::new(&args, &config).unwrap(); + assert_eq!(options.test_packages, TestPackages::Mutated); + } + + #[test] + fn test_workspace_args_override_config_true() { + let args = Args::parse_from(["mutants", "--test-workspace=true"]); + let config = indoc! { r#" + test_workspace = false + "#}; + let config = Config::from_str(config).unwrap(); + let options = Options::new(&args, &config).unwrap(); + assert_eq!(options.test_packages, TestPackages::Workspace); + } + + #[test] + fn test_workspace_args_override_config_false() { + let args = Args::parse_from(["mutants", "--test-workspace=false"]); + let config = indoc! { r#" + test_workspace = true + "#}; + let config = Config::from_str(config).unwrap(); + let options = Options::new(&args, &config).unwrap(); + assert_eq!(options.test_packages, TestPackages::Mutated); + } + + #[test] + fn test_workspace_arg_false_allows_packages_from_config() { + let args = Args::parse_from(["mutants", "--test-workspace=false"]); + let config = indoc! { r#" + # Normally the packages would be ignored, but --test-workspace=false. + test_workspace = true + test_packages = ["foo", "bar"] + "#}; + let config = Config::from_str(config).unwrap(); + let options = Options::new(&args, &config).unwrap(); + assert_eq!( + options.test_packages, + TestPackages::Named(vec!["foo".to_string(), "bar".to_string()]) + ); + } + + #[test] + fn test_packages_arg_with_commas() { + let args = Args::parse_from(["mutants", "--test-packages=foo,bar"]); + let options = Options::new(&args, &Config::default()).unwrap(); + assert_eq!( + options.test_packages, + TestPackages::Named(vec!["foo".to_string(), "bar".to_string()]) + ); } } From 05336b46bc298274add0086bc91c81eed059360d Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 06:05:44 -0800 Subject: [PATCH 06/37] factor out Options.phases() --- src/lab.rs | 7 +------ src/options.rs | 9 +++++++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 3789d3e4..b7dc0ecf 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -223,11 +223,6 @@ fn test_scenario( let dir = build_dir.path(); console.scenario_started(dir, scenario, scenario_output.open_log_read()?)?; - let phases: &[Phase] = if options.check_only { - &[Phase::Check] - } else { - &[Phase::Build, Phase::Test] - }; if let Some(mutant) = scenario.mutant() { let mutated_code = mutant.mutated_code(); let diff = scenario.mutant().unwrap().diff(&mutated_code); @@ -236,7 +231,7 @@ fn test_scenario( } let mut outcome = ScenarioOutcome::new(&scenario_output, scenario.clone()); - for &phase in phases { + for &phase in options.phases() { console.scenario_phase_started(dir, phase); let timeout = match phase { Phase::Test => timeouts.test, diff --git a/src/options.rs b/src/options.rs index 8af66136..2613349e 100644 --- a/src/options.rs +++ b/src/options.rs @@ -298,6 +298,15 @@ impl Options { 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 { + &[Phase::Check] + } else { + &[Phase::Build, Phase::Test] + } + } } /// If the first slices is non-empty, return that, otherwise the second. From 88a7149460b8bef351eebf57fb878ba724fa2c1d Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 06:07:26 -0800 Subject: [PATCH 07/37] Stop scenario phases on an internal error Previously, only mutants were stopped and baselines were allowed to continue: but this was unlikely to be hit. --- src/lab.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lab.rs b/src/lab.rs index b7dc0ecf..f1ad882a 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -256,11 +256,12 @@ fn test_scenario( } } Err(err) => { + error!(?err, ?phase, "scenario execution internal error"); // Some unexpected internal error that stops the program. if let Some(mutant) = scenario.mutant() { mutant.revert(build_dir)?; - return Err(err); } + return Err(err); } } } From 253f84a55a915b92b2b3ad747d8f2e8e9e89987d Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 06:09:53 -0800 Subject: [PATCH 08/37] Start jobserver a little later Wait until we're sure we have mutants to test and are about to begin. --- src/lab.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index f1ad882a..61c33988 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -34,15 +34,6 @@ pub fn test_mutants( ) -> Result { let start_time = Instant::now(); console.set_debug_log(output_dir.open_debug_log()?); - let jobserver = options - .jobserver - .then(|| { - let n_tasks = options.jobserver_tasks.unwrap_or_else(num_cpus::get); - debug!(n_tasks, "starting jobserver"); - jobserver::Client::new(n_tasks) - }) - .transpose() - .context("Start jobserver")?; if options.shuffle { fastrand::shuffle(&mut mutants); } @@ -61,6 +52,15 @@ pub fn test_mutants( false => BuildDir::copy_from(workspace_dir, options.gitignore, options.leak_dirs, console)?, }; + let jobserver = options + .jobserver + .then(|| { + let n_tasks = options.jobserver_tasks.unwrap_or_else(num_cpus::get); + debug!(n_tasks, "starting jobserver"); + jobserver::Client::new(n_tasks) + }) + .transpose() + .context("Start jobserver")?; let timeouts = match options.baseline { BaselineStrategy::Run => { let outcome = test_scenario( From 773a51b86e3a3dac1b120608ab94d957e1b599dc Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 06:59:26 -0800 Subject: [PATCH 09/37] Refactor/simplify lab code --- src/lab.rs | 255 ++++++++++++++++++++++++++-------------------------- src/main.rs | 2 +- 2 files changed, 130 insertions(+), 127 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 61c33988..812ad75c 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -6,8 +6,8 @@ use std::cmp::{max, min}; use std::panic::resume_unwind; use std::sync::Mutex; -use std::thread; use std::time::Instant; +use std::{thread, vec}; use itertools::Itertools; use tracing::{debug, debug_span, error, trace, warn}; @@ -29,7 +29,7 @@ pub fn test_mutants( mut mutants: Vec, workspace_dir: &Utf8Path, output_dir: OutputDir, - options: Options, + options: &Options, console: &Console, ) -> Result { let start_time = Instant::now(); @@ -47,12 +47,12 @@ pub fn test_mutants( debug!(?mutant_packages); let output_mutex = Mutex::new(output_dir); - let build_dir = match options.in_place { + let baseline_build_dir = match options.in_place { true => BuildDir::in_place(workspace_dir)?, false => BuildDir::copy_from(workspace_dir, options.gitignore, options.leak_dirs, console)?, }; - let jobserver = options + let jobserver = &options .jobserver .then(|| { let n_tasks = options.jobserver_tasks.unwrap_or_else(num_cpus::get); @@ -63,40 +63,38 @@ pub fn test_mutants( .context("Start jobserver")?; let timeouts = match options.baseline { BaselineStrategy::Run => { - let outcome = test_scenario( - &build_dir, - &output_mutex, - &jobserver, - &Scenario::Baseline, - Some(&mutant_packages), - Timeouts::for_baseline(&options), - &options, + let outcome = Worker { + build_dir: &baseline_build_dir, + output_mutex: &output_mutex, + jobserver, + timeouts: Timeouts::for_baseline(options), + options, console, - )?; + } + .run_one_scenario(Scenario::Baseline, Some(&mutant_packages))?; if !outcome.success() { error!( "cargo {} failed in an unmutated tree, so no mutants were tested", outcome.last_phase(), ); - // We "successfully" established that the baseline tree doesn't work; arguably this should be represented as an error - // but we'd need a way for that error to convey an exit code... return Ok(output_mutex .into_inner() .expect("lock output_dir") .take_lab_outcome()); + } else { + Timeouts::from_baseline(&outcome, options) } - Timeouts::from_baseline(&outcome, &options) } - BaselineStrategy::Skip => Timeouts::without_baseline(&options), + BaselineStrategy::Skip => Timeouts::without_baseline(options), }; debug!(?timeouts); - let build_dir_0 = Mutex::new(Some(build_dir)); + let build_dir_0 = Mutex::new(Some(baseline_build_dir)); // Create n threads, each dedicated to one build directory. Each of them tries to take a // scenario to test off the queue, and then exits when there are no more left. console.start_testing_mutants(mutants.len()); let n_threads = max(1, min(options.jobs.unwrap_or(1), mutants.len())); - let pending = Mutex::new(mutants.into_iter()); + let work_queue = &Mutex::new(mutants.into_iter()); thread::scope(|scope| -> crate::Result<()> { let mut threads = Vec::new(); for _i_thread in 0..n_threads { @@ -104,7 +102,7 @@ pub fn test_mutants( trace!(thread_id = ?thread::current().id(), "start thread"); // First thread to start can use the initial build dir; others need to copy a new one let build_dir_0 = build_dir_0.lock().expect("lock build dir 0").take(); // separate for lock - let build_dir = match build_dir_0 { + let build_dir = &match build_dir_0 { Some(d) => d, None => { debug!("copy build dir"); @@ -116,46 +114,15 @@ pub fn test_mutants( )? } }; - let _thread_span = - debug_span!("worker thread", build_dir = ?build_dir.path()).entered(); - loop { - // Extract the mutant in a separate statement so that we don't hold the - // lock while testing it. - let next = pending.lock().map(|mut s| s.next()); // separate for lock - match next { - Err(err) => { - // PoisonError is not Send so we can't pass it directly. - return Err(anyhow!("Failed to lock pending work queue: {}", err)); - } - Ok(Some(mutant)) => { - let scenario = Scenario::Mutant(mutant.clone()); - let _span = - debug_span!("mutant", name = mutant.name(false, false)).entered(); - let package = mutant.package().clone(); // hold - let packages = [&package]; - let test_packages: Option<&[&Package]> = match &options.test_packages { - TestPackages::Workspace => None, - TestPackages::Mutated => Some(&packages), - TestPackages::Named(_named) => { - unimplemented!("get packages by name") - } - }; - test_scenario( - &build_dir, - &output_mutex, - &jobserver, - &scenario, - test_packages, - timeouts, - &options, - console, - )?; - } - Ok(None) => { - return Ok(()); // no more work for this thread - } - } - } + let worker = Worker { + build_dir, + output_mutex: &output_mutex, + jobserver, + timeouts, + options, + console, + }; + worker.run_queue(work_queue) })); } // The errors potentially returned from `join` are a special `std::thread::Result` @@ -189,7 +156,7 @@ pub fn test_mutants( let output_dir = output_mutex .into_inner() .expect("final unlock mutants queue"); - console.lab_finished(&output_dir.lab_outcome, start_time, &options); + console.lab_finished(&output_dir.lab_outcome, start_time, options); let lab_outcome = output_dir.take_lab_outcome(); if lab_outcome.total_mutants == 0 { // This should be unreachable as we also bail out before copying @@ -201,79 +168,115 @@ pub fn test_mutants( Ok(lab_outcome) } -/// Test various phases of one scenario in a build dir. +/// A worker owns one build directory and runs a single thread of testing. /// -/// The [BuildDir] is passed as mutable because it's for the exclusive use of this function for the -/// duration of the test. -#[allow(clippy::too_many_arguments)] // I agree it's a lot but I'm not sure wrapping in a struct would be better. -fn test_scenario( - build_dir: &BuildDir, - output_mutex: &Mutex, - jobserver: &Option, - scenario: &Scenario, - test_packages: Option<&[&Package]>, +/// It consumes jobs from an input queue and runs them until the queue is empty, +/// appending output to the output directory. +struct Worker<'a> { + build_dir: &'a BuildDir, + output_mutex: &'a Mutex, + jobserver: &'a Option, timeouts: Timeouts, - options: &Options, - console: &Console, -) -> Result { - let mut scenario_output = output_mutex - .lock() - .expect("lock output_dir to start scenario") - .start_scenario(scenario)?; - let dir = build_dir.path(); - console.scenario_started(dir, scenario, scenario_output.open_log_read()?)?; + options: &'a Options, + console: &'a Console, +} - if let Some(mutant) = scenario.mutant() { - let mutated_code = mutant.mutated_code(); - let diff = scenario.mutant().unwrap().diff(&mutated_code); - scenario_output.write_diff(&diff)?; - mutant.apply(build_dir, &mutated_code)?; +impl Worker<'_> { + /// Run until the input queue is empty. + fn run_queue(mut self, work_queue: &Mutex>) -> Result<()> { + let _thread_span = + debug_span!("worker thread", build_dir = ?self.build_dir.path()).entered(); + loop { + // Extract the mutant in a separate statement so that we don't hold the + // lock while testing it. + let next_mutant = work_queue.lock().expect("Lock pending work queue").next(); // separate for lock + if let Some(mutant) = next_mutant { + let _span = debug_span!("mutant", name = mutant.name(false, false)).entered(); + let package = mutant.package().clone(); // hold + let packages = [&package]; // hold + let scenario = Scenario::Mutant(mutant); + let test_packages: Option<&[&Package]> = match &self.options.test_packages { + TestPackages::Workspace => None, + TestPackages::Mutated => Some(&packages), + TestPackages::Named(_named) => { + unimplemented!("get packages by name") + } + }; + self.run_one_scenario(scenario, test_packages)?; + } else { + return Ok(()); + } + } } - let mut outcome = ScenarioOutcome::new(&scenario_output, scenario.clone()); - for &phase in options.phases() { - console.scenario_phase_started(dir, phase); - let timeout = match phase { - Phase::Test => timeouts.test, - Phase::Build | Phase::Check => timeouts.build, - }; - match run_cargo( - build_dir, - jobserver, - test_packages, - phase, - timeout, - &mut scenario_output, - options, - console, - ) { - Ok(phase_result) => { - let success = phase_result.is_success(); // so we can move it away - outcome.add_phase_result(phase_result); - console.scenario_phase_finished(dir, phase); - if !success { - break; + // #[allow(clippy::too_many_arguments)] // I agree it's a lot but I'm not sure wrapping in a struct would be better. + fn run_one_scenario( + &mut self, + scenario: Scenario, + test_packages: Option<&[&Package]>, + ) -> Result { + let mut scenario_output = self + .output_mutex + .lock() + .expect("lock output_dir to start scenario") + .start_scenario(&scenario)?; + let dir = self.build_dir.path(); + self.console + .scenario_started(dir, &scenario, scenario_output.open_log_read()?)?; + + if let Some(mutant) = scenario.mutant() { + let mutated_code = mutant.mutated_code(); + let diff = scenario.mutant().unwrap().diff(&mutated_code); + scenario_output.write_diff(&diff)?; + mutant.apply(self.build_dir, &mutated_code)?; + } + + let mut outcome = ScenarioOutcome::new(&scenario_output, scenario.clone()); + for &phase in self.options.phases() { + self.console.scenario_phase_started(dir, phase); + let timeout = match phase { + Phase::Test => self.timeouts.test, + Phase::Build | Phase::Check => self.timeouts.build, + }; + match run_cargo( + self.build_dir, + self.jobserver, + test_packages, + phase, + timeout, + &mut scenario_output, + self.options, + self.console, + ) { + Ok(phase_result) => { + let success = phase_result.is_success(); // so we can move it away + outcome.add_phase_result(phase_result); + self.console.scenario_phase_finished(dir, phase); + if !success { + break; + } } - } - Err(err) => { - error!(?err, ?phase, "scenario execution internal error"); - // Some unexpected internal error that stops the program. - if let Some(mutant) = scenario.mutant() { - mutant.revert(build_dir)?; + Err(err) => { + error!(?err, ?phase, "scenario execution internal error"); + // Some unexpected internal error that stops the program. + if let Some(mutant) = scenario.mutant() { + mutant.revert(self.build_dir)?; + } + return Err(err); } - return Err(err); } } - } - if let Some(mutant) = scenario.mutant() { - mutant.revert(build_dir)?; - } - output_mutex - .lock() - .expect("lock output dir to add outcome") - .add_scenario_outcome(&outcome)?; - debug!(outcome = ?outcome.summary()); - console.scenario_finished(dir, scenario, &outcome, options); + if let Some(mutant) = scenario.mutant() { + mutant.revert(self.build_dir)?; + } + self.output_mutex + .lock() + .expect("lock output dir to add outcome") + .add_scenario_outcome(&outcome)?; + debug!(outcome = ?outcome.summary()); + self.console + .scenario_finished(dir, &scenario, &outcome, self.options); - Ok(outcome) + Ok(outcome) + } } diff --git a/src/main.rs b/src/main.rs index 1f38ab08..fb3bde60 100644 --- a/src/main.rs +++ b/src/main.rs @@ -483,7 +483,7 @@ fn main() -> Result<()> { output_dir.write_previously_caught(&previously_caught)?; } console.set_debug_log(output_dir.open_debug_log()?); - let lab_outcome = test_mutants(mutants, workspace.root(), output_dir, options, &console)?; + let lab_outcome = test_mutants(mutants, workspace.root(), output_dir, &options, &console)?; exit(lab_outcome.exit_code()); } Ok(()) From af6570ceed39a110b8c8a20612b2bd67df140954 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 07:18:20 -0800 Subject: [PATCH 10/37] clippy: more cleanups --- src/lab.rs | 58 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 28 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 812ad75c..fed42a27 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -3,6 +3,8 @@ //! Successively apply mutations to the source code and run cargo to check, //! build, and test them. +#![warn(clippy::pedantic)] + use std::cmp::{max, min}; use std::panic::resume_unwind; use std::sync::Mutex; @@ -12,10 +14,10 @@ use std::{thread, vec}; use itertools::Itertools; use tracing::{debug, debug_span, error, trace, warn}; -use crate::*; use crate::{ cargo::run_cargo, options::TestPackages, outcome::LabOutcome, output::OutputDir, - package::Package, timeouts::Timeouts, + package::Package, timeouts::Timeouts, BaselineStrategy, BuildDir, Console, Context, Mutant, + Options, Phase, Result, Scenario, ScenarioOutcome, Utf8Path, }; /// Run all possible mutation experiments. @@ -25,6 +27,7 @@ use crate::{ /// /// Before testing the mutants, the lab checks that the source tree passes its tests with no /// mutations applied. +#[allow(clippy::too_many_lines)] // just for now pub fn test_mutants( mut mutants: Vec, workspace_dir: &Utf8Path, @@ -43,13 +46,14 @@ pub fn test_mutants( warn!("No mutants found under the active filters"); return Ok(LabOutcome::default()); } - let mutant_packages = mutants.iter().map(|m| m.package()).unique().collect_vec(); // hold + let mutant_packages = mutants.iter().map(Mutant::package).unique().collect_vec(); // hold debug!(?mutant_packages); let output_mutex = Mutex::new(output_dir); - let baseline_build_dir = match options.in_place { - true => BuildDir::in_place(workspace_dir)?, - false => BuildDir::copy_from(workspace_dir, options.gitignore, options.leak_dirs, console)?, + let baseline_build_dir = if options.in_place { + BuildDir::in_place(workspace_dir)? + } else { + BuildDir::copy_from(workspace_dir, options.gitignore, options.leak_dirs, console)? }; let jobserver = &options @@ -71,8 +75,10 @@ pub fn test_mutants( options, console, } - .run_one_scenario(Scenario::Baseline, Some(&mutant_packages))?; - if !outcome.success() { + .run_one_scenario(&Scenario::Baseline, Some(&mutant_packages))?; + if outcome.success() { + Timeouts::from_baseline(&outcome, options) + } else { error!( "cargo {} failed in an unmutated tree, so no mutants were tested", outcome.last_phase(), @@ -81,8 +87,6 @@ pub fn test_mutants( .into_inner() .expect("lock output_dir") .take_lab_outcome()); - } else { - Timeouts::from_baseline(&outcome, options) } } BaselineStrategy::Skip => Timeouts::without_baseline(options), @@ -102,17 +106,16 @@ pub fn test_mutants( trace!(thread_id = ?thread::current().id(), "start thread"); // First thread to start can use the initial build dir; others need to copy a new one let build_dir_0 = build_dir_0.lock().expect("lock build dir 0").take(); // separate for lock - let build_dir = &match build_dir_0 { - Some(d) => d, - None => { - debug!("copy build dir"); - BuildDir::copy_from( - workspace_dir, - options.gitignore, - options.leak_dirs, - console, - )? - } + let build_dir = &if let Some(d) = build_dir_0 { + d + } else { + debug!("copy build dir"); + BuildDir::copy_from( + workspace_dir, + options.gitignore, + options.leak_dirs, + console, + )? }; let worker = Worker { build_dir, @@ -133,7 +136,7 @@ pub fn test_mutants( // etc. In that case, print them all and return the first. let errors = threads .into_iter() - .flat_map(|thread| match thread.join() { + .filter_map(|thread| match thread.join() { Err(panic) => resume_unwind(panic), Ok(Ok(())) => None, Ok(Err(err)) => { @@ -202,27 +205,26 @@ impl Worker<'_> { unimplemented!("get packages by name") } }; - self.run_one_scenario(scenario, test_packages)?; + self.run_one_scenario(&scenario, test_packages)?; } else { return Ok(()); } } } - // #[allow(clippy::too_many_arguments)] // I agree it's a lot but I'm not sure wrapping in a struct would be better. fn run_one_scenario( &mut self, - scenario: Scenario, + scenario: &Scenario, test_packages: Option<&[&Package]>, ) -> Result { let mut scenario_output = self .output_mutex .lock() .expect("lock output_dir to start scenario") - .start_scenario(&scenario)?; + .start_scenario(scenario)?; let dir = self.build_dir.path(); self.console - .scenario_started(dir, &scenario, scenario_output.open_log_read()?)?; + .scenario_started(dir, scenario, scenario_output.open_log_read()?)?; if let Some(mutant) = scenario.mutant() { let mutated_code = mutant.mutated_code(); @@ -275,7 +277,7 @@ impl Worker<'_> { .add_scenario_outcome(&outcome)?; debug!(outcome = ?outcome.summary()); self.console - .scenario_finished(dir, &scenario, &outcome, self.options); + .scenario_finished(dir, scenario, &outcome, self.options); Ok(outcome) } From 751b6e54df3983459cbd36e9baef8a90cb27be6e Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 07:32:51 -0800 Subject: [PATCH 11/37] clean up --- src/workspace.rs | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/workspace.rs b/src/workspace.rs index cf583501..9a4b4196 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -22,16 +22,6 @@ use crate::source::SourceFile; use crate::visit::{walk_tree, Discovered}; use crate::Result; -impl fmt::Debug for Workspace { - #[mutants::skip] - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("Workspace") - .field("root", &self.root().to_string()) - // .field("metadata", &self.metadata) - .finish() - } -} - /// Which packages to mutate in a workspace? #[derive(Debug, Clone)] pub enum PackageFilter { @@ -103,10 +93,21 @@ struct PackageTop { top_sources: Vec, } +/// A cargo workspace. pub struct Workspace { metadata: cargo_metadata::Metadata, } +impl fmt::Debug for Workspace { + #[mutants::skip] + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Workspace") + .field("root", &self.root().to_string()) + // .field("metadata", &self.metadata) + .finish() + } +} + impl Workspace { /// The root directory of the workspace. pub fn root(&self) -> &Utf8Path { From 206aec86c6aa37f9bdb72cf50a24a5ef5c5b96d1 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 07:32:59 -0800 Subject: [PATCH 12/37] Refactor BuildDir constructors --- src/build_dir.rs | 71 ++++++++++++++++++++++++++++++++++++++++++------ src/lab.rs | 20 ++++---------- src/main.rs | 2 +- 3 files changed, 68 insertions(+), 25 deletions(-) diff --git a/src/build_dir.rs b/src/build_dir.rs index e6aa7141..5f37ef48 100644 --- a/src/build_dir.rs +++ b/src/build_dir.rs @@ -34,18 +34,29 @@ impl Debug for BuildDir { } impl BuildDir { - /// Make a new build dir, copying from a source directory, subject to exclusions. - pub fn copy_from( - source: &Utf8Path, - gitignore: bool, - leak_temp_dir: bool, + /// Make the build dir for the baseline. + /// + /// Depending on the options, this might be either a copy of the source directory + /// or in-place. + pub fn for_baseline( + workspace: &Workspace, + options: &Options, console: &Console, ) -> Result { + if options.in_place { + BuildDir::in_place(workspace.root()) + } else { + BuildDir::copy_from(workspace.root(), options, console) + } + } + + /// Make a new build dir, copying from a source directory, subject to exclusions. + pub fn copy_from(source: &Utf8Path, options: &Options, console: &Console) -> Result { let name_base = format!("cargo-mutants-{}-", source.file_name().unwrap_or("unnamed")); let source_abs = source .canonicalize_utf8() .context("canonicalize source path")?; - let temp_dir = copy_tree(source, &name_base, gitignore, console)?; + let temp_dir = copy_tree(source, &name_base, options.gitignore, console)?; let path: Utf8PathBuf = temp_dir .path() .to_owned() @@ -53,7 +64,7 @@ impl BuildDir { .context("tempdir path to UTF-8")?; fix_manifest(&path.join("Cargo.toml"), &source_abs)?; fix_cargo_config(&path, &source_abs)?; - let temp_dir = if leak_temp_dir { + let temp_dir = if options.leak_dirs { let _ = temp_dir.into_path(); info!(?path, "Build directory will be leaked for inspection"); None @@ -98,8 +109,13 @@ mod test { fn build_dir_copy_from() { let tmp = copy_of_testdata("factorial"); let workspace = Workspace::open(tmp.path()).unwrap(); - let build_dir = - BuildDir::copy_from(workspace.root(), true, false, &Console::new()).unwrap(); + let options = Options { + in_place: false, + gitignore: true, + leak_dirs: false, + ..Default::default() + }; + let build_dir = BuildDir::copy_from(workspace.root(), &options, &Console::new()).unwrap(); let debug_form = format!("{build_dir:?}"); println!("debug form is {debug_form:?}"); assert!(debug_form.starts_with("BuildDir { path: ")); @@ -108,6 +124,43 @@ mod test { assert!(build_dir.path().join("src").is_dir()); } + #[test] + fn for_baseline_in_place() -> Result<()> { + let tmp = copy_of_testdata("factorial"); + let workspace = Workspace::open(tmp.path())?; + let options = Options { + in_place: true, + ..Default::default() + }; + let build_dir = BuildDir::for_baseline(&workspace, &options, &Console::new())?; + assert_eq!( + build_dir.path().canonicalize_utf8()?, + workspace.root().canonicalize_utf8()? + ); + assert!(build_dir.temp_dir.is_none()); + Ok(()) + } + + #[test] + fn for_baseline_copied() -> Result<()> { + let tmp = copy_of_testdata("factorial"); + let workspace = Workspace::open(tmp.path())?; + let options = Options { + in_place: false, + ..Default::default() + }; + let build_dir = BuildDir::for_baseline(&workspace, &options, &Console::new())?; + assert!(build_dir.path().is_dir()); + assert!(build_dir.path().join("Cargo.toml").is_file()); + assert!(build_dir.path().join("src").is_dir()); + assert!(build_dir.temp_dir.is_some()); + assert_ne!( + build_dir.path().canonicalize_utf8()?, + workspace.root().canonicalize_utf8()? + ); + Ok(()) + } + #[test] fn build_dir_in_place() -> Result<()> { let tmp = copy_of_testdata("factorial"); diff --git a/src/lab.rs b/src/lab.rs index fed42a27..70352ca2 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -16,8 +16,8 @@ use tracing::{debug, debug_span, error, trace, warn}; use crate::{ cargo::run_cargo, options::TestPackages, outcome::LabOutcome, output::OutputDir, - package::Package, timeouts::Timeouts, BaselineStrategy, BuildDir, Console, Context, Mutant, - Options, Phase, Result, Scenario, ScenarioOutcome, Utf8Path, + package::Package, timeouts::Timeouts, workspace::Workspace, BaselineStrategy, BuildDir, + Console, Context, Mutant, Options, Phase, Result, Scenario, ScenarioOutcome, }; /// Run all possible mutation experiments. @@ -30,7 +30,7 @@ use crate::{ #[allow(clippy::too_many_lines)] // just for now pub fn test_mutants( mut mutants: Vec, - workspace_dir: &Utf8Path, + workspace: &Workspace, output_dir: OutputDir, options: &Options, console: &Console, @@ -50,11 +50,7 @@ pub fn test_mutants( debug!(?mutant_packages); let output_mutex = Mutex::new(output_dir); - let baseline_build_dir = if options.in_place { - BuildDir::in_place(workspace_dir)? - } else { - BuildDir::copy_from(workspace_dir, options.gitignore, options.leak_dirs, console)? - }; + let baseline_build_dir = BuildDir::for_baseline(workspace, options, console)?; let jobserver = &options .jobserver @@ -109,13 +105,7 @@ pub fn test_mutants( let build_dir = &if let Some(d) = build_dir_0 { d } else { - debug!("copy build dir"); - BuildDir::copy_from( - workspace_dir, - options.gitignore, - options.leak_dirs, - console, - )? + BuildDir::copy_from(workspace.root(), options, console)? }; let worker = Worker { build_dir, diff --git a/src/main.rs b/src/main.rs index fb3bde60..ea3387d5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -483,7 +483,7 @@ fn main() -> Result<()> { output_dir.write_previously_caught(&previously_caught)?; } console.set_debug_log(output_dir.open_debug_log()?); - let lab_outcome = test_mutants(mutants, workspace.root(), output_dir, &options, &console)?; + let lab_outcome = test_mutants(mutants, &workspace, output_dir, &options, &console)?; exit(lab_outcome.exit_code()); } Ok(()) From 993d38cd1224c4990530d02c899f6d07c546393e Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 07:37:16 -0800 Subject: [PATCH 13/37] clippy: pedantic cleanups for BuildDir --- src/build_dir.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/build_dir.rs b/src/build_dir.rs index 5f37ef48..7cff8b71 100644 --- a/src/build_dir.rs +++ b/src/build_dir.rs @@ -2,20 +2,29 @@ //! A directory containing mutated source to run cargo builds and tests. -use std::fmt::{self, Debug}; +#![warn(clippy::pedantic)] + use std::fs::write; +use anyhow::{ensure, Context}; +use camino::{Utf8Path, Utf8PathBuf}; use tempfile::TempDir; use tracing::info; -use crate::copy_tree::copy_tree; -use crate::manifest::fix_cargo_config; -use crate::*; +use crate::{ + console::Console, + copy_tree::copy_tree, + manifest::{fix_cargo_config, fix_manifest}, + options::Options, + workspace::Workspace, + Result, +}; /// A directory containing source, that can be mutated, built, and tested. /// /// Depending on how its constructed, this might be a copy in a tempdir /// or the original source directory. +#[derive(Debug)] pub struct BuildDir { /// The path of the root of the build directory. path: Utf8PathBuf, @@ -25,14 +34,6 @@ pub struct BuildDir { temp_dir: Option, } -impl Debug for BuildDir { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("BuildDir") - .field("path", &self.path) - .finish() - } -} - impl BuildDir { /// Make the build dir for the baseline. /// @@ -71,7 +72,7 @@ impl BuildDir { } else { Some(temp_dir) }; - let build_dir = BuildDir { temp_dir, path }; + let build_dir = BuildDir { path, temp_dir }; Ok(build_dir) } @@ -81,8 +82,7 @@ impl BuildDir { temp_dir: None, path: source_path .canonicalize_utf8() - .context("canonicalize source path")? - .to_owned(), + .context("canonicalize source path")?, }) } @@ -101,7 +101,7 @@ impl BuildDir { #[cfg(test)] mod test { - use test_util::copy_of_testdata; + use crate::test_util::copy_of_testdata; use super::*; From 49e1bdeb2975111d1ca7bac3c0e207884e07af12 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 07:44:13 -0800 Subject: [PATCH 14/37] Tidy up --- src/lab.rs | 10 +++++----- src/main.rs | 1 - 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 70352ca2..d422fe55 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -46,8 +46,6 @@ pub fn test_mutants( warn!("No mutants found under the active filters"); return Ok(LabOutcome::default()); } - let mutant_packages = mutants.iter().map(Mutant::package).unique().collect_vec(); // hold - debug!(?mutant_packages); let output_mutex = Mutex::new(output_dir); let baseline_build_dir = BuildDir::for_baseline(workspace, options, console)?; @@ -63,6 +61,8 @@ pub fn test_mutants( .context("Start jobserver")?; let timeouts = match options.baseline { BaselineStrategy::Run => { + let all_mutated_packages = mutants.iter().map(Mutant::package).unique().collect_vec(); // hold + debug!(?all_mutated_packages); let outcome = Worker { build_dir: &baseline_build_dir, output_mutex: &output_mutex, @@ -71,13 +71,13 @@ pub fn test_mutants( options, console, } - .run_one_scenario(&Scenario::Baseline, Some(&mutant_packages))?; + .run_one_scenario(&Scenario::Baseline, Some(&all_mutated_packages))?; if outcome.success() { Timeouts::from_baseline(&outcome, options) } else { error!( - "cargo {} failed in an unmutated tree, so no mutants were tested", - outcome.last_phase(), + "cargo {phase} failed in an unmutated tree, so no mutants were tested", + phase = outcome.last_phase(), ); return Ok(output_mutex .into_inner() diff --git a/src/main.rs b/src/main.rs index ea3387d5..8fe44981 100644 --- a/src/main.rs +++ b/src/main.rs @@ -58,7 +58,6 @@ use crate::in_diff::diff_filter; use crate::interrupt::check_interrupted; use crate::lab::test_mutants; use crate::list::{list_files, list_mutants, FmtToIoWrite}; -use crate::manifest::fix_manifest; use crate::mutate::{Genre, Mutant}; use crate::options::{Colors, Options, TestTool}; use crate::outcome::{Phase, ScenarioOutcome}; From 86d504b7bdcb86ac62f6d6b70024d3976183f3e6 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 08:07:57 -0800 Subject: [PATCH 15/37] Factor out Lab struct --- src/lab.rs | 96 ++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 71 insertions(+), 25 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index d422fe55..e414cc66 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -49,8 +49,7 @@ pub fn test_mutants( let output_mutex = Mutex::new(output_dir); let baseline_build_dir = BuildDir::for_baseline(workspace, options, console)?; - - let jobserver = &options + let jobserver = options .jobserver .then(|| { let n_tasks = options.jobserver_tasks.unwrap_or_else(num_cpus::get); @@ -59,19 +58,16 @@ pub fn test_mutants( }) .transpose() .context("Start jobserver")?; + let lab = Lab { + workspace, + output_mutex, + jobserver, + options, + console, + }; let timeouts = match options.baseline { BaselineStrategy::Run => { - let all_mutated_packages = mutants.iter().map(Mutant::package).unique().collect_vec(); // hold - debug!(?all_mutated_packages); - let outcome = Worker { - build_dir: &baseline_build_dir, - output_mutex: &output_mutex, - jobserver, - timeouts: Timeouts::for_baseline(options), - options, - console, - } - .run_one_scenario(&Scenario::Baseline, Some(&all_mutated_packages))?; + let outcome = lab.run_baseline(&baseline_build_dir, &mutants)?; if outcome.success() { Timeouts::from_baseline(&outcome, options) } else { @@ -79,7 +75,8 @@ pub fn test_mutants( "cargo {phase} failed in an unmutated tree, so no mutants were tested", phase = outcome.last_phase(), ); - return Ok(output_mutex + return Ok(lab + .output_mutex .into_inner() .expect("lock output_dir") .take_lab_outcome()); @@ -100,22 +97,15 @@ pub fn test_mutants( for _i_thread in 0..n_threads { threads.push(scope.spawn(|| -> crate::Result<()> { trace!(thread_id = ?thread::current().id(), "start thread"); - // First thread to start can use the initial build dir; others need to copy a new one + // First thread to start can use the baseline's build dir; + // others need to copy a new one let build_dir_0 = build_dir_0.lock().expect("lock build dir 0").take(); // separate for lock let build_dir = &if let Some(d) = build_dir_0 { d } else { BuildDir::copy_from(workspace.root(), options, console)? }; - let worker = Worker { - build_dir, - output_mutex: &output_mutex, - jobserver, - timeouts, - options, - console, - }; - worker.run_queue(work_queue) + lab.run_queue(build_dir, timeouts, work_queue) })); } // The errors potentially returned from `join` are a special `std::thread::Result` @@ -146,7 +136,8 @@ pub fn test_mutants( } })?; - let output_dir = output_mutex + let output_dir = lab + .output_mutex .into_inner() .expect("final unlock mutants queue"); console.lab_finished(&output_dir.lab_outcome, start_time, options); @@ -161,6 +152,61 @@ pub fn test_mutants( Ok(lab_outcome) } +/// Common context across all scenarios, threads, and build dirs. +struct Lab<'a> { + #[allow(unused)] // needed later to find packages + workspace: &'a Workspace, + output_mutex: Mutex, + jobserver: Option, + options: &'a Options, + console: &'a Console, +} + +impl Lab<'_> { + /// Run the baseline scenario, which is the same as running `cargo test` on the unmutated + /// tree. + /// + /// If it fails, return None, indicating that no further testing should be done. + /// + /// If it succeeds, return the timeouts to be used for the other scenarios. + fn run_baseline( + &self, + baseline_build_dir: &BuildDir, + mutants: &[Mutant], + ) -> Result { + let all_mutated_packages = mutants.iter().map(Mutant::package).unique().collect_vec(); + Worker { + build_dir: baseline_build_dir, + output_mutex: &self.output_mutex, + jobserver: &self.jobserver, + timeouts: Timeouts::for_baseline(self.options), + options: self.options, + console: self.console, + } + .run_one_scenario(&Scenario::Baseline, Some(&all_mutated_packages)) + } + + /// Run until the input queue is empty. + /// + /// The queue, inside a mutex, can be consumed by multiple threads. + fn run_queue( + &self, + build_dir: &BuildDir, + timeouts: Timeouts, + work_queue: &Mutex>, + ) -> Result<()> { + Worker { + build_dir, + output_mutex: &self.output_mutex, + jobserver: &self.jobserver, + timeouts, + options: self.options, + console: self.console, + } + .run_queue(work_queue) + } +} + /// A worker owns one build directory and runs a single thread of testing. /// /// It consumes jobs from an input queue and runs them until the queue is empty, From 6821d51940ed39c37065c441e92a948b140926e5 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 6 Nov 2024 11:59:41 -0800 Subject: [PATCH 16/37] Comment --- src/workspace.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/workspace.rs b/src/workspace.rs index 9a4b4196..c573fc31 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -101,9 +101,10 @@ pub struct Workspace { impl fmt::Debug for Workspace { #[mutants::skip] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // The `metadata` value is very large so is omitted here; + // just the root is enough. f.debug_struct("Workspace") .field("root", &self.root().to_string()) - // .field("metadata", &self.metadata) .finish() } } From c21e9315cd6b32de7ef87c0e98f2dbbaed160471 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 8 Nov 2024 18:54:12 -0800 Subject: [PATCH 17/37] Store just the package name in SourceFile --- src/cargo.rs | 105 ++++++++++++++++++++++++--------------------- src/lab.rs | 20 +++++---- src/list.rs | 2 +- src/mutate.rs | 14 +----- src/source.rs | 19 +++----- src/visit.rs | 8 +--- src/workspace.rs | 4 +- tests/main.rs | 4 +- tests/workspace.rs | 4 +- 9 files changed, 84 insertions(+), 96 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index 3ea434ab..ec8cd8c1 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -10,7 +10,6 @@ use tracing::{debug, debug_span, warn}; use crate::outcome::PhaseResult; use crate::output::ScenarioOutput; -use crate::package::Package; use crate::process::{Process, ProcessStatus}; use crate::*; @@ -19,7 +18,7 @@ use crate::*; pub fn run_cargo( build_dir: &BuildDir, jobserver: &Option, - packages: Option<&[&Package]>, + package_names: Option<&[&str]>, phase: Phase, timeout: Option, scenario_output: &mut ScenarioOutput, @@ -28,7 +27,7 @@ pub fn run_cargo( ) -> Result { let _span = debug_span!("run", ?phase).entered(); let start = Instant::now(); - let argv = cargo_argv(build_dir.path(), packages, phase, options); + let argv = cargo_argv(build_dir.path(), package_names, phase, options); let mut env = vec![ // The tests might use Insta , and we don't want it to write // updates to the source tree, and we *certainly* don't want it to write @@ -80,8 +79,8 @@ pub fn cargo_bin() -> String { /// cargo binary itself. // (This is split out so it's easier to test.) fn cargo_argv( - build_dir: &Utf8Path, - packages: Option<&[&Package]>, + _build_dir: &Utf8Path, + package_names: Option<&[&str]>, phase: Phase, options: &Options, ) -> Vec { @@ -127,15 +126,19 @@ fn cargo_argv( } } cargo_args.push("--verbose".to_string()); - if let Some([package]) = packages { - // Use the unambiguous form for this case; it works better when the same - // package occurs multiple times in the tree with different versions? - cargo_args.push("--manifest-path".to_owned()); - cargo_args.push(build_dir.join(&package.relative_manifest_path).to_string()); - } else if let Some(packages) = packages { - for package in packages.iter().map(|p| p.name.to_owned()).sorted() { + // TODO: If there's just one package then look up its manifest path in the + // workspace and use that instead, because it's less ambiguous when there's + // multiple different-version packages with the same name in the workspace. + // (A rare case, but it happens in itertools.) + // if let Some([package]) = package_names { + // // Use the unambiguous form for this case; it works better when the same + // // package occurs multiple times in the tree with different versions? + // cargo_args.push("--manifest-path".to_owned()); + // cargo_args.push(build_dir.join(&package.relative_manifest_path).to_string()); + if let Some(packages) = package_names { + for package in packages.iter().sorted() { cargo_args.push("--package".to_owned()); - cargo_args.push(package); + cargo_args.push(package.to_string()); } } else { cargo_args.push("--workspace".to_string()); @@ -200,8 +203,6 @@ fn encoded_rustflags(options: &Options) -> Option { #[cfg(test)] mod test { - use std::sync::Arc; - use pretty_assertions::assert_eq; use rusty_fork::rusty_fork_test; @@ -230,46 +231,50 @@ mod test { let mut options = Options::default(); let package_name = "cargo-mutants-testdata-something"; let build_dir = Utf8Path::new("/tmp/buildXYZ"); - let relative_manifest_path = Utf8PathBuf::from("testdata/something/Cargo.toml"); + // 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())); - let package = Arc::new(Package { - name: package_name.to_owned(), - relative_manifest_path: relative_manifest_path.clone(), - }); - let build_manifest_path = build_dir.join(relative_manifest_path); - assert_eq!( - cargo_argv(build_dir, Some(&[&package]), Phase::Check, &options)[1..], - [ - "check", - "--tests", - "--verbose", - "--manifest-path", - build_manifest_path.as_str(), - ] - ); - assert_eq!( - cargo_argv(build_dir, Some(&[&package]), Phase::Build, &options)[1..], - [ - "test", - "--no-run", - "--verbose", - "--manifest-path", - build_manifest_path.as_str(), - ] - ); + // TODO: It wolud be a bit better to use `--manifest-path` here, to get + // the fix for + // but it's temporarily regressed. assert_eq!( - cargo_argv(build_dir, Some(&[&package]), Phase::Test, &options)[1..], - [ - "test", - "--verbose", - "--manifest-path", - build_manifest_path.as_str(), - "--lib", - "--no-fail-fast" - ] + cargo_argv(build_dir, Some(&[package_name]), Phase::Check, &options)[1..], + ["check", "--tests", "--verbose", "--package", package_name] ); + + // let build_manifest_path = build_dir.join(relative_manifest_path); + // assert_eq!( + // cargo_argv(build_dir, Some(&[package_name]), Phase::Check, &options)[1..], + // [ + // "check", + // "--tests", + // "--verbose", + // "--manifest-path", + // build_manifest_path.as_str(), + // ] + // ); + // assert_eq!( + // cargo_argv(build_dir, Some(&[package_name]), Phase::Build, &options)[1..], + // [ + // "test", + // "--no-run", + // "--verbose", + // "--manifest-path", + // build_manifest_path.as_str(), + // ] + // ); + // assert_eq!( + // cargo_argv(build_dir, Some(&[package_name]), Phase::Test, &options)[1..], + // [ + // "test", + // "--verbose", + // "--manifest-path", + // build_manifest_path.as_str(), + // "--lib", + // "--no-fail-fast" + // ] + // ); } #[test] diff --git a/src/lab.rs b/src/lab.rs index e414cc66..22a60367 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -16,8 +16,8 @@ use tracing::{debug, debug_span, error, trace, warn}; use crate::{ cargo::run_cargo, options::TestPackages, outcome::LabOutcome, output::OutputDir, - package::Package, timeouts::Timeouts, workspace::Workspace, BaselineStrategy, BuildDir, - Console, Context, Mutant, Options, Phase, Result, Scenario, ScenarioOutcome, + timeouts::Timeouts, workspace::Workspace, BaselineStrategy, BuildDir, Console, Context, Mutant, + Options, Phase, Result, Scenario, ScenarioOutcome, }; /// Run all possible mutation experiments. @@ -174,7 +174,11 @@ impl Lab<'_> { baseline_build_dir: &BuildDir, mutants: &[Mutant], ) -> Result { - let all_mutated_packages = mutants.iter().map(Mutant::package).unique().collect_vec(); + let all_mutated_packages = mutants + .iter() + .map(|m| m.source_file.package_name.as_str()) + .unique() + .collect_vec(); Worker { build_dir: baseline_build_dir, output_mutex: &self.output_mutex, @@ -231,12 +235,12 @@ impl Worker<'_> { let next_mutant = work_queue.lock().expect("Lock pending work queue").next(); // separate for lock if let Some(mutant) = next_mutant { let _span = debug_span!("mutant", name = mutant.name(false, false)).entered(); - let package = mutant.package().clone(); // hold - let packages = [&package]; // hold + let package_name = mutant.source_file.package_name.clone(); // hold let scenario = Scenario::Mutant(mutant); - let test_packages: Option<&[&Package]> = match &self.options.test_packages { + let local_packages: &[&str] = &[&package_name]; // hold + let test_packages: Option<&[&str]> = match &self.options.test_packages { TestPackages::Workspace => None, - TestPackages::Mutated => Some(&packages), + TestPackages::Mutated => Some(local_packages), TestPackages::Named(_named) => { unimplemented!("get packages by name") } @@ -251,7 +255,7 @@ impl Worker<'_> { fn run_one_scenario( &mut self, scenario: &Scenario, - test_packages: Option<&[&Package]>, + test_packages: Option<&[&str]>, ) -> Result { let mut scenario_output = self .output_mutex diff --git a/src/list.rs b/src/list.rs index bf1a6397..b2a53692 100644 --- a/src/list.rs +++ b/src/list.rs @@ -71,7 +71,7 @@ pub(crate) fn list_files( .map(|source_file| { json!({ "path": source_file.tree_relative_path.to_slash_path(), - "package": source_file.package.name, + "package": source_file.package_name, }) }) .collect(), diff --git a/src/mutate.rs b/src/mutate.rs index f9a5d532..213f824e 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -14,7 +14,6 @@ use tracing::trace; use crate::build_dir::BuildDir; use crate::output::clean_filename; -use crate::package::Package; use crate::source::SourceFile; use crate::span::Span; use crate::MUTATION_MARKER_COMMENT; @@ -160,15 +159,6 @@ impl Mutant { self.replacement.as_str() } - /// Return the cargo package name. - pub fn package_name(&self) -> &str { - &self.source_file.package.name - } - - pub fn package(&self) -> &Package { - &self.source_file.package - } - /// Return a unified diff for the mutant. /// /// The mutated text must be passed in because we should have already computed @@ -220,7 +210,7 @@ impl fmt::Debug for Mutant { .field("replacement", &self.replacement) .field("genre", &self.genre) .field("span", &self.span) - .field("package_name", &self.package_name()) + .field("package_name", &self.source_file.package_name) .finish() } } @@ -232,7 +222,7 @@ impl Serialize for Mutant { { // custom serialize to omit inessential info let mut ss = serializer.serialize_struct("Mutant", 7)?; - ss.serialize_field("package", &self.package_name())?; + ss.serialize_field("package", &self.source_file.package_name)?; ss.serialize_field("file", &self.source_file.tree_relative_slashes())?; ss.serialize_field("function", &self.function.as_ref().map(|a| a.as_ref()))?; ss.serialize_field("span", &self.span)?; diff --git a/src/source.rs b/src/source.rs index ecfc1f6f..b441e428 100644 --- a/src/source.rs +++ b/src/source.rs @@ -9,7 +9,6 @@ use camino::{Utf8Path, Utf8PathBuf}; #[allow(unused_imports)] use tracing::{debug, info, warn}; -use crate::package::Package; use crate::path::{ascent, Utf8PathSlashes}; use crate::span::LineColumn; @@ -22,8 +21,8 @@ use crate::span::LineColumn; /// files are written with Unix line endings. #[derive(Clone, Debug, Eq, PartialEq)] pub struct SourceFile { - /// Package within the workspace. - pub package: Arc, + /// Which package within the workspace contains this file? + pub package_name: String, /// Path of this source file relative to workspace. pub tree_relative_path: Utf8PathBuf, @@ -46,7 +45,7 @@ impl SourceFile { pub fn new( tree_path: &Utf8Path, tree_relative_path: Utf8PathBuf, - package: &Arc, + package_name: &str, is_top: bool, ) -> Result> { if ascent(&tree_relative_path) > 0 { @@ -65,7 +64,7 @@ impl SourceFile { Ok(Some(SourceFile { tree_relative_path, code, - package: Arc::clone(package), + package_name: package_name.to_owned(), is_top, })) } @@ -113,10 +112,7 @@ mod test { let source_file = SourceFile::new( temp_dir_path, file_name.parse().unwrap(), - &Arc::new(Package { - name: "imaginary-package".to_owned(), - relative_manifest_path: "whatever/Cargo.toml".into(), - }), + "imaginary-package", true, ) .unwrap() @@ -129,10 +125,7 @@ mod test { let source_file = SourceFile::new( &Utf8PathBuf::from("unimportant"), "../outside_workspace.rs".parse().unwrap(), - &Arc::new(Package { - name: "imaginary-package".to_owned(), - relative_manifest_path: "whatever/Cargo.toml".into(), - }), + "imaginary-package", true, ) .unwrap(); diff --git a/src/visit.rs b/src/visit.rs index 6a14f927..4f463e37 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -80,7 +80,7 @@ pub fn walk_tree( file_queue.extend(SourceFile::new( workspace_dir, mod_path, - &source_file.package, + &source_file.package_name, false, )?) } @@ -729,7 +729,6 @@ mod test { use test_util::copy_of_testdata; use super::*; - use crate::package::Package; /// We should not generate mutants that produce the same tokens as the /// source. @@ -740,10 +739,7 @@ mod test { "}; let source_file = SourceFile { code: Arc::new(code.to_owned()), - package: Arc::new(Package { - name: "unimportant".to_owned(), - relative_manifest_path: "Cargo.toml".into(), - }), + package_name: "unimportant".to_owned(), tree_relative_path: Utf8PathBuf::from("src/lib.rs"), is_top: true, }; diff --git a/src/workspace.rs b/src/workspace.rs index c573fc31..2727f6df 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -101,7 +101,7 @@ pub struct Workspace { impl fmt::Debug for Workspace { #[mutants::skip] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - // The `metadata` value is very large so is omitted here; + // The `metadata` value is very large so is omitted here; // just the root is enough. f.debug_struct("Workspace") .field("root", &self.root().to_string()) @@ -207,7 +207,7 @@ impl Workspace { sources.extend(SourceFile::new( self.root(), source_path.to_owned(), - &package, + &package.name, true, )?); } diff --git a/tests/main.rs b/tests/main.rs index c43d904d..9637a2ae 100644 --- a/tests/main.rs +++ b/tests/main.rs @@ -446,11 +446,11 @@ fn small_well_tested_mutants_with_cargo_arg_release() { println!("{}", baseline_log_path.display()); let log_content = fs::read_to_string(baseline_log_path).unwrap(); println!("{log_content}"); - regex::Regex::new(r"cargo.* test --no-run --verbose --manifest-path .* --release") + regex::Regex::new(r"cargo.* test --no-run --verbose .* --release") .unwrap() .captures(&log_content) .unwrap(); - regex::Regex::new(r"cargo.* test --verbose --manifest-path .* --release") + regex::Regex::new(r"cargo.* test --verbose .* --release") .unwrap() .captures(&log_content) .unwrap(); diff --git a/tests/workspace.rs b/tests/workspace.rs index 77e5325e..073a867c 100644 --- a/tests/workspace.rs +++ b/tests/workspace.rs @@ -161,8 +161,8 @@ fn workspace_tree_is_well_tested() { ); assert_eq!(mutant_phases[1]["process_status"], json!({"Failure": 101})); assert_eq!( - mutant_phases[1]["argv"].as_array().unwrap()[1..=3], - ["test", "--verbose", "--manifest-path"], + mutant_phases[1]["argv"].as_array().unwrap()[1..=2], + ["test", "--verbose"], ); } { From bb3912aa3a510d2d852c2fba069a5f88717a5d6a Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 8 Nov 2024 19:06:19 -0800 Subject: [PATCH 18/37] Simplify Workspace/package a bit --- src/mutate.rs | 18 +++++++++--------- src/workspace.rs | 30 ++++++++---------------------- 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/src/mutate.rs b/src/mutate.rs index 213f824e..d07074e2 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -247,8 +247,9 @@ mod test { let workspace = Workspace::open(tmp.path()).unwrap(); let options = Options::default(); let mutants = workspace - .mutants(&PackageFilter::All, &options, &Console::new()) - .unwrap(); + .discover(&PackageFilter::All, &options, &Console::new()) + .unwrap() + .mutants; assert_eq!(mutants.len(), 5); assert_eq!( format!("{:#?}", mutants[0]), @@ -309,8 +310,9 @@ mod test { let tmp = copy_of_testdata("hang_avoided_by_attr"); let mutants = Workspace::open(tmp.path()) .unwrap() - .mutants(&PackageFilter::All, &Options::default(), &Console::new()) - .unwrap(); + .discover(&PackageFilter::All, &Options::default(), &Console::new()) + .unwrap() + .mutants; let descriptions = mutants.iter().map(Mutant::describe_change).collect_vec(); insta::assert_snapshot!( descriptions.join("\n"), @@ -328,11 +330,9 @@ mod test { fn mutate_factorial() -> Result<()> { let temp = copy_of_testdata("factorial"); let tree_path = temp.path(); - let mutants = Workspace::open(tree_path)?.mutants( - &PackageFilter::All, - &Options::default(), - &Console::new(), - )?; + let mutants = Workspace::open(tree_path)? + .discover(&PackageFilter::All, &Options::default(), &Console::new())? + .mutants; assert_eq!(mutants.len(), 5); let mutated_code = mutants[0].mutated_code(); diff --git a/src/workspace.rs b/src/workspace.rs index 2727f6df..1173d572 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -4,7 +4,6 @@ use std::fmt; use std::panic::catch_unwind; use std::path::Path; use std::process::Command; -use std::sync::Arc; use anyhow::{anyhow, bail, ensure, Context}; use camino::{Utf8Path, Utf8PathBuf}; @@ -15,7 +14,6 @@ use tracing::{debug, debug_span, error, warn}; use crate::cargo::cargo_bin; use crate::console::Console; use crate::interrupt::check_interrupted; -use crate::mutate::Mutant; use crate::options::Options; use crate::package::Package; use crate::source::SourceFile; @@ -89,7 +87,7 @@ impl PackageFilter { /// A package and the top source files within it. struct PackageTop { - package: Arc, + package: Package, top_sources: Vec, } @@ -137,12 +135,12 @@ impl Workspace { } /// Find packages to mutate, subject to some filtering. - #[allow(dead_code)] - pub fn packages(&self, package_filter: &PackageFilter) -> Result>> { + #[cfg(test)] + pub fn packages(&self, package_filter: &PackageFilter) -> Result> { Ok(self .package_tops(package_filter)? .into_iter() - .map(|pt| pt.package) + .map(|pt| pt.package.clone()) .collect()) } @@ -176,12 +174,11 @@ impl Workspace { ) })? .to_owned(); - let package = Arc::new(Package { - name: package_metadata.name.clone(), - relative_manifest_path, - }); tops.push(PackageTop { - package, + package: Package { + name: package_metadata.name.clone(), + relative_manifest_path, + }, top_sources: direct_package_sources(self.root(), package_metadata)?, }); } @@ -229,17 +226,6 @@ impl Workspace { console, ) } - - /// Return all mutants generated from this workspace. - #[allow(dead_code)] // called from tests, for now - pub fn mutants( - &self, - package_filter: &PackageFilter, - options: &Options, - console: &Console, - ) -> Result> { - Ok(self.discover(package_filter, options, console)?.mutants) - } } /// Find all the files that are named in the `path` of targets in a Cargo manifest that should be tested. From 309eba12b06a532a5818805c799faf85e26003d1 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 8 Nov 2024 19:19:32 -0800 Subject: [PATCH 19/37] Unify Package and PackageTop --- src/package.rs | 4 ++++ src/workspace.rs | 35 +++++++++++------------------------ 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/src/package.rs b/src/package.rs index 3bb30947..78878639 100644 --- a/src/package.rs +++ b/src/package.rs @@ -12,4 +12,8 @@ pub struct Package { /// For Cargo, the path of the `Cargo.toml` manifest file, relative to the top of the tree. pub relative_manifest_path: Utf8PathBuf, + + /// The top source files for this package, relative to the workspace root, + /// like `["src/lib.rs"]`. + pub top_sources: Vec, } diff --git a/src/workspace.rs b/src/workspace.rs index 1173d572..b9790345 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -85,12 +85,6 @@ impl PackageFilter { } } -/// A package and the top source files within it. -struct PackageTop { - package: Package, - top_sources: Vec, -} - /// A cargo workspace. pub struct Workspace { metadata: cargo_metadata::Metadata, @@ -137,16 +131,12 @@ impl Workspace { /// Find packages to mutate, subject to some filtering. #[cfg(test)] pub fn packages(&self, package_filter: &PackageFilter) -> Result> { - Ok(self - .package_tops(package_filter)? - .into_iter() - .map(|pt| pt.package.clone()) - .collect()) + self.package_tops(package_filter) } /// Find all the packages and their top source files. - fn package_tops(&self, package_filter: &PackageFilter) -> Result> { - let mut tops = Vec::new(); + fn package_tops(&self, package_filter: &PackageFilter) -> Result> { + let mut packages = Vec::new(); let package_filter = package_filter.resolve_auto(&self.metadata)?; for package_metadata in self .metadata @@ -174,37 +164,34 @@ impl Workspace { ) })? .to_owned(); - tops.push(PackageTop { - package: Package { - name: package_metadata.name.clone(), - relative_manifest_path, - }, + packages.push(Package { + name: package_metadata.name.clone(), + relative_manifest_path, top_sources: direct_package_sources(self.root(), package_metadata)?, }); } if let PackageFilter::Explicit(ref names) = package_filter { for wanted in names { - if !tops.iter().any(|found| found.package.name == *wanted) { + if !packages.iter().any(|package| package.name == *wanted) { warn!("package {wanted:?} not found in source tree"); } } } - Ok(tops) + Ok(packages) } /// Find all the top source files for selected packages. fn top_sources(&self, package_filter: &PackageFilter) -> Result> { let mut sources = Vec::new(); - for PackageTop { - package, - top_sources, + for Package { + name, top_sources, .. } in self.package_tops(package_filter)? { for source_path in top_sources { sources.extend(SourceFile::new( self.root(), source_path.to_owned(), - &package.name, + &name, true, )?); } From 763e2f033e81c92fd4befeb5fff1c89ef0126bd5 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 07:37:31 -0800 Subject: [PATCH 20/37] Merge Package and PackageTop --- src/workspace.rs | 64 +++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 36 deletions(-) diff --git a/src/workspace.rs b/src/workspace.rs index b9790345..2704cb1b 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -128,14 +128,8 @@ impl Workspace { Ok(Workspace { metadata }) } - /// Find packages to mutate, subject to some filtering. - #[cfg(test)] - pub fn packages(&self, package_filter: &PackageFilter) -> Result> { - self.package_tops(package_filter) - } - - /// Find all the packages and their top source files. - fn package_tops(&self, package_filter: &PackageFilter) -> Result> { + /// Find packages matching some filter. + fn packages(&self, package_filter: &PackageFilter) -> Result> { let mut packages = Vec::new(); let package_filter = package_filter.resolve_auto(&self.metadata)?; for package_metadata in self @@ -180,25 +174,6 @@ impl Workspace { Ok(packages) } - /// Find all the top source files for selected packages. - fn top_sources(&self, package_filter: &PackageFilter) -> Result> { - let mut sources = Vec::new(); - for Package { - name, top_sources, .. - } in self.package_tops(package_filter)? - { - for source_path in top_sources { - sources.extend(SourceFile::new( - self.root(), - source_path.to_owned(), - &name, - true, - )?); - } - } - Ok(sources) - } - /// Make all the mutants from the filtered packages in this workspace. pub fn discover( &self, @@ -208,13 +183,27 @@ impl Workspace { ) -> Result { walk_tree( self.root(), - &self.top_sources(package_filter)?, + &top_sources(self.root(), &self.packages(package_filter)?)?, options, console, ) } } +/// Find all the top source files for selected packages. +fn top_sources(root: &Utf8Path, packages: &[Package]) -> Result> { + let mut sources = Vec::new(); + for Package { + name, top_sources, .. + } in packages + { + for source_path in top_sources { + sources.extend(SourceFile::new(root, source_path.to_owned(), name, true)?); + } + } + Ok(sources) +} + /// Find all the files that are named in the `path` of targets in a Cargo manifest that should be tested. /// /// These are the starting points for discovering source files. @@ -314,7 +303,7 @@ mod test { use crate::console::Console; use crate::options::Options; use crate::test_util::copy_of_testdata; - use crate::workspace::PackageFilter; + use crate::workspace::{top_sources, PackageFilter}; use super::Workspace; @@ -358,12 +347,14 @@ mod test { ["cargo_mutants_testdata_workspace_utils", "main", "main2"] ); assert_eq!( - workspace - .top_sources(&PackageFilter::All) - .unwrap() - .iter() - .map(|sf| sf.tree_relative_path.clone()) - .collect_vec(), + top_sources( + workspace.root(), + workspace.packages(&PackageFilter::All).unwrap().as_slice() + ) + .unwrap() + .iter() + .map(|sf| sf.tree_relative_path.clone()) + .collect_vec(), // ordered by package name ["utils/src/lib.rs", "main/src/main.rs", "main2/src/main.rs"] ); @@ -425,7 +416,8 @@ mod test { .collect_vec(), ["main"] ); - let top_sources = workspace.top_sources(&filter).unwrap(); + let top_sources = + top_sources(workspace.root(), &workspace.packages(&filter).unwrap()).unwrap(); println!("{top_sources:#?}"); assert_eq!( top_sources From 14189de7e0d99b3effb1d2b68e910991beaad98d Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 08:00:33 -0800 Subject: [PATCH 21/37] Separate finding packages from filtering them --- src/workspace.rs | 89 ++++++++++++++++++++++++++---------------------- 1 file changed, 49 insertions(+), 40 deletions(-) diff --git a/src/workspace.rs b/src/workspace.rs index 2704cb1b..8acc93b8 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -7,6 +7,7 @@ use std::process::Command; use anyhow::{anyhow, bail, ensure, Context}; use camino::{Utf8Path, Utf8PathBuf}; +use cargo_metadata::Metadata; use itertools::Itertools; use serde_json::Value; use tracing::{debug, debug_span, error, warn}; @@ -88,6 +89,7 @@ impl PackageFilter { /// A cargo workspace. pub struct Workspace { metadata: cargo_metadata::Metadata, + packages: Vec, } impl fmt::Debug for Workspace { @@ -97,7 +99,7 @@ impl fmt::Debug for Workspace { // just the root is enough. f.debug_struct("Workspace") .field("root", &self.root().to_string()) - .finish() + .finish_non_exhaustive() } } @@ -125,50 +127,25 @@ impl Workspace { .exec() .with_context(|| format!("Failed to run cargo metadata on {:?}", manifest_path))?; debug!(workspace_root = ?metadata.workspace_root, "Found workspace root"); - Ok(Workspace { metadata }) + let packages = packages_from_metadata(&metadata)?; + Ok(Workspace { metadata, packages }) } /// Find packages matching some filter. fn packages(&self, package_filter: &PackageFilter) -> Result> { - let mut packages = Vec::new(); let package_filter = package_filter.resolve_auto(&self.metadata)?; - for package_metadata in self - .metadata - .workspace_packages() - .into_iter() - .sorted_by_key(|p| &p.name) - { - check_interrupted()?; - let name = &package_metadata.name; - let _span = debug_span!("package", %name).entered(); - if let PackageFilter::Explicit(ref include_names) = package_filter { - if !include_names.contains(name) { - continue; - } - } - let manifest_path = &package_metadata.manifest_path; - debug!(%manifest_path, "walk package"); - let relative_manifest_path = manifest_path - .strip_prefix(self.root()) - .map_err(|_| { - anyhow!( - "manifest path {manifest_path:?} for package {name:?} is not \ - within the detected source root path {dir:?}", - dir = self.root(), - ) - })? - .to_owned(); - packages.push(Package { - name: package_metadata.name.clone(), - relative_manifest_path, - top_sources: direct_package_sources(self.root(), package_metadata)?, - }); - } - if let PackageFilter::Explicit(ref names) = package_filter { - for wanted in names { - if !packages.iter().any(|package| package.name == *wanted) { - warn!("package {wanted:?} not found in source tree"); - } + let PackageFilter::Explicit(wanted) = package_filter else { + return Ok(self.packages.clone()); + }; + let packages = self + .packages + .iter() + .filter(|p| wanted.contains(&p.name)) + .cloned() + .collect_vec(); + for wanted in wanted { + if !packages.iter().any(|package| package.name == *wanted) { + warn!("package {wanted:?} not found in source tree"); } } Ok(packages) @@ -190,6 +167,38 @@ impl Workspace { } } +/// Read `cargo-metadata` parsed output, and produce our package representation. +fn packages_from_metadata(metadata: &Metadata) -> Result> { + let mut packages = Vec::new(); + let root = &metadata.workspace_root; + for package_metadata in metadata + .workspace_packages() + .into_iter() + .sorted_by_key(|p| &p.name) + { + check_interrupted()?; + let name = &package_metadata.name; + let _span = debug_span!("package", %name).entered(); + let manifest_path = &package_metadata.manifest_path; + debug!(%manifest_path, "walk package"); + let relative_manifest_path = manifest_path + .strip_prefix(root) + .map_err(|_| { + // TODO: Maybe just warn and skip? + anyhow!( + "manifest path {manifest_path:?} for package {name:?} is not within the detected source root path {root:?}", + ) + })? + .to_owned(); + packages.push(Package { + name: package_metadata.name.clone(), + relative_manifest_path, + top_sources: direct_package_sources(root, package_metadata)?, + }); + } + Ok(packages) +} + /// Find all the top source files for selected packages. fn top_sources(root: &Utf8Path, packages: &[Package]) -> Result> { let mut sources = Vec::new(); From 44992bb3ae98d0c5882efd88c0ee403c123beba2 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 08:30:01 -0800 Subject: [PATCH 22/37] Clean up auto package selection Rather than having a PackageFilter which is sometimes allowed to be Auto, map it to a separate PackageSelection when resolved. Type safety! --- src/workspace.rs | 141 +++++++++++++++++++++++++++++++---------------- 1 file changed, 92 insertions(+), 49 deletions(-) diff --git a/src/workspace.rs b/src/workspace.rs index 8acc93b8..286a36c4 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -1,5 +1,18 @@ // Copyright 2023-2024 Martin Pool +//! Understand cargo workspaces, which can contain multiple packages. +//! +//! In cargo-mutants there are a few important connections to workspaces: +//! +//! 1. We copy the whole workspace to scratch directories, so need to find the root. +//! +//! 2. We can select to mutate, or run tests from, all packages in the workspace, +//! or just some, so we need to find the packages. Also, mutants are marked with the +//! package they come from. +//! +//! 3. In particular when selecting packages, we attempt to match cargo's own heuristics +//! when invoked inside a workspace. + use std::fmt; use std::panic::catch_unwind; use std::path::Path; @@ -28,7 +41,9 @@ pub enum PackageFilter { All, /// Packages with given names, from `--package`. Explicit(Vec), - /// Automatic behavior when invoked from a subdirectory, as per + /// Automatic behavior when invoked from a subdirectory. + /// + /// This tries to match /// . /// /// If the directory is within a package directory, select that package. @@ -39,49 +54,76 @@ pub enum PackageFilter { Auto(Utf8PathBuf), } +/// A more specific view of which packages to mutate, after resolving `PackageFilter::Auto`. +#[derive(Debug, Clone)] +enum PackageSelection { + All, + Explicit(Vec), +} + impl PackageFilter { + /// Convenience constructor for `PackageFilter::Explicit`. pub fn explicit>(names: I) -> PackageFilter { PackageFilter::Explicit(names.into_iter().map(|s| s.to_string()).collect_vec()) } /// Translate an auto package filter to either All or Explicit. - pub fn resolve_auto(&self, metadata: &cargo_metadata::Metadata) -> Result { - if let PackageFilter::Auto(dir) = &self { - let package_dir = locate_project(dir, false)?; - assert!(package_dir.is_absolute()); - let workspace_dir = &metadata.workspace_root; - // It's not required that the members be inside the workspace directory: see - // - for package in metadata.workspace_packages() { - if package.manifest_path.parent().expect("remove Cargo.toml") == package_dir { - debug!("resolved auto package filter to {:?}", package.name); - return Ok(PackageFilter::explicit([&package.name])); + fn resolve_auto(&self, metadata: &cargo_metadata::Metadata) -> Result { + match &self { + PackageFilter::Auto(dir) => { + // Find the closest package directory (with a cargo manifest) to the current directory. + let package_dir = locate_project(dir, false)?; + assert!(package_dir.is_absolute()); + // It's not required that the members be inside the workspace directory: see + // + for package in metadata.workspace_packages() { + // If this package is one of the workspace members, then select this package. + if package.manifest_path.parent().expect("remove Cargo.toml") == package_dir { + debug!("resolved auto package filter to {:?}", package.name); + return Ok(PackageSelection::Explicit(vec![package.name.to_owned()])); + } } + // Otherwise, we're in a virtual workspace directory, and not inside any package. + // Use configured defaults if there are any, otherwise test all packages. + let workspace_dir = &metadata.workspace_root; + ensure!( + &package_dir == workspace_dir, + "package {package_dir:?} doesn't match any child and doesn't match the workspace root {workspace_dir:?}?", + ); + Ok(workspace_default_packages(metadata)) } - // Presumably our manifest is the workspace root manifest and there is no - // top-level package? - ensure!( - &package_dir == workspace_dir, - "package {package_dir:?} doesn't match any child and doesn't match the workspace root {workspace_dir:?}?", - ); - // `workspace_default_packages` will panic when calling Cargo older than 1.71; - // in that case we'll just fall back to everything, for lack of a better option. - match catch_unwind(|| metadata.workspace_default_packages()) { - Ok(dm) if dm.is_empty() => Ok(PackageFilter::All), - Ok(dm) => Ok(PackageFilter::explicit( - dm.into_iter().map(|pmeta| &pmeta.name), - )), - Err(err) => { - warn!( - cargo_metadata_error = - err.downcast::().expect("panic message is a string"), - "workspace_default_packages is not supported; testing all packages", - ); - Ok(PackageFilter::All) - } + PackageFilter::All => Ok(PackageSelection::All), + PackageFilter::Explicit(names) => Ok(PackageSelection::Explicit(names.clone())), + } + } +} + +/// Return the default workspace packages. +/// +/// Default packages can be specified in the workspace's `Cargo.toml` file; +/// if not, all packages are included. +fn workspace_default_packages(metadata: &Metadata) -> PackageSelection { + // `cargo_metadata::workspace_default_packages` will panic when calling Cargo older than 1.71; + // in that case we'll just fall back to everything, for lack of a better option. + match catch_unwind(|| metadata.workspace_default_packages()) { + Ok(default_packages) => { + if default_packages.is_empty() { + PackageSelection::All + } else { + PackageSelection::Explicit( + default_packages + .into_iter() + .map(|pmeta| pmeta.name.clone()) + .collect(), + ) } - } else { - Ok(self.clone()) + } + Err(err) => { + warn!( + cargo_metadata_error = err.downcast::().unwrap_or_default(), + "workspace_default_packages is not supported; testing all packages", + ); + PackageSelection::All } } } @@ -133,22 +175,23 @@ impl Workspace { /// Find packages matching some filter. fn packages(&self, package_filter: &PackageFilter) -> Result> { - let package_filter = package_filter.resolve_auto(&self.metadata)?; - let PackageFilter::Explicit(wanted) = package_filter else { - return Ok(self.packages.clone()); - }; - let packages = self - .packages - .iter() - .filter(|p| wanted.contains(&p.name)) - .cloned() - .collect_vec(); - for wanted in wanted { - if !packages.iter().any(|package| package.name == *wanted) { - warn!("package {wanted:?} not found in source tree"); + match package_filter.resolve_auto(&self.metadata)? { + PackageSelection::Explicit(wanted) => { + let packages = self + .packages + .iter() + .filter(|p| wanted.contains(&p.name)) + .cloned() + .collect_vec(); + for wanted in wanted { + if !packages.iter().any(|package| package.name == *wanted) { + warn!("package {wanted:?} not found in source tree"); + } + } + Ok(packages) } + PackageSelection::All => Ok(self.packages.iter().cloned().collect_vec()), } - Ok(packages) } /// Make all the mutants from the filtered packages in this workspace. From fab5aabb16ba0031889c32d22288e43b20b8b9fb Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 08:34:27 -0800 Subject: [PATCH 23/37] clippy cleanups --- src/workspace.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/workspace.rs b/src/workspace.rs index 286a36c4..0c486866 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -13,6 +13,8 @@ //! 3. In particular when selecting packages, we attempt to match cargo's own heuristics //! when invoked inside a workspace. +#![warn(clippy::pedantic)] + use std::fmt; use std::panic::catch_unwind; use std::path::Path; @@ -80,7 +82,7 @@ impl PackageFilter { // If this package is one of the workspace members, then select this package. if package.manifest_path.parent().expect("remove Cargo.toml") == package_dir { debug!("resolved auto package filter to {:?}", package.name); - return Ok(PackageSelection::Explicit(vec![package.name.to_owned()])); + return Ok(PackageSelection::Explicit(vec![package.name.clone()])); } } // Otherwise, we're in a virtual workspace directory, and not inside any package. @@ -167,7 +169,7 @@ impl Workspace { .manifest_path(&manifest_path) .verbose(false) .exec() - .with_context(|| format!("Failed to run cargo metadata on {:?}", manifest_path))?; + .with_context(|| format!("Failed to run cargo metadata on {manifest_path}"))?; debug!(workspace_root = ?metadata.workspace_root, "Found workspace root"); let packages = packages_from_metadata(&metadata)?; Ok(Workspace { metadata, packages }) @@ -236,7 +238,7 @@ fn packages_from_metadata(metadata: &Metadata) -> Result> { packages.push(Package { name: package_metadata.name.clone(), relative_manifest_path, - top_sources: direct_package_sources(root, package_metadata)?, + top_sources: package_top_sources(root, package_metadata), }); } Ok(packages) @@ -256,13 +258,14 @@ fn top_sources(root: &Utf8Path, packages: &[Package]) -> Result> Ok(sources) } -/// Find all the files that are named in the `path` of targets in a Cargo manifest that should be tested. +/// Find all the files that are named in the `path` of targets in a +/// Cargo manifest, if the kind of the target is one that we should mutate. /// /// These are the starting points for discovering source files. -fn direct_package_sources( +fn package_top_sources( workspace_root: &Utf8Path, package_metadata: &cargo_metadata::Package, -) -> Result> { +) -> Vec { let mut found = Vec::new(); let pkg_dir = package_metadata.manifest_path.parent().unwrap(); for target in &package_metadata.targets { @@ -289,11 +292,11 @@ fn direct_package_sources( } found.sort(); found.dedup(); - Ok(found) + found } fn should_mutate_target(target: &cargo_metadata::Target) -> bool { - for kind in target.kind.iter() { + for kind in &target.kind { if kind == "bin" || kind == "proc-macro" || kind.ends_with("lib") { return true; } @@ -429,7 +432,7 @@ mod test { let subdir_path = Utf8PathBuf::try_from(tmp.path().join("main")).unwrap(); let workspace = Workspace::open(&subdir_path).expect("Find workspace root"); let packages = workspace - .packages(&PackageFilter::Auto(subdir_path.to_owned())) + .packages(&PackageFilter::Auto(subdir_path.clone())) .unwrap(); assert_eq!(packages.iter().map(|p| &p.name).collect_vec(), ["main"]); } From 888dd792ed2100aee24870dd924251b9b83e40e4 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 08:45:38 -0800 Subject: [PATCH 24/37] Run tests from named packages The second part of #394 --- src/lab.rs | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 22a60367..4d3866cc 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -227,28 +227,32 @@ struct Worker<'a> { impl Worker<'_> { /// Run until the input queue is empty. fn run_queue(mut self, work_queue: &Mutex>) -> Result<()> { - let _thread_span = - debug_span!("worker thread", build_dir = ?self.build_dir.path()).entered(); + let _span = debug_span!("worker thread", build_dir = ?self.build_dir.path()).entered(); loop { // Extract the mutant in a separate statement so that we don't hold the // lock while testing it. - let next_mutant = work_queue.lock().expect("Lock pending work queue").next(); // separate for lock - if let Some(mutant) = next_mutant { - let _span = debug_span!("mutant", name = mutant.name(false, false)).entered(); - let package_name = mutant.source_file.package_name.clone(); // hold - let scenario = Scenario::Mutant(mutant); - let local_packages: &[&str] = &[&package_name]; // hold - let test_packages: Option<&[&str]> = match &self.options.test_packages { - TestPackages::Workspace => None, - TestPackages::Mutated => Some(local_packages), - TestPackages::Named(_named) => { - unimplemented!("get packages by name") - } - }; - self.run_one_scenario(&scenario, test_packages)?; - } else { + let Some(mutant) = work_queue.lock().expect("Lock pending work queue").next() else { return Ok(()); - } + }; + let _span = debug_span!("mutant", name = mutant.name(false, false)).entered(); + // variables held here for lifetime + let package_name = mutant.source_file.package_name.clone(); + let scenario = Scenario::Mutant(mutant); + let local_packages: [&str; 1]; + let named_packages; + let test_packages: Option<&[&str]> = match &self.options.test_packages { + TestPackages::Workspace => None, + TestPackages::Mutated => { + local_packages = [&package_name]; + Some(&local_packages) + } + TestPackages::Named(named) => { + named_packages = named.iter().map(String::as_str).collect_vec(); + Some(&named_packages) + } + }; + debug!(?test_packages); + self.run_one_scenario(&scenario, test_packages)?; } } From 5375fc8cab4f85151989742e2b7fd309258e6c12 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 09:14:22 -0800 Subject: [PATCH 25/37] Use PackageSelection more widely Clearer than an Option<&[&str]> --- src/cargo.rs | 67 +++++++++++++++++++++++++++++++----------------- src/lab.rs | 37 ++++++++++++-------------- src/package.rs | 16 +++++++++++- src/workspace.rs | 9 +------ 4 files changed, 76 insertions(+), 53 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index ec8cd8c1..3c01fa62 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -2,23 +2,30 @@ //! Run Cargo as a subprocess, including timeouts and propagating signals. +use std::env; use std::iter::once; use std::time::{Duration, Instant}; +use camino::Utf8Path; use itertools::Itertools; use tracing::{debug, debug_span, warn}; -use crate::outcome::PhaseResult; +use crate::build_dir::BuildDir; +use crate::console::Console; +use crate::interrupt::check_interrupted; +use crate::options::{Options, TestTool}; +use crate::outcome::{Phase, PhaseResult}; use crate::output::ScenarioOutput; +use crate::package::PackageSelection; use crate::process::{Process, ProcessStatus}; -use crate::*; +use crate::Result; /// Run cargo build, check, or test. #[allow(clippy::too_many_arguments)] // I agree it's a lot but I'm not sure wrapping in a struct would be better. pub fn run_cargo( build_dir: &BuildDir, jobserver: &Option, - package_names: Option<&[&str]>, + packages: &PackageSelection, phase: Phase, timeout: Option, scenario_output: &mut ScenarioOutput, @@ -27,7 +34,7 @@ pub fn run_cargo( ) -> Result { let _span = debug_span!("run", ?phase).entered(); let start = Instant::now(); - let argv = cargo_argv(build_dir.path(), package_names, phase, options); + let argv = cargo_argv(build_dir.path(), packages, phase, options); let mut env = vec![ // The tests might use Insta , and we don't want it to write // updates to the source tree, and we *certainly* don't want it to write @@ -80,7 +87,7 @@ pub fn cargo_bin() -> String { // (This is split out so it's easier to test.) fn cargo_argv( _build_dir: &Utf8Path, - package_names: Option<&[&str]>, + packages: &PackageSelection, phase: Phase, options: &Options, ) -> Vec { @@ -135,13 +142,16 @@ fn cargo_argv( // // package occurs multiple times in the tree with different versions? // cargo_args.push("--manifest-path".to_owned()); // cargo_args.push(build_dir.join(&package.relative_manifest_path).to_string()); - if let Some(packages) = package_names { - for package in packages.iter().sorted() { - cargo_args.push("--package".to_owned()); - cargo_args.push(package.to_string()); + match packages { + PackageSelection::All => { + cargo_args.push("--workspace".to_string()); + } + PackageSelection::Explicit(package_names) => { + for package in package_names.iter().sorted() { + cargo_args.push("--package".to_owned()); + cargo_args.push(package.to_string()); + } } - } else { - cargo_args.push("--workspace".to_string()); } let features = &options.features; if features.no_default_features { @@ -150,6 +160,7 @@ fn cargo_argv( if features.all_features { cargo_args.push("--all-features".to_owned()); } + // N.B. it can make sense to have --all-features and also explicit features from non-default packages.` cargo_args.extend( features .features @@ -203,9 +214,12 @@ fn encoded_rustflags(options: &Options) -> Option { #[cfg(test)] mod test { + use clap::Parser; use pretty_assertions::assert_eq; use rusty_fork::rusty_fork_test; + use crate::Args; + use super::*; #[test] @@ -213,15 +227,15 @@ mod test { let options = Options::default(); let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, None, Phase::Check, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], ["check", "--tests", "--verbose", "--workspace"] ); assert_eq!( - cargo_argv(build_dir, None, Phase::Build, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Build, &options)[1..], ["test", "--no-run", "--verbose", "--workspace"] ); assert_eq!( - cargo_argv(build_dir, None, Phase::Test, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Test, &options)[1..], ["test", "--verbose", "--workspace"] ); } @@ -239,7 +253,12 @@ mod test { // the fix for // but it's temporarily regressed. assert_eq!( - cargo_argv(build_dir, Some(&[package_name]), Phase::Check, &options)[1..], + cargo_argv( + build_dir, + &PackageSelection::explicit([package_name]), + Phase::Check, + &options + )[1..], ["check", "--tests", "--verbose", "--package", package_name] ); @@ -288,15 +307,15 @@ mod test { .additional_cargo_args .extend(["--release".to_owned()]); assert_eq!( - cargo_argv(build_dir, None, Phase::Check, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], ["check", "--tests", "--verbose", "--workspace", "--release"] ); assert_eq!( - cargo_argv(build_dir, None, Phase::Build, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Build, &options)[1..], ["test", "--no-run", "--verbose", "--workspace", "--release"] ); assert_eq!( - cargo_argv(build_dir, None, Phase::Test, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Test, &options)[1..], [ "test", "--verbose", @@ -314,7 +333,7 @@ mod test { let options = Options::from_args(&args).unwrap(); let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, None, Phase::Check, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], [ "check", "--tests", @@ -331,7 +350,7 @@ mod test { let options = Options::from_args(&args).unwrap(); let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, None, Phase::Check, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], [ "check", "--tests", @@ -348,7 +367,7 @@ mod test { let options = Options::from_args(&args).unwrap(); let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, None, Phase::Check, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], ["check", "--tests", "--verbose", "--workspace",] ); } @@ -362,7 +381,7 @@ mod test { let options = Options::from_args(&args).unwrap(); let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, None, Phase::Check, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], [ "check", "--tests", @@ -380,7 +399,7 @@ mod test { let options = Options::from_args(&args).unwrap(); let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, None, Phase::Check, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], [ "check", "--tests", @@ -400,7 +419,7 @@ mod test { let options = Options::from_args(&args).unwrap(); let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, None, Phase::Build, &options)[1..], + cargo_argv(build_dir, &PackageSelection::All, Phase::Build, &options)[1..], [ "nextest", "run", diff --git a/src/lab.rs b/src/lab.rs index 4d3866cc..c303e946 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -16,8 +16,11 @@ use tracing::{debug, debug_span, error, trace, warn}; use crate::{ cargo::run_cargo, options::TestPackages, outcome::LabOutcome, output::OutputDir, - timeouts::Timeouts, workspace::Workspace, BaselineStrategy, BuildDir, Console, Context, Mutant, - Options, Phase, Result, Scenario, ScenarioOutcome, + package::PackageSelection, timeouts::Timeouts, workspace::Workspace, +}; +use crate::{ + BaselineStrategy, BuildDir, Console, Context, Mutant, Options, Phase, Result, Scenario, + ScenarioOutcome, }; /// Run all possible mutation experiments. @@ -187,7 +190,10 @@ impl Lab<'_> { options: self.options, console: self.console, } - .run_one_scenario(&Scenario::Baseline, Some(&all_mutated_packages)) + .run_one_scenario( + &Scenario::Baseline, + &PackageSelection::explicit(all_mutated_packages), + ) } /// Run until the input queue is empty. @@ -229,37 +235,28 @@ impl Worker<'_> { fn run_queue(mut self, work_queue: &Mutex>) -> Result<()> { let _span = debug_span!("worker thread", build_dir = ?self.build_dir.path()).entered(); loop { - // Extract the mutant in a separate statement so that we don't hold the - // lock while testing it. + // Not a `for` statement so that we don't hold the lock + // for the whole iteration. let Some(mutant) = work_queue.lock().expect("Lock pending work queue").next() else { return Ok(()); }; let _span = debug_span!("mutant", name = mutant.name(false, false)).entered(); - // variables held here for lifetime - let package_name = mutant.source_file.package_name.clone(); - let scenario = Scenario::Mutant(mutant); - let local_packages: [&str; 1]; - let named_packages; - let test_packages: Option<&[&str]> = match &self.options.test_packages { - TestPackages::Workspace => None, + let test_packages = match &self.options.test_packages { + TestPackages::Workspace => PackageSelection::All, TestPackages::Mutated => { - local_packages = [&package_name]; - Some(&local_packages) - } - TestPackages::Named(named) => { - named_packages = named.iter().map(String::as_str).collect_vec(); - Some(&named_packages) + PackageSelection::Explicit(vec![mutant.source_file.package_name.clone()]) } + TestPackages::Named(named) => PackageSelection::Explicit(named.clone()), }; debug!(?test_packages); - self.run_one_scenario(&scenario, test_packages)?; + self.run_one_scenario(&Scenario::Mutant(mutant), &test_packages)?; } } fn run_one_scenario( &mut self, scenario: &Scenario, - test_packages: Option<&[&str]>, + test_packages: &PackageSelection, ) -> Result { let mut scenario_output = self .output_mutex diff --git a/src/package.rs b/src/package.rs index 78878639..f11d4785 100644 --- a/src/package.rs +++ b/src/package.rs @@ -1,4 +1,4 @@ -// Copyright 2023 Martin Pool +// Copyright 2023-2024 Martin Pool //! Discover and represent cargo packages within a workspace. @@ -17,3 +17,17 @@ pub struct Package { /// like `["src/lib.rs"]`. pub top_sources: Vec, } + +/// A more specific view of which packages to mutate, after resolving `PackageFilter::Auto`. +#[derive(Debug, Clone)] +pub enum PackageSelection { + All, + Explicit(Vec), +} + +impl PackageSelection { + /// Helper constructor for `PackageSelection::Explicit`. + pub fn explicit, S: ToString>(names: I) -> Self { + Self::Explicit(names.into_iter().map(|s| s.to_string()).collect()) + } +} diff --git a/src/workspace.rs b/src/workspace.rs index 0c486866..f2785e5d 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -31,7 +31,7 @@ use crate::cargo::cargo_bin; use crate::console::Console; use crate::interrupt::check_interrupted; use crate::options::Options; -use crate::package::Package; +use crate::package::{Package, PackageSelection}; use crate::source::SourceFile; use crate::visit::{walk_tree, Discovered}; use crate::Result; @@ -56,13 +56,6 @@ pub enum PackageFilter { Auto(Utf8PathBuf), } -/// A more specific view of which packages to mutate, after resolving `PackageFilter::Auto`. -#[derive(Debug, Clone)] -enum PackageSelection { - All, - Explicit(Vec), -} - impl PackageFilter { /// Convenience constructor for `PackageFilter::Explicit`. pub fn explicit>(names: I) -> PackageFilter { From e286dcadcbc7fa521ce5e1acc3d33410f6f99fef Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:08:14 -0800 Subject: [PATCH 26/37] Copy testdata for shard test --- tests/shard.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/shard.rs b/tests/shard.rs index fac06561..8a348b17 100644 --- a/tests/shard.rs +++ b/tests/shard.rs @@ -1,17 +1,18 @@ -// Copyright 2023 Martin Pool +// Copyright 2023-2024 Martin Pool //! Test `--shard` use itertools::Itertools; mod util; -use util::run; +use util::{copy_of_testdata, run}; #[test] fn shard_divides_all_mutants() { // For speed, this only lists the mutants, trusting that the mutants // that are listed are the ones that are run. - let common_args = ["mutants", "--list", "-d", "testdata/well_tested"]; + let tmp = copy_of_testdata("well_tested"); + let common_args = ["mutants", "--list", "-d", tmp.path().to_str().unwrap()]; let full_list = String::from_utf8( run() .args(common_args) From 6fefb2cee0e0d4f04373ce66e9ce20e0dab4b2a3 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:09:51 -0800 Subject: [PATCH 27/37] Check named test packages exist --- src/lab.rs | 6 ++---- src/workspace.rs | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index c303e946..b5518bf3 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -43,6 +43,7 @@ pub fn test_mutants( if options.shuffle { fastrand::shuffle(&mut mutants); } + workspace.check_test_packages_are_present(&options.test_packages)?; output_dir.write_mutants_list(&mutants)?; console.discovered_mutants(&mutants); if mutants.is_empty() { @@ -62,7 +63,6 @@ pub fn test_mutants( .transpose() .context("Start jobserver")?; let lab = Lab { - workspace, output_mutex, jobserver, options, @@ -150,15 +150,13 @@ pub fn test_mutants( // the tree if no mutants are generated. warn!("No mutants were generated"); } else if lab_outcome.unviable == lab_outcome.total_mutants { - warn!("No mutants were viable; perhaps there is a problem with building in a scratch directory"); + warn!("No mutants were viable: perhaps there is a problem with building in a scratch directory. Look in mutants.out/log/* for more information."); } Ok(lab_outcome) } /// Common context across all scenarios, threads, and build dirs. struct Lab<'a> { - #[allow(unused)] // needed later to find packages - workspace: &'a Workspace, output_mutex: Mutex, jobserver: Option, options: &'a Options, diff --git a/src/workspace.rs b/src/workspace.rs index f2785e5d..05cdc968 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -30,7 +30,7 @@ use tracing::{debug, debug_span, error, warn}; use crate::cargo::cargo_bin; use crate::console::Console; use crate::interrupt::check_interrupted; -use crate::options::Options; +use crate::options::{Options, TestPackages}; use crate::package::{Package, PackageSelection}; use crate::source::SourceFile; use crate::visit::{walk_tree, Discovered}; @@ -168,6 +168,26 @@ impl Workspace { Ok(Workspace { metadata, packages }) } + pub fn has_package(&self, name: &str) -> bool { + self.packages.iter().any(|p| p.name == name) + } + + pub fn check_test_packages_are_present(&self, test_packages: &TestPackages) -> Result<()> { + if let TestPackages::Named(test_packages) = test_packages { + let missing = test_packages + .iter() + .filter(|&name| !self.has_package(name)) + .collect_vec(); + if !missing.is_empty() { + // TODO: Test for this + bail!( + "Some package names in test-packages are not present in the workspace: {}", + missing.into_iter().join(", ") + ); + } + } + Ok(()) + } /// Find packages matching some filter. fn packages(&self, package_filter: &PackageFilter) -> Result> { match package_filter.resolve_auto(&self.metadata)? { From 4cb1bac43daf48efb628fbb03cec82a952136e89 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:33:08 -0800 Subject: [PATCH 28/37] Rename to --test-package (singular) Makes it match --package --- book/src/workspaces.md | 4 +++- src/config.rs | 4 ++-- src/lab.rs | 12 ++++++------ src/main.rs | 4 ++-- src/options.rs | 36 ++++++++++++++++++------------------ src/workspace.rs | 8 ++++---- 6 files changed, 35 insertions(+), 33 deletions(-) diff --git a/book/src/workspaces.md b/book/src/workspaces.md index 2de95d58..4ca39dfa 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -40,8 +40,10 @@ By default, the baseline runs the tests from all and only the packages for which By default, each mutant runs only the tests from the package that's being mutated. -If the `--test-workspace` arguments or `test_workspace` configuration key is set, then all tests from the workspace are run for the baseline and against each mutant. +If the `--test-workspace=true` argument or `test_workspace` configuration key is set, then all tests from the workspace are run for the baseline and against each mutant. If the `--test-package` argument or `test_package` configuration key is set then the specified packages are tested for the baseline and all mutants. As for other options, the command line arguments have priority over the configuration file. + +Like `--package`, the argument to `--test-package` can be a comma-separated list, or the option can be repeated. diff --git a/src/config.rs b/src/config.rs index d85271f8..42c61dfb 100644 --- a/src/config.rs +++ b/src/config.rs @@ -48,7 +48,7 @@ pub struct Config { /// Cargo profile. pub profile: Option, /// Run tests from these packages for all mutants. - pub test_packages: Vec, + pub test_package: Vec, /// Choice of test tool: cargo or nextest. pub test_tool: Option, /// Timeout multiplier, relative to the baseline 'cargo test'. @@ -57,7 +57,7 @@ pub struct Config { pub build_timeout_multiplier: Option, /// Run tests from all packages in the workspace, not just the mutated package. /// - /// Overrides `test_packages`. + /// Overrides `test_package`. pub test_workspace: Option, } diff --git a/src/lab.rs b/src/lab.rs index b5518bf3..45155540 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -43,7 +43,7 @@ pub fn test_mutants( if options.shuffle { fastrand::shuffle(&mut mutants); } - workspace.check_test_packages_are_present(&options.test_packages)?; + workspace.check_test_packages_are_present(&options.test_package)?; output_dir.write_mutants_list(&mutants)?; console.discovered_mutants(&mutants); if mutants.is_empty() { @@ -239,22 +239,22 @@ impl Worker<'_> { return Ok(()); }; let _span = debug_span!("mutant", name = mutant.name(false, false)).entered(); - let test_packages = match &self.options.test_packages { + let test_package = match &self.options.test_package { TestPackages::Workspace => PackageSelection::All, TestPackages::Mutated => { PackageSelection::Explicit(vec![mutant.source_file.package_name.clone()]) } TestPackages::Named(named) => PackageSelection::Explicit(named.clone()), }; - debug!(?test_packages); - self.run_one_scenario(&Scenario::Mutant(mutant), &test_packages)?; + debug!(?test_package); + self.run_one_scenario(&Scenario::Mutant(mutant), &test_package)?; } } fn run_one_scenario( &mut self, scenario: &Scenario, - test_packages: &PackageSelection, + test_package: &PackageSelection, ) -> Result { let mut scenario_output = self .output_mutex @@ -282,7 +282,7 @@ impl Worker<'_> { match run_cargo( self.build_dir, self.jobserver, - test_packages, + test_package, phase, timeout, &mut scenario_output, diff --git a/src/main.rs b/src/main.rs index 8fe44981..ce3c58e0 100644 --- a/src/main.rs +++ b/src/main.rs @@ -306,7 +306,7 @@ pub struct Args { /// Run tests from these packages for all mutants. #[arg(long, help_heading = "Tests")] - test_packages: Vec, + test_package: Vec, /// Tool used to run test suites: cargo or nextest. #[arg(long, help_heading = "Execution")] @@ -316,7 +316,7 @@ pub struct Args { /// /// If false, only the tests in the mutated package are run. /// - /// Overrides `--test_packages`. + /// Overrides `--test_package`. #[arg(long, help_heading = "Tests")] test_workspace: Option, diff --git a/src/options.rs b/src/options.rs index 2613349e..00e256eb 100644 --- a/src/options.rs +++ b/src/options.rs @@ -58,7 +58,7 @@ pub struct Options { /// Which packages to test for a given mutant. /// /// Comes from `--test-workspace` etc. - pub test_packages: TestPackages, + pub test_package: TestPackages, /// The time limit for build tasks, if set. /// @@ -221,11 +221,11 @@ impl Options { ); // If either command line argument is set, it overrides the config. - let test_packages = if args.test_workspace == Some(true) { + let test_package = if args.test_workspace == Some(true) { TestPackages::Workspace - } else if !args.test_packages.is_empty() { + } else if !args.test_package.is_empty() { TestPackages::Named( - args.test_packages + args.test_package .iter() .flat_map(|s| s.split(',')) .map(|s| s.to_string()) @@ -233,8 +233,8 @@ impl Options { ) } else if args.test_workspace.is_none() && config.test_workspace == Some(true) { TestPackages::Workspace - } else if !config.test_packages.is_empty() { - TestPackages::Named(config.test_packages.clone()) + } else if !config.test_package.is_empty() { + TestPackages::Named(config.test_package.clone()) } else { TestPackages::Mutated }; @@ -278,7 +278,7 @@ impl Options { show_line_col: args.line_col, show_times: !args.no_times, show_all_logs: args.all_logs, - test_packages, + test_package, test_timeout: args.timeout.map(Duration::from_secs_f64), test_timeout_multiplier: args.timeout_multiplier.or(config.timeout_multiplier), test_tool: args.test_tool.or(config.test_tool).unwrap_or_default(), @@ -601,14 +601,14 @@ mod test { fn test_workspace_arg_true() { let args = Args::parse_from(["mutants", "--test-workspace=true"]); let options = Options::new(&args, &Config::default()).unwrap(); - assert_eq!(options.test_packages, TestPackages::Workspace); + 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(); - assert_eq!(options.test_packages, TestPackages::Mutated); + assert_eq!(options.test_package, TestPackages::Mutated); } #[test] @@ -619,7 +619,7 @@ mod test { "#}; let config = Config::from_str(config).unwrap(); let options = Options::new(&args, &config).unwrap(); - assert_eq!(options.test_packages, TestPackages::Workspace); + assert_eq!(options.test_package, TestPackages::Workspace); } #[test] @@ -630,7 +630,7 @@ mod test { "#}; let config = Config::from_str(config).unwrap(); let options = Options::new(&args, &config).unwrap(); - assert_eq!(options.test_packages, TestPackages::Mutated); + assert_eq!(options.test_package, TestPackages::Mutated); } #[test] @@ -641,7 +641,7 @@ mod test { "#}; let config = Config::from_str(config).unwrap(); let options = Options::new(&args, &config).unwrap(); - assert_eq!(options.test_packages, TestPackages::Workspace); + assert_eq!(options.test_package, TestPackages::Workspace); } #[test] @@ -652,7 +652,7 @@ mod test { "#}; let config = Config::from_str(config).unwrap(); let options = Options::new(&args, &config).unwrap(); - assert_eq!(options.test_packages, TestPackages::Mutated); + assert_eq!(options.test_package, TestPackages::Mutated); } #[test] @@ -661,22 +661,22 @@ mod test { let config = indoc! { r#" # Normally the packages would be ignored, but --test-workspace=false. test_workspace = true - test_packages = ["foo", "bar"] + test_package = ["foo", "bar"] "#}; let config = Config::from_str(config).unwrap(); let options = Options::new(&args, &config).unwrap(); assert_eq!( - options.test_packages, + options.test_package, TestPackages::Named(vec!["foo".to_string(), "bar".to_string()]) ); } #[test] - fn test_packages_arg_with_commas() { - let args = Args::parse_from(["mutants", "--test-packages=foo,bar"]); + fn test_package_arg_with_commas() { + let args = Args::parse_from(["mutants", "--test-package=foo,bar"]); let options = Options::new(&args, &Config::default()).unwrap(); assert_eq!( - options.test_packages, + options.test_package, TestPackages::Named(vec!["foo".to_string(), "bar".to_string()]) ); } diff --git a/src/workspace.rs b/src/workspace.rs index 05cdc968..7d0385da 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -172,16 +172,16 @@ impl Workspace { self.packages.iter().any(|p| p.name == name) } - pub fn check_test_packages_are_present(&self, test_packages: &TestPackages) -> Result<()> { - if let TestPackages::Named(test_packages) = test_packages { - let missing = test_packages + pub fn check_test_packages_are_present(&self, test_package: &TestPackages) -> Result<()> { + if let TestPackages::Named(test_package) = test_package { + let missing = test_package .iter() .filter(|&name| !self.has_package(name)) .collect_vec(); if !missing.is_empty() { // TODO: Test for this bail!( - "Some package names in test-packages are not present in the workspace: {}", + "Some package names in --test-package are not present in the workspace: {}", missing.into_iter().join(", ") ); } From 72e00abada00e838eaeda9cd73da145f07801ddd Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:33:21 -0800 Subject: [PATCH 29/37] Add testdata and tests for #394 --- testdata/cross_package_tests/Cargo_test.toml | 6 + testdata/cross_package_tests/README.md | 3 + .../cross_package_tests/lib/Cargo_test.toml | 7 + testdata/cross_package_tests/lib/src/lib.rs | 3 + .../cross_package_tests/tests/Cargo_test.toml | 10 ++ .../cross_package_tests/tests/tests/lib.rs | 6 + ...st__list_mutants_in_all_trees_as_json.snap | 129 ++++++++++++++++++ ...st__list_mutants_in_all_trees_as_text.snap | 11 ++ tests/workspace.rs | 105 +++++++++++++- 9 files changed, 279 insertions(+), 1 deletion(-) create mode 100644 testdata/cross_package_tests/Cargo_test.toml create mode 100644 testdata/cross_package_tests/README.md create mode 100644 testdata/cross_package_tests/lib/Cargo_test.toml create mode 100644 testdata/cross_package_tests/lib/src/lib.rs create mode 100644 testdata/cross_package_tests/tests/Cargo_test.toml create mode 100644 testdata/cross_package_tests/tests/tests/lib.rs diff --git a/testdata/cross_package_tests/Cargo_test.toml b/testdata/cross_package_tests/Cargo_test.toml new file mode 100644 index 00000000..c68769d8 --- /dev/null +++ b/testdata/cross_package_tests/Cargo_test.toml @@ -0,0 +1,6 @@ +# This workspace has no root package, which is an interesting edge case to test: +# older cargo-mutants assumed there was a root package and only tested it. + +[workspace] +members = ["lib", "tests"] +resolver = "2" diff --git a/testdata/cross_package_tests/README.md b/testdata/cross_package_tests/README.md new file mode 100644 index 00000000..7ed57c77 --- /dev/null +++ b/testdata/cross_package_tests/README.md @@ -0,0 +1,3 @@ +# cross-package tests + +This is an example of a workspace where code in one package is tested by another package. To measure the right coverage, we need to run tests either for the whole workspace or at least for some other named packages. diff --git a/testdata/cross_package_tests/lib/Cargo_test.toml b/testdata/cross_package_tests/lib/Cargo_test.toml new file mode 100644 index 00000000..3228fe0e --- /dev/null +++ b/testdata/cross_package_tests/lib/Cargo_test.toml @@ -0,0 +1,7 @@ +[package] +name = "cargo-mutants-testdata-cross-package-tests-lib" +version = "0.1.0" +edition = "2021" +publish = false + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html diff --git a/testdata/cross_package_tests/lib/src/lib.rs b/testdata/cross_package_tests/lib/src/lib.rs new file mode 100644 index 00000000..6eec06a1 --- /dev/null +++ b/testdata/cross_package_tests/lib/src/lib.rs @@ -0,0 +1,3 @@ +pub fn add(a: u32, b: u32) -> u32 { + a + b +} diff --git a/testdata/cross_package_tests/tests/Cargo_test.toml b/testdata/cross_package_tests/tests/Cargo_test.toml new file mode 100644 index 00000000..722f4cda --- /dev/null +++ b/testdata/cross_package_tests/tests/Cargo_test.toml @@ -0,0 +1,10 @@ +[package] +name = "cargo-mutants-testdata-cross-package-tests-tests" +version = "0.1.0" +edition = "2021" +publish = false + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] +cargo-mutants-testdata-cross-package-tests-lib = { path = "../lib" } diff --git a/testdata/cross_package_tests/tests/tests/lib.rs b/testdata/cross_package_tests/tests/tests/lib.rs new file mode 100644 index 00000000..fd5a1d1f --- /dev/null +++ b/testdata/cross_package_tests/tests/tests/lib.rs @@ -0,0 +1,6 @@ +use cargo_mutants_testdata_cross_package_tests_lib::add; + +#[test] +fn test_add() { + assert_eq!(add(1, 2), 3); +} diff --git a/tests/snapshots/list__list_mutants_in_all_trees_as_json.snap b/tests/snapshots/list__list_mutants_in_all_trees_as_json.snap index f0f40db6..e7dd54c8 100644 --- a/tests/snapshots/list__list_mutants_in_all_trees_as_json.snap +++ b/tests/snapshots/list__list_mutants_in_all_trees_as_json.snap @@ -559,6 +559,133 @@ expression: buf [] ``` +## testdata/cross_package_tests + +```json +[ + { + "file": "lib/src/lib.rs", + "function": { + "function_name": "add", + "return_type": "-> u32", + "span": { + "end": { + "column": 2, + "line": 3 + }, + "start": { + "column": 1, + "line": 1 + } + } + }, + "genre": "FnValue", + "package": "cargo-mutants-testdata-cross-package-tests-lib", + "replacement": "0", + "span": { + "end": { + "column": 10, + "line": 2 + }, + "start": { + "column": 5, + "line": 2 + } + } + }, + { + "file": "lib/src/lib.rs", + "function": { + "function_name": "add", + "return_type": "-> u32", + "span": { + "end": { + "column": 2, + "line": 3 + }, + "start": { + "column": 1, + "line": 1 + } + } + }, + "genre": "FnValue", + "package": "cargo-mutants-testdata-cross-package-tests-lib", + "replacement": "1", + "span": { + "end": { + "column": 10, + "line": 2 + }, + "start": { + "column": 5, + "line": 2 + } + } + }, + { + "file": "lib/src/lib.rs", + "function": { + "function_name": "add", + "return_type": "-> u32", + "span": { + "end": { + "column": 2, + "line": 3 + }, + "start": { + "column": 1, + "line": 1 + } + } + }, + "genre": "BinaryOperator", + "package": "cargo-mutants-testdata-cross-package-tests-lib", + "replacement": "-", + "span": { + "end": { + "column": 8, + "line": 2 + }, + "start": { + "column": 7, + "line": 2 + } + } + }, + { + "file": "lib/src/lib.rs", + "function": { + "function_name": "add", + "return_type": "-> u32", + "span": { + "end": { + "column": 2, + "line": 3 + }, + "start": { + "column": 1, + "line": 1 + } + } + }, + "genre": "BinaryOperator", + "package": "cargo-mutants-testdata-cross-package-tests-lib", + "replacement": "*", + "span": { + "end": { + "column": 8, + "line": 2 + }, + "start": { + "column": 7, + "line": 2 + } + } + } +] +``` + ## testdata/custom_top_file ```json @@ -9715,3 +9842,5 @@ expression: buf } ] ``` + + diff --git a/tests/snapshots/list__list_mutants_in_all_trees_as_text.snap b/tests/snapshots/list__list_mutants_in_all_trees_as_text.snap index cfac1c9f..4159add5 100644 --- a/tests/snapshots/list__list_mutants_in_all_trees_as_text.snap +++ b/tests/snapshots/list__list_mutants_in_all_trees_as_text.snap @@ -54,6 +54,15 @@ src/lib.rs:18:7: replace * with / in double ``` ``` +## testdata/cross_package_tests + +``` +lib/src/lib.rs:2:5: replace add -> u32 with 0 +lib/src/lib.rs:2:5: replace add -> u32 with 1 +lib/src/lib.rs:2:7: replace + with - in add +lib/src/lib.rs:2:7: replace + with * in add +``` + ## testdata/custom_top_file ``` @@ -536,3 +545,5 @@ main2/src/main.rs:10:5: replace triple_3 -> i32 with 0 main2/src/main.rs:10:5: replace triple_3 -> i32 with 1 main2/src/main.rs:10:5: replace triple_3 -> i32 with -1 ``` + + diff --git a/tests/workspace.rs b/tests/workspace.rs index 073a867c..45b5c026 100644 --- a/tests/workspace.rs +++ b/tests/workspace.rs @@ -2,10 +2,11 @@ //! Tests for cargo workspaces with multiple packages. -use std::fs::{self, read_to_string}; +use std::fs::{self, create_dir, read_to_string, write}; use insta::assert_snapshot; use itertools::Itertools; +use predicates::prelude::predicate; use serde_json::json; mod util; @@ -258,3 +259,105 @@ fn baseline_test_respects_package_options() { "" ); } + +#[test] +fn cross_package_tests() { + // This workspace has two packages, one of which contains the tests. + // Mutating the one with no tests will find test gaps, but + // either testing the whole workspace, or naming the test package, + // will show that it's actually all well tested. + // + // + + let tmp = copy_of_testdata("cross_package_tests"); + let path = tmp.path(); + + // Testing only this one package will find gaps. + run() + .args(["mutants"]) + .arg("-d") + .arg(path.join("lib")) + .assert() + .stdout(predicate::str::contains("4 missed")) + .code(2); // missed mutants + + // Just asking to *mutate* the whole workspace will not cause us + // to run the tests in "tests" against mutants in "lib". + run() + .args(["mutants", "--workspace"]) + .arg("-d") + .arg(path.join("lib")) + .assert() + .stdout(predicate::str::contains("4 missed")) + .code(2); // missed mutants + + // Similarly, starting in the workspace dir is not enough. + run() + .args(["mutants"]) + .arg("-d") + .arg(path) + .assert() + .stdout(predicate::str::contains("4 missed")) + .code(2); // missed mutants + + // Testing the whole workspace does catch everything. + run() + .args(["mutants", "--test-workspace=true"]) + .arg("-d") + .arg(path.join("lib")) + .assert() + .stdout(predicate::str::contains("4 caught")) + .code(0); + + // And naming the test package also catches everything. + run() + .args([ + "mutants", + "--test-package=cargo-mutants-testdata-cross-package-tests-tests", + ]) + .arg("-d") + .arg(path.join("lib")) + .assert() + .stdout(predicate::str::contains("4 caught")) + .code(0); + + // Using the wrong package name is an error + run() + .args(["mutants", "--test-package=tests"]) + .arg("-d") + .arg(path.join("lib")) + .env_remove("RUST_BACKTRACE") + .assert() + .stderr( + "Error: Some package names in test-package are not present in the workspace: tests\n", + ) + .code(1); + + // You can configure the test package in the workspace + let cargo_dir = path.join(".cargo"); + create_dir(&cargo_dir).unwrap(); + let mutants_toml_path = cargo_dir.join("mutants.toml"); + write(&mutants_toml_path, b"test_workspace = true").unwrap(); + // Now the mutants are caught + run() + .args(["mutants"]) + .arg("-d") + .arg(path.join("lib")) + .assert() + .stdout(predicate::str::contains("4 caught")) + .code(0); + + // It would also work to name the test package + write( + &mutants_toml_path, + br#"test_package = ["cargo-mutants-testdata-cross-package-tests-tests"]"#, + ) + .unwrap(); + run() + .args(["mutants"]) + .arg("-d") + .arg(path.join("lib")) + .assert() + .stdout(predicate::str::contains("4 caught")) + .code(0); +} From f5820618edbfd29aee1fbb53a740a782563a5182 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:35:23 -0800 Subject: [PATCH 30/37] News for #394 --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index db31ced3..478141cd 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,8 @@ ## Unreleased +- New: `--test-workspace` and `--test-package` arguments and config options support projects whose tests live in a different package. + - New: Mutate `proc_macro` targets and functions. - New: Write diffs to dedicated files under `mutants.out/diff/`. The filename is included in the mutant json output. From d2ef0e78dab061908eea849a8d0c7af84d1eba45 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:36:09 -0800 Subject: [PATCH 31/37] Fix cross-package tests for rename --- tests/workspace.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/workspace.rs b/tests/workspace.rs index 45b5c026..37028c4b 100644 --- a/tests/workspace.rs +++ b/tests/workspace.rs @@ -329,7 +329,7 @@ fn cross_package_tests() { .env_remove("RUST_BACKTRACE") .assert() .stderr( - "Error: Some package names in test-package are not present in the workspace: tests\n", + "Error: Some package names in --test-package are not present in the workspace: tests\n", ) .code(1); From 61b22772a6ed516e17c0608f0ceb4b11c669da9e Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:37:11 -0800 Subject: [PATCH 32/37] Comment --- src/workspace.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/workspace.rs b/src/workspace.rs index 7d0385da..d4b4d933 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -179,7 +179,6 @@ impl Workspace { .filter(|&name| !self.has_package(name)) .collect_vec(); if !missing.is_empty() { - // TODO: Test for this bail!( "Some package names in --test-package are not present in the workspace: {}", missing.into_iter().join(", ") From 1993524b7346775651e457d6a15f27de624f8459 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:44:04 -0800 Subject: [PATCH 33/37] Tidy up --- src/lab.rs | 56 +++++++++++++++++++++++++++++------------------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 45155540..ece777bf 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -111,32 +111,7 @@ pub fn test_mutants( lab.run_queue(build_dir, timeouts, work_queue) })); } - // The errors potentially returned from `join` are a special `std::thread::Result` - // that does not implement error, indicating that the thread panicked. - // Probably the most useful thing is to `resume_unwind` it. - // Inside that, there's an actual Mutants error indicating a non-panic error. - // Most likely, this would be "interrupted" but it might be some IO error - // etc. In that case, print them all and return the first. - let errors = threads - .into_iter() - .filter_map(|thread| match thread.join() { - Err(panic) => resume_unwind(panic), - Ok(Ok(())) => None, - Ok(Err(err)) => { - // To avoid spam, as a special case, don't print "interrupted" errors for each thread, - // since that should have been printed by check_interrupted: but, do return them. - if err.to_string() != "interrupted" { - error!("Worker thread failed: {:?}", err); - } - Some(err) - } - }) - .collect_vec(); // print/process them all - if let Some(first_err) = errors.into_iter().next() { - Err(first_err) - } else { - Ok(()) - } + join_threads(threads) })?; let output_dir = lab @@ -155,6 +130,35 @@ pub fn test_mutants( Ok(lab_outcome) } +fn join_threads(threads: Vec>>) -> Result<()> { + // The errors potentially returned from `join` are a special `std::thread::Result` + // that does not implement error, indicating that the thread panicked. + // Probably the most useful thing is to `resume_unwind` it. + // Inside that, there's an actual Mutants error indicating a non-panic error. + // Most likely, this would be "interrupted" but it might be some IO error + // etc. In that case, print them all and return the first. + let errors = threads + .into_iter() + .filter_map(|thread| match thread.join() { + Err(panic) => resume_unwind(panic), + Ok(Ok(())) => None, + Ok(Err(err)) => { + // To avoid console spam don't print "interrupted" errors for each thread, + // since that should have been printed by check_interrupted but do return them. + if err.to_string() != "interrupted" { + error!("Worker thread failed: {:?}", err); + } + Some(err) + } + }) + .collect_vec(); + if let Some(first_err) = errors.into_iter().next() { + Err(first_err) + } else { + Ok(()) + } +} + /// Common context across all scenarios, threads, and build dirs. struct Lab<'a> { output_mutex: Mutex, From 92eb8e14db4effd712dfe90beb9c3473cd4d0a30 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 10:50:00 -0800 Subject: [PATCH 34/37] Refactor Lab/Worker interface --- src/lab.rs | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index ece777bf..f353762e 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -30,7 +30,6 @@ use crate::{ /// /// Before testing the mutants, the lab checks that the source tree passes its tests with no /// mutations applied. -#[allow(clippy::too_many_lines)] // just for now pub fn test_mutants( mut mutants: Vec, workspace: &Workspace, @@ -174,27 +173,16 @@ impl Lab<'_> { /// If it fails, return None, indicating that no further testing should be done. /// /// If it succeeds, return the timeouts to be used for the other scenarios. - fn run_baseline( - &self, - baseline_build_dir: &BuildDir, - mutants: &[Mutant], - ) -> Result { + fn run_baseline(&self, build_dir: &BuildDir, mutants: &[Mutant]) -> Result { let all_mutated_packages = mutants .iter() .map(|m| m.source_file.package_name.as_str()) .unique() .collect_vec(); - Worker { - build_dir: baseline_build_dir, - output_mutex: &self.output_mutex, - jobserver: &self.jobserver, - timeouts: Timeouts::for_baseline(self.options), - options: self.options, - console: self.console, - } - .run_one_scenario( + self.make_worker(build_dir).run_one_scenario( &Scenario::Baseline, &PackageSelection::explicit(all_mutated_packages), + Timeouts::for_baseline(self.options), ) } @@ -207,15 +195,17 @@ impl Lab<'_> { timeouts: Timeouts, work_queue: &Mutex>, ) -> Result<()> { + self.make_worker(build_dir).run_queue(work_queue, timeouts) + } + + fn make_worker<'a>(&'a self, build_dir: &'a BuildDir) -> Worker<'a> { Worker { build_dir, output_mutex: &self.output_mutex, jobserver: &self.jobserver, - timeouts, options: self.options, console: self.console, } - .run_queue(work_queue) } } @@ -227,14 +217,17 @@ struct Worker<'a> { build_dir: &'a BuildDir, output_mutex: &'a Mutex, jobserver: &'a Option, - timeouts: Timeouts, options: &'a Options, console: &'a Console, } impl Worker<'_> { /// Run until the input queue is empty. - fn run_queue(mut self, work_queue: &Mutex>) -> Result<()> { + fn run_queue( + mut self, + work_queue: &Mutex>, + timeouts: Timeouts, + ) -> Result<()> { let _span = debug_span!("worker thread", build_dir = ?self.build_dir.path()).entered(); loop { // Not a `for` statement so that we don't hold the lock @@ -251,7 +244,7 @@ impl Worker<'_> { TestPackages::Named(named) => PackageSelection::Explicit(named.clone()), }; debug!(?test_package); - self.run_one_scenario(&Scenario::Mutant(mutant), &test_package)?; + self.run_one_scenario(&Scenario::Mutant(mutant), &test_package, timeouts)?; } } @@ -259,6 +252,7 @@ impl Worker<'_> { &mut self, scenario: &Scenario, test_package: &PackageSelection, + timeouts: Timeouts, ) -> Result { let mut scenario_output = self .output_mutex @@ -280,8 +274,8 @@ impl Worker<'_> { for &phase in self.options.phases() { self.console.scenario_phase_started(dir, phase); let timeout = match phase { - Phase::Test => self.timeouts.test, - Phase::Build | Phase::Check => self.timeouts.build, + Phase::Test => timeouts.test, + Phase::Build | Phase::Check => timeouts.build, }; match run_cargo( self.build_dir, From 0035ac463481eab71428a17b292fa4f99c4b1083 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 9 Nov 2024 11:00:01 -0800 Subject: [PATCH 35/37] cargo_argv does not need build_dir --- src/cargo.rs | 44 ++++++++++++++------------------------------ 1 file changed, 14 insertions(+), 30 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index 3c01fa62..5360017d 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -6,7 +6,6 @@ use std::env; use std::iter::once; use std::time::{Duration, Instant}; -use camino::Utf8Path; use itertools::Itertools; use tracing::{debug, debug_span, warn}; @@ -34,7 +33,7 @@ pub fn run_cargo( ) -> Result { let _span = debug_span!("run", ?phase).entered(); let start = Instant::now(); - let argv = cargo_argv(build_dir.path(), packages, phase, options); + let argv = cargo_argv(packages, phase, options); let mut env = vec![ // The tests might use Insta , and we don't want it to write // updates to the source tree, and we *certainly* don't want it to write @@ -85,12 +84,7 @@ pub fn cargo_bin() -> String { /// Make up the argv for a cargo check/build/test invocation, including argv[0] as the /// cargo binary itself. // (This is split out so it's easier to test.) -fn cargo_argv( - _build_dir: &Utf8Path, - packages: &PackageSelection, - phase: Phase, - options: &Options, -) -> Vec { +fn cargo_argv(packages: &PackageSelection, phase: Phase, options: &Options) -> Vec { let mut cargo_args = vec![cargo_bin()]; match phase { Phase::Test => match &options.test_tool { @@ -225,17 +219,16 @@ mod test { #[test] fn generate_cargo_args_for_baseline_with_default_options() { let options = Options::default(); - let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], ["check", "--tests", "--verbose", "--workspace"] ); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Build, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Build, &options)[1..], ["test", "--no-run", "--verbose", "--workspace"] ); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Test, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Test, &options)[1..], ["test", "--verbose", "--workspace"] ); } @@ -244,7 +237,6 @@ mod test { fn generate_cargo_args_with_additional_cargo_test_args_and_package() { let mut options = Options::default(); let package_name = "cargo-mutants-testdata-something"; - let build_dir = Utf8Path::new("/tmp/buildXYZ"); // let relative_manifest_path = Utf8PathBuf::from("testdata/something/Cargo.toml"); options .additional_cargo_test_args @@ -254,7 +246,6 @@ mod test { // but it's temporarily regressed. assert_eq!( cargo_argv( - build_dir, &PackageSelection::explicit([package_name]), Phase::Check, &options @@ -299,7 +290,6 @@ mod test { #[test] fn generate_cargo_args_with_additional_cargo_args_and_test_args() { let mut options = Options::default(); - let build_dir = Utf8Path::new("/tmp/buildXYZ"); options .additional_cargo_test_args .extend(["--lib", "--no-fail-fast"].iter().map(|s| s.to_string())); @@ -307,15 +297,15 @@ mod test { .additional_cargo_args .extend(["--release".to_owned()]); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], ["check", "--tests", "--verbose", "--workspace", "--release"] ); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Build, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Build, &options)[1..], ["test", "--no-run", "--verbose", "--workspace", "--release"] ); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Test, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Test, &options)[1..], [ "test", "--verbose", @@ -331,9 +321,8 @@ mod 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 build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], [ "check", "--tests", @@ -348,9 +337,8 @@ mod 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 build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], [ "check", "--tests", @@ -365,9 +353,8 @@ mod 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 build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], ["check", "--tests", "--verbose", "--workspace",] ); } @@ -379,9 +366,8 @@ mod test { ) .unwrap(); let options = Options::from_args(&args).unwrap(); - let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], [ "check", "--tests", @@ -397,9 +383,8 @@ mod test { fn profile_arg_passed_to_cargo() { let args = Args::try_parse_from(["mutants", "--profile", "mutants"].as_slice()).unwrap(); let options = Options::from_args(&args).unwrap(); - let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Check, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Check, &options)[1..], [ "check", "--tests", @@ -417,9 +402,8 @@ mod test { ) .unwrap(); let options = Options::from_args(&args).unwrap(); - let build_dir = Utf8Path::new("/tmp/buildXYZ"); assert_eq!( - cargo_argv(build_dir, &PackageSelection::All, Phase::Build, &options)[1..], + cargo_argv(&PackageSelection::All, Phase::Build, &options)[1..], [ "nextest", "run", From d783f325c6777beef5d933d1f738f097b974c3a7 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Mon, 11 Nov 2024 07:18:43 -0800 Subject: [PATCH 36/37] Run only shard 0/4 of cross-package tests Makes the test a bit faster --- tests/workspace.rs | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/tests/workspace.rs b/tests/workspace.rs index 37028c4b..feaf61e1 100644 --- a/tests/workspace.rs +++ b/tests/workspace.rs @@ -274,39 +274,40 @@ fn cross_package_tests() { // Testing only this one package will find gaps. run() - .args(["mutants"]) + .args(["mutants", "-v", "--shard=0/4"]) + .arg("--no-times") .arg("-d") .arg(path.join("lib")) .assert() - .stdout(predicate::str::contains("4 missed")) + .stdout(predicate::str::contains("1 mutant tested: 1 missed")) .code(2); // missed mutants // Just asking to *mutate* the whole workspace will not cause us // to run the tests in "tests" against mutants in "lib". run() - .args(["mutants", "--workspace"]) + .args(["mutants", "--workspace", "--shard=0/4"]) .arg("-d") .arg(path.join("lib")) .assert() - .stdout(predicate::str::contains("4 missed")) + .stdout(predicate::str::contains("1 missed")) .code(2); // missed mutants // Similarly, starting in the workspace dir is not enough. run() - .args(["mutants"]) + .args(["mutants", "-v", "--shard=0/4"]) .arg("-d") .arg(path) .assert() - .stdout(predicate::str::contains("4 missed")) + .stdout(predicate::str::contains("1 missed")) .code(2); // missed mutants // Testing the whole workspace does catch everything. run() - .args(["mutants", "--test-workspace=true"]) + .args(["mutants", "--test-workspace=true", "--shard=0/4"]) .arg("-d") .arg(path.join("lib")) .assert() - .stdout(predicate::str::contains("4 caught")) + .stdout(predicate::str::contains("1 caught")) .code(0); // And naming the test package also catches everything. @@ -314,11 +315,12 @@ fn cross_package_tests() { .args([ "mutants", "--test-package=cargo-mutants-testdata-cross-package-tests-tests", + "--shard=0/4", ]) .arg("-d") .arg(path.join("lib")) .assert() - .stdout(predicate::str::contains("4 caught")) + .stdout(predicate::str::contains("1 caught")) .code(0); // Using the wrong package name is an error @@ -341,10 +343,11 @@ fn cross_package_tests() { // Now the mutants are caught run() .args(["mutants"]) + .arg("--shard=0/4") .arg("-d") .arg(path.join("lib")) .assert() - .stdout(predicate::str::contains("4 caught")) + .stdout(predicate::str::contains("1 caught")) .code(0); // It would also work to name the test package @@ -355,9 +358,10 @@ fn cross_package_tests() { .unwrap(); run() .args(["mutants"]) + .arg("--shard=0/4") .arg("-d") .arg(path.join("lib")) .assert() - .stdout(predicate::str::contains("4 caught")) + .stdout(predicate::str::contains("1 caught")) .code(0); } From a8685cc22db43046f32111a3f9b7e3e28b6d489a Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Mon, 11 Nov 2024 07:20:24 -0800 Subject: [PATCH 37/37] mutants::skip join_threads for now --- src/lab.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lab.rs b/src/lab.rs index f353762e..66957461 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -129,6 +129,7 @@ pub fn test_mutants( Ok(lab_outcome) } +#[mutants::skip] // it's a little hard to observe that the threads were collected? fn join_threads(threads: Vec>>) -> Result<()> { // The errors potentially returned from `join` are a special `std::thread::Result` // that does not implement error, indicating that the thread panicked.