Repository: sourcefrog/cargo-mutants (rust)

Issue:
Baseline always runs tests with --workspace, which may include unwanted tests
I am working in a workspace with multiple crates. Some have tests that depend on stuff like DBs. As pointed out in the documentation, I tried to exclude those crates from mutation testing, or similarly to explicitly run mutation testing in specific packages only. No matter how I do it, the baseline tests fail because apparently it always executes `cargo test --workspace` and runs all the tests, including those with external dependencies and they naturally fail. So I tried passing `--exclude <crate>`. That made the baseline tests pass but all the following tests failed because this option can only be used with the `--workspace` option. So I passed this one along but then the baseline tests fail again because I am not allowed to use `--workspace` twice and the baseline tests already have that.

Is there any way to do what I am asking? It seems to that what I want should be the default behavior: when excluding crates from mutation tests, their tests should not be run at all. And when running with the `-d` option, it should only run tests in that directory, not the entire workspace. In short, I think the `--workspace` option might not be the optimal way for running the baseline tests.

Pull request description:
Baseline only tests packages that will be mutated
Fixes #151 

Patch:
diff --git a/Cargo.lock b/Cargo.lock
index e1375aa..40400d2 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -643,9 +643,9 @@ dependencies = [
 
 [[package]]
 name = "nutmeg"
-version = "0.1.4"
+version = "0.1.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "210b363fa6901c372f264fa32ef3710c0e86328901deaed31294fecfd51e848b"
+checksum = "956e58d52fb6a72060882865340633bf356d389f099a057fd3d7b26f5a304493"
 dependencies = [
  "atty",
  "parking_lot",
diff --git a/Cargo.toml b/Cargo.toml
index b7c29de..e9f4d34 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -66,9 +66,8 @@ version = "1.5"
 version = "0.5.1"
 
 [dependencies.nutmeg]
-version = "0.1.4"
+version = "0.1.2"
 # git = "https://github.com/sourcefrog/nutmeg.git"
-# branch = "const-new"
 
 [dependencies.proc-macro2]
 features = ["span-locations"]
@@ -93,6 +92,7 @@ walkdir = "2.3"
 
 [workspace]
 members = ["mutants_attrs"]
+resolver = "2"
 
 # Exclude all testdata, so that they're more isolated from the real tree, and
 # so that support for testing workspaces does not try to test the whole
@@ -116,6 +116,7 @@ exclude = [
     "testdata/tree/mut_ref",
     "testdata/tree/never_type",
     "testdata/tree/override_dependency",
+    "testdata/tree/package-fails/",
     "testdata/tree/patch_dependency",
     "testdata/tree/relative_dependency",
     "testdata/tree/replace_dependency",
diff --git a/src/cargo.rs b/src/cargo.rs
index 2b97a6b..58d7478 100644
--- a/src/cargo.rs
+++ b/src/cargo.rs
@@ -131,24 +131,14 @@ impl Tool for CargoTool {
     fn compose_argv(
         &self,
         build_dir: &BuildDir,
-        scenario: &Scenario,
+        packages: Option<&[&Package]>,
         phase: Phase,
         options: &Options,
     ) -> Result<Vec<String>> {
-        Ok(cargo_argv(
-            build_dir.path(),
-            scenario.package(),
-            phase,
-            options,
-        ))
+        Ok(cargo_argv(build_dir.path(), packages, phase, options))
     }
 
-    fn compose_env(
-        &self,
-        _scenario: &Scenario,
-        _phase: Phase,
-        _options: &Options,
-    ) -> Result<Vec<(String, String)>> {
+    fn compose_env(&self) -> Result<Vec<(String, String)>> {
         Ok(self.env.clone())
     }
 }
@@ -163,9 +153,10 @@ 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,
-    package: Option<&Package>,
+    packages: Option<&[&Package]>,
     phase: Phase,
     options: &Options,
 ) -> Vec<String> {
@@ -173,9 +164,16 @@ fn cargo_argv(
     if phase == Phase::Check || phase == Phase::Build {
         cargo_args.push("--tests".to_string());
     }
-    if let Some(package) = package {
+    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() {
+            cargo_args.push("--package".to_owned());
+            cargo_args.push(package);
+        }
     } else {
         cargo_args.push("--workspace".to_string());
     }
@@ -304,7 +302,7 @@ mod test {
         });
         let build_manifest_path = build_dir.join(relative_manifest_path);
         assert_eq!(
-            cargo_argv(build_dir, Some(&package), Phase::Check, &options)[1..],
+            cargo_argv(build_dir, Some(&[&package]), Phase::Check, &options)[1..],
             [
                 "check",
                 "--tests",
@@ -313,7 +311,7 @@ mod test {
             ]
         );
         assert_eq!(
-            cargo_argv(build_dir, Some(&package), Phase::Build, &options)[1..],
+            cargo_argv(build_dir, Some(&[&package]), Phase::Build, &options)[1..],
             [
                 "build",
                 "--tests",
@@ -322,7 +320,7 @@ mod test {
             ]
         );
         assert_eq!(
-            cargo_argv(build_dir, Some(&package), Phase::Test, &options)[1..],
+            cargo_argv(build_dir, Some(&[&package]), Phase::Test, &options)[1..],
             [
                 "test",
                 "--manifest-path",
diff --git a/src/console.rs b/src/console.rs
index ce628e1..ba0eb51 100644
--- a/src/console.rs
+++ b/src/console.rs
@@ -210,6 +210,10 @@ impl Console {
         ));
     }
 
+    pub fn clear(&self) {
+        self.view.clear()
+    }
+
     pub fn message(&self, message: &str) {
         self.view.message(message)
     }
diff --git a/src/lab.rs b/src/lab.rs
index b4f574a..e95164c 100644
--- a/src/lab.rs
+++ b/src/lab.rs
@@ -8,6 +8,7 @@ use std::thread;
 use std::time::{Duration, Instant};
 
 use anyhow::{anyhow, Context, Result};
+use itertools::Itertools;
 use tracing::warn;
 #[allow(unused)]
 use tracing::{debug, debug_span, error, info, trace};
@@ -16,6 +17,7 @@ use crate::console::Console;
 use crate::outcome::{LabOutcome, Phase, PhaseResult, ScenarioOutcome};
 use crate::output::OutputDir;
 use crate::process::Process;
+use crate::source::Package;
 use crate::*;
 
 /// Run all possible mutation experiments.
@@ -45,6 +47,7 @@ pub fn test_unmutated_then_all_mutants(
     if mutants.is_empty() {
         return Err(anyhow!("No mutants found"));
     }
+    let all_packages = mutants.iter().map(|m| m.package()).unique().collect_vec();
 
     let output_mutex = Mutex::new(output_dir);
     let mut build_dirs = vec![BuildDir::new(source_tree, &options, console)?];
@@ -56,6 +59,7 @@ pub fn test_unmutated_then_all_mutants(
             &output_mutex,
             &options,
             &Scenario::Baseline,
+            &all_packages,
             options.test_timeout.unwrap_or(Duration::MAX),
             console,
         )?
@@ -120,6 +124,7 @@ pub fn test_unmutated_then_all_mutants(
                     let next = numbered_mutants.lock().expect("lock mutants queue").next();
                     if let Some((mutant_id, mutant)) = next {
                         let _span = debug_span!("mutant", id = mutant_id).entered();
+                        let package = mutant.package().clone();
                         // We don't care about the outcome; it's been collected into the output_dir.
                         let _outcome = test_scenario(
                             tool,
@@ -127,6 +132,7 @@ pub fn test_unmutated_then_all_mutants(
                             &output_mutex,
                             &options,
                             &Scenario::Mutant(mutant),
+                            &[&package],
                             mutated_test_timeout,
                             console,
                         )
@@ -169,6 +175,7 @@ fn test_scenario(
     output_mutex: &Mutex<OutputDir>,
     options: &Options,
     scenario: &Scenario,
+    packages: &[&Package],
     test_timeout: Duration,
     console: &Console,
 ) -> Result<ScenarioOutcome> {
@@ -197,8 +204,8 @@ fn test_scenario(
             Phase::Test => test_timeout,
             _ => Duration::MAX,
         };
-        let argv = tool.compose_argv(build_dir, scenario, phase, options)?;
-        let env = tool.compose_env(scenario, phase, options)?;
+        let argv = tool.compose_argv(build_dir, Some(packages), phase, options)?;
+        let env = tool.compose_env()?;
         let process_status = Process::run(
             &argv,
             &env,
diff --git a/src/list.rs b/src/list.rs
index 908196f..9eae357 100644
--- a/src/list.rs
+++ b/src/list.rs
@@ -36,6 +36,7 @@ pub(crate) fn list_mutants<W: fmt::Write>(
     console: &Console,
 ) -> Result<()> {
     let discovered = walk_tree(tool, source_tree_root, options, console)?;
+    console.clear();
     if options.emit_json {
         let mut list: Vec<serde_json::Value> = Vec::new();
         for mutant in discovered.mutants {
diff --git a/src/scenario.rs b/src/scenario.rs
index a9f7f15..59a2879 100644
--- a/src/scenario.rs
+++ b/src/scenario.rs
@@ -3,7 +3,6 @@
 use serde::Serialize;
 use std::fmt;
 
-use crate::source::Package;
 use crate::Mutant;
 
 /// A scenario is either a freshening build in the source tree, a baseline test with no mutations, or a mutation test.
@@ -35,13 +34,4 @@ impl Scenario {
             Scenario::Mutant(mutant) => mutant.log_file_name_base(),
         }
     }
-
-    /// Return the package tested for this scenario (for mutants), or None to test
-    /// every package (for baselines).
-    pub fn package(&self) -> Option<&Package> {
-        match self {
-            Scenario::Mutant(mutant) => Some(mutant.package()),
-            Scenario::Baseline => None,
-        }
-    }
 }
diff --git a/src/source.rs b/src/source.rs
index d707980..44eaa17 100644
--- a/src/source.rs
+++ b/src/source.rs
@@ -57,7 +57,7 @@ impl SourceFile {
 }
 
 /// A package built and tested as a unit.
-#[derive(Debug, Eq, PartialEq)] // no clone, use Arc
+#[derive(Debug, Eq, PartialEq, Hash, Clone)]
 pub struct Package {
     /// The short name of the package, like "mutants".
     pub name: String,
diff --git a/src/tool.rs b/src/tool.rs
index dac4264..96cdbcf 100644
--- a/src/tool.rs
+++ b/src/tool.rs
@@ -16,7 +16,7 @@ use tracing::{debug, debug_span, trace};
 
 use crate::options::Options;
 use crate::outcome::Phase;
-use crate::scenario::Scenario;
+use crate::source::Package;
 use crate::SourceFile;
 use crate::{build_dir, Result};
 
@@ -48,15 +48,10 @@ pub trait Tool: Debug + Send + Sync {
     fn compose_argv(
         &self,
         build_dir: &build_dir::BuildDir,
-        scenario: &Scenario,
+        packages: Option<&[&Package]>,
         phase: Phase,
         options: &Options,
     ) -> Result<Vec<String>>;
 
-    fn compose_env(
-        &self,
-        scenario: &Scenario,
-        phase: Phase,
-        options: &Options,
-    ) -> Result<Vec<(String, String)>>;
+    fn compose_env(&self) -> Result<Vec<(String, String)>>;
 }


Test patch:
diff --git a/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap b/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap
index dc663f1..76a0c5c 100644
--- a/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap
+++ b/src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap
@@ -58,6 +58,7 @@ src/console.rs: replace Console::start_testing_mutants with ()
 src/console.rs: replace Console::scenario_phase_started with ()
 src/console.rs: replace Console::scenario_phase_finished with ()
 src/console.rs: replace Console::lab_finished with ()
+src/console.rs: replace Console::clear with ()
 src/console.rs: replace Console::message with ()
 src/console.rs: replace Console::tick with ()
 src/console.rs: replace Console::make_terminal_writer -> TerminalWriter with Default::default()
@@ -301,8 +302,6 @@ src/scenario.rs: replace Scenario::is_mutant -> bool with true
 src/scenario.rs: replace Scenario::is_mutant -> bool with false
 src/scenario.rs: replace Scenario::log_file_name_base -> String with String::new()
 src/scenario.rs: replace Scenario::log_file_name_base -> String with "xyzzy".into()
-src/scenario.rs: replace Scenario::package -> Option<&Package> with None
-src/scenario.rs: replace Scenario::package -> Option<&Package> with Some(&Default::default())
 src/source.rs: replace SourceFile::tree_relative_slashes -> String with String::new()
 src/source.rs: replace SourceFile::tree_relative_slashes -> String with "xyzzy".into()
 src/textedit.rs: replace <impl From for LineColumn>::from -> Self with Default::default()
diff --git a/testdata/tree/package_fails/Cargo.toml b/testdata/tree/package_fails/Cargo.toml
new file mode 100644
index 0000000..ff04f9f
--- /dev/null
+++ b/testdata/tree/package_fails/Cargo.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 = ["passing", "failing"]
+resolver = "2"
diff --git a/testdata/tree/package_fails/README.md b/testdata/tree/package_fails/README.md
new file mode 100644
index 0000000..8af27a6
--- /dev/null
+++ b/testdata/tree/package_fails/README.md
@@ -0,0 +1,7 @@
+# package-fails
+
+A tree with a workspace and two packages, one of which has already-failing
+tests. (Let's suppose they have a non-hermetic dependency.) It should still
+be possible to test the other.
+
+See <https://github.com/sourcefrog/cargo-mutants/issues/151>.
diff --git a/testdata/tree/package_fails/failing/Cargo.toml b/testdata/tree/package_fails/failing/Cargo.toml
new file mode 100644
index 0000000..5d1e717
--- /dev/null
+++ b/testdata/tree/package_fails/failing/Cargo.toml
@@ -0,0 +1,5 @@
+[package]
+name = "cargo-mutants-testdata-package-fails-failing"
+version = "0.1.0"
+edition = "2021"
+publish = false
diff --git a/testdata/tree/package_fails/failing/src/lib.rs b/testdata/tree/package_fails/failing/src/lib.rs
new file mode 100644
index 0000000..e0a6746
--- /dev/null
+++ b/testdata/tree/package_fails/failing/src/lib.rs
@@ -0,0 +1,13 @@
+pub fn triple(a: usize) -> usize {
+    3 * a
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn triple_3_is_10() {
+        assert_eq!(triple(3), 10);
+    }
+}
diff --git a/testdata/tree/package_fails/passing/Cargo.toml b/testdata/tree/package_fails/passing/Cargo.toml
new file mode 100644
index 0000000..1f15882
--- /dev/null
+++ b/testdata/tree/package_fails/passing/Cargo.toml
@@ -0,0 +1,7 @@
+[package]
+name = "cargo-mutants-testdata-package-fails-passing"
+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/tree/package_fails/passing/src/lib.rs b/testdata/tree/package_fails/passing/src/lib.rs
new file mode 100644
index 0000000..ef5323a
--- /dev/null
+++ b/testdata/tree/package_fails/passing/src/lib.rs
@@ -0,0 +1,8 @@
+pub fn triple(a: usize) -> usize {
+    a * 3
+}
+
+#[test]
+fn triple_2() {
+    assert_eq!(triple(2), 6);
+}
diff --git a/tests/cli/main.rs b/tests/cli/main.rs
index e89ac40..3c52d72 100644
--- a/tests/cli/main.rs
+++ b/tests/cli/main.rs
@@ -4,7 +4,7 @@
 
 use std::env;
 use std::fmt::Write;
-use std::fs::{self, read_dir};
+use std::fs::{self, read_dir, read_to_string};
 use std::io::Read;
 use std::path::{Path, PathBuf};
 use std::thread::sleep;
@@ -523,13 +523,20 @@ fn workspace_tree_is_well_tested() {
         assert_eq!(baseline_phases.len(), 2);
         assert_eq!(baseline_phases[0]["process_status"], "Success");
         assert_eq!(
-            baseline_phases[0]["argv"].as_array().unwrap()[1..],
-            ["build", "--tests", "--workspace"]
+            baseline_phases[0]["argv"].as_array().unwrap().into_iter().map(|v| v.as_str().unwrap()).skip(1).collect_vec().join(" "),
+            "build --tests --package cargo_mutants_testdata_workspace_utils --package main --package main2"
         );
         assert_eq!(baseline_phases[1]["process_status"], "Success");
         assert_eq!(
-            baseline_phases[1]["argv"].as_array().unwrap()[1..],
-            ["test", "--workspace"]
+            baseline_phases[1]["argv"]
+                .as_array()
+                .unwrap()
+                .into_iter()
+                .map(|v| v.as_str().unwrap())
+                .skip(1)
+                .collect_vec()
+                .join(" "),
+            "test --package cargo_mutants_testdata_workspace_utils --package main --package main2"
         );
     }
 
@@ -560,17 +567,52 @@ fn workspace_tree_is_well_tested() {
         assert_eq!(baseline_phases.len(), 2);
         assert_eq!(baseline_phases[0]["process_status"], "Success");
         assert_eq!(
-            baseline_phases[0]["argv"].as_array().unwrap()[1..],
-            ["build", "--tests", "--workspace"]
+            baseline_phases[0]["argv"].as_array().unwrap()[1..].into_iter().map(|v| v.as_str().unwrap()).join(" "),
+            "build --tests --package cargo_mutants_testdata_workspace_utils --package main --package main2",
         );
         assert_eq!(baseline_phases[1]["process_status"], "Success");
         assert_eq!(
-            baseline_phases[1]["argv"].as_array().unwrap()[1..],
-            ["test", "--workspace"]
+            baseline_phases[1]["argv"].as_array().unwrap()[1..]
+                .into_iter()
+                .map(|v| v.as_str().unwrap())
+                .join(" "),
+            "test --package cargo_mutants_testdata_workspace_utils --package main --package main2",
         );
     }
 }
 
+#[test]
+/// Baseline tests in a workspace only test the packages that will later
+/// be mutated.
+/// See <https://github.com/sourcefrog/cargo-mutants/issues/151>
+fn in_workspace_only_relevant_packages_included_in_baseline_tests() {
+    let tmp = copy_of_testdata("package_fails");
+    run()
+        .args(["mutants", "-f", "passing/src/lib.rs", "--no-shuffle", "-d"])
+        .arg(tmp.path())
+        .assert()
+        .success();
+    assert_eq!(
+        read_to_string(tmp.path().join("mutants.out/caught.txt")).unwrap(),
+        indoc! { "\
+            passing/src/lib.rs:1: replace triple -> usize with 0
+            passing/src/lib.rs:1: replace triple -> usize with 1
+            "}
+    );
+    assert_eq!(
+        read_to_string(tmp.path().join("mutants.out/timeout.txt")).unwrap(),
+        ""
+    );
+    assert_eq!(
+        read_to_string(tmp.path().join("mutants.out/missed.txt")).unwrap(),
+        ""
+    );
+    assert_eq!(
+        read_to_string(tmp.path().join("mutants.out/unviable.txt")).unwrap(),
+        ""
+    );
+}
+
 #[test]
 fn copy_testdata_doesnt_include_build_artifacts() {
     // If there is a target or mutants.out in the source directory, we don't want it in the copy,
@@ -881,11 +923,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.* build --tests --workspace --release")
+    regex::Regex::new(r"cargo.* build --tests --manifest-path .* --release")
         .unwrap()
         .captures(&log_content)
         .unwrap();
-    regex::Regex::new(r"cargo.* test --workspace --release")
+    regex::Regex::new(r"cargo.* test --manifest-path .* --release")
         .unwrap()
         .captures(&log_content)
         .unwrap();
diff --git a/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_json.snap b/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_json.snap
index 755c26b..13ec1b5 100644
--- a/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_json.snap
+++ b/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_json.snap
@@ -472,6 +472,49 @@ expression: buf
 ]
 ```
 
+## testdata/tree/package_fails
+
+```json
+[
+  {
+    "file": "failing/src/lib.rs",
+    "function": "triple",
+    "genre": "FnValue",
+    "line": 1,
+    "package": "cargo-mutants-testdata-package-fails-failing",
+    "replacement": "0",
+    "return_type": "-> usize"
+  },
+  {
+    "file": "failing/src/lib.rs",
+    "function": "triple",
+    "genre": "FnValue",
+    "line": 1,
+    "package": "cargo-mutants-testdata-package-fails-failing",
+    "replacement": "1",
+    "return_type": "-> usize"
+  },
+  {
+    "file": "passing/src/lib.rs",
+    "function": "triple",
+    "genre": "FnValue",
+    "line": 1,
+    "package": "cargo-mutants-testdata-package-fails-passing",
+    "replacement": "0",
+    "return_type": "-> usize"
+  },
+  {
+    "file": "passing/src/lib.rs",
+    "function": "triple",
+    "genre": "FnValue",
+    "line": 1,
+    "package": "cargo-mutants-testdata-package-fails-passing",
+    "replacement": "1",
+    "return_type": "-> usize"
+  }
+]
+```
+
 ## testdata/tree/patch_dependency
 
 ```json
diff --git a/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_text.snap b/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_text.snap
index 8609700..849f1ef 100644
--- a/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_text.snap
+++ b/tests/cli/snapshots/cli__list_mutants_in_all_trees_as_text.snap
@@ -139,6 +139,15 @@ src/lib.rs:6: replace is_even -> bool with true
 src/lib.rs:6: replace is_even -> bool with false
 ```
 
+## testdata/tree/package_fails
+
+```
+failing/src/lib.rs:1: replace triple -> usize with 0
+failing/src/lib.rs:1: replace triple -> usize with 1
+passing/src/lib.rs:1: replace triple -> usize with 0
+passing/src/lib.rs:1: replace triple -> usize with 1
+```
+
 ## testdata/tree/patch_dependency
 
 ```