Add --baseline=skip (#247)

If you already are sure the tests pass in a clean tree, then this will skip running them and save a little time.

- [x] Mention in the book
- [x] Refactor implementation
- [x] Tests: 
- [x] With `--baseline=skip` in a small tree we don't see the baseline run, and we do see the warning about a timeout
- [x] Use it in CI
- [x] News

Fixes #153

.github/workflows/tests.yml

@@ -29,7 +29,7 @@ env:
   CARGO_MUTANTS_MINIMUM_TEST_TIMEOUT: 60
 
 jobs:
-  build:
+  test:
     strategy:
       matrix:
         os: [macOS-latest, ubuntu-latest, windows-latest]
@@ -121,11 +121,11 @@ jobs:
 
   cargo-mutants:
     runs-on: ubuntu-latest
-    needs: [build, pr-mutants, release-binary]
+    needs: [test, pr-mutants, release-binary]
     strategy:
       matrix:
         shard: [0, 1, 2, 3, 4, 5, 6, 7]
-        test_tool: [cargo, nextest]
+        test_tool: [cargo]
     steps:
       - uses: actions/checkout@v3
       - uses: dtolnay/rust-toolchain@master
@@ -144,9 +144,10 @@ jobs:
         run: |
           install cargo-mutants $HOME/.cargo/bin/
       - name: Mutants
+        # Skip baselines because this action depends on the tests
         run: >
           cargo mutants --no-shuffle -vV --shard ${{ matrix.shard }}/8
-          --test-tool ${{ matrix.test_tool }}
+          --test-tool ${{ matrix.test_tool }} --baseline=skip
       - name: Archive mutants.out
         uses: actions/upload-artifact@v3
         if: always()

.markdownlint.jsonc

@@ -2,5 +2,6 @@
   "default": true,
   // Allow lines that are all italics or bold; they're not meant to be headings
   "MD036": false,
-  "MD013": false // Allow long lines
+  "MD013": false, // Allow long lines
+  "MD033": false // Allow inline HTML, needed for <div class="warning"> etc
 }

NEWS.md

@@ -4,6 +4,8 @@
 
 - New: Mutate `+, -, *, /, %, &, ^, |, <<, >>` binary ops, and their corresponding assignment ops like `+=`.
 
+- New: `--baseline=skip` option to skip running tests in an unmutated tree, when they're already been checked externally.
+
 - Changed: Stop generating mutations of `||` and `&&` to `!=` and `||`, because it seems to raise too many low-value false positives that may be hard to test.
 
 ## 24.1.0

book/src/SUMMARY.md

@@ -17,6 +17,7 @@
   - [Passing options to Cargo](cargo-args.md)
   - [Build directories](build-dirs.md)
   - [Using nextest](nextest.md)
+  - [Baseline tests](baseline.md)
 - [Generating mutants](mutants.md)
   - [Error values](error-values.md)
 - [Improving performance](performance.md)

book/src/baseline.md

@@ -0,0 +1,29 @@
+# Baseline tests
+
+Normally, cargo-mutants builds and runs your tree in a temporary directory before applying any mutations. This makes sure that your tests are in fact all passing, including in the copy of the tree that cargo-mutants will mutate.
+
+Baseline tests can be skipped by passing the `--baseline=skip` command line option. (There is no config option for this.)
+
+<div class="warning">
+If you use <code>--baseline=skip</code>, you must make sure that the tests are actually passing, otherwise the results of cargo-mutants will be meaningless. cargo-mutants will probably report that all or most mutations were caught, but the test failures were not because of the mutations.
+</div>
+
+## Performance effects
+
+The performance gain from skipping the baseline is one run of the full test suite, plus one incremental build. When the baseline is run, its build is typically slow because it must do the initial build of the tree, but when it is skipped, the first mutant will have to do a full (rather than incremental) build instead.
+
+This means that, in a run that tests many mutants, the relative performance gain from skipping the baseline will be relatively small. However, it may still be useful to skip baseline tests in some specific situations.
+
+## Timeouts
+
+Normally, cargo-mutants uses the baseline test to establish an appropriate `timeout` for the test suite. If you skip the baseline, you should set `--timeout` manually.
+
+## Use cases for skipping baseline tests
+
+`--baseline=skip` might be useful in these situations:
+
+1. You are running cargo-mutants in a CI or build system that separately runs the tests before cargo-mutants. In this case, you can be confident that the tests are passing, and you can save time by skipping the baseline. In particular, if you are [sharding](shards.md) work in CI, this avoids running the baseline on each shard.
+
+2. You're repeatedly running `cargo-mutants` with different options, without changing the source code, perhaps with different `--file` or `--exclude` options.
+
+3. You're developing `cargo-mutants` itself, and running it repeatedly on a tree that doesn't change.

book/src/how-it-works.md

@@ -20,7 +20,7 @@ The basic approach is:
   - Before applying any mutations, check that `cargo test` succeeds in the
     scratch directory: perhaps a test is already broken, or perhaps the tree
     doesn't build when copied because it relies on relative paths to find
-    dependencies, etc.
+    dependencies, etc. This is called the "baseline" test.
   - If running more than one parallel job, make the appropriate number of
     additional scratch directories.
 

src/lab.rs

@@ -55,48 +55,34 @@ pub fn test_mutants(
 
     let output_mutex = Mutex::new(output_dir);
     let mut build_dirs = vec![BuildDir::new(workspace_dir, &options, console)?];
-    let baseline_outcome = {
-        let _span = debug_span!("baseline").entered();
-        test_scenario(
-            &mut build_dirs[0],
-            &output_mutex,
-            &Scenario::Baseline,
-            &all_packages,
-            options.test_timeout.unwrap_or(Duration::MAX),
-            &options,
-            console,
-        )?
-    };
-    if !baseline_outcome.success() {
-        error!(
-            "cargo {} failed in an unmutated tree, so no mutants were tested",
-            baseline_outcome.last_phase(),
-        );
-        return Ok(output_mutex
-            .into_inner()
-            .expect("lock output_dir")
-            .take_lab_outcome());
-    }
-
-    let mutated_test_timeout = if let Some(timeout) = options.test_timeout {
-        timeout
-    } else if let Some(baseline_test_duration) = baseline_outcome
-        .phase_results()
-        .iter()
-        .find(|r| r.phase == Phase::Test)
-        .map(|r| r.duration)
-    {
-        let auto_timeout = max(
-            options.minimum_test_timeout,
-            baseline_test_duration.mul_f32(5.0),
-        );
-        if options.show_times {
-            console.autoset_timeout(auto_timeout);
+    let baseline_outcome = match options.baseline {
+        BaselineStrategy::Run => {
+            let outcome = test_scenario(
+                &mut build_dirs[0],
+                &output_mutex,
+                &Scenario::Baseline,
+                &all_packages,
+                options.test_timeout.unwrap_or(Duration::MAX),
+                &options,
+                console,
+            )?;
+            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());
+            }
+            Some(outcome)
         }
-        auto_timeout
-    } else {
-        Duration::MAX
+        BaselineStrategy::Skip => None,
     };
+    let test_timeout = test_timeout(&baseline_outcome, &options, console);
 
     let jobs = max(1, min(options.jobs.unwrap_or(1), mutants.len()));
     console.build_dirs_start(jobs - 1);
@@ -131,7 +117,7 @@ pub fn test_mutants(
                             &output_mutex,
                             &Scenario::Mutant(mutant),
                             &[&package],
-                            mutated_test_timeout,
+                            test_timeout,
                             &options,
                             console,
                         )
@@ -163,6 +149,35 @@ pub fn test_mutants(
     Ok(lab_outcome)
 }
 
+fn test_timeout(
+    baseline_outcome: &Option<ScenarioOutcome>,
+    options: &Options,
+    console: &Console,
+) -> Duration {
+    if let Some(timeout) = options.test_timeout {
+        timeout
+    } else if options.check_only {
+        Duration::ZERO
+    } else if options.baseline == BaselineStrategy::Skip {
+        warn!("An explicit timeout is recommended when using --baseline=skip; using 300 seconds by default");
+        Duration::from_secs(300)
+    } else {
+        let auto_timeout = max(
+            options.minimum_test_timeout,
+            baseline_outcome
+                .as_ref()
+                .expect("Baseline tests should have run")
+                .total_phase_duration(Phase::Test)
+                .mul_f32(5.0),
+        );
+        if options.show_times {
+            // TODO: Just `info!` not a special method.
+            console.autoset_timeout(auto_timeout);
+        }
+        auto_timeout
+    }
+}
+
 /// Test various phases of one scenario in a build dir.
 ///
 /// The [BuildDir] is passed as mutable because it's for the exclusive use of this function for the

src/main.rs

@@ -38,7 +38,7 @@ use std::process::exit;
 
 use anyhow::{anyhow, ensure, Context, Result};
 use camino::{Utf8Path, Utf8PathBuf};
-use clap::{ArgAction, CommandFactory, Parser};
+use clap::{ArgAction, CommandFactory, Parser, ValueEnum};
 use clap_complete::{generate, Shell};
 use tracing::debug;
 
@@ -70,6 +70,16 @@ enum Cargo {
     Mutants(Args),
 }
 
+#[derive(Debug, Default, ValueEnum, Clone, Copy, Eq, PartialEq)]
+pub enum BaselineStrategy {
+    /// Run tests in an unmutated tree before testing mutants.
+    #[default]
+    Run,
+
+    /// Don't run tests in an unmutated tree: assume that they pass.
+    Skip,
+}
+
 /// Find inadequately-tested code that can be removed without any tests failing.
 ///
 /// See <https://github.com/sourcefrog/cargo-mutants> for more information.
@@ -80,6 +90,10 @@ struct Args {
     #[arg(long)]
     all_logs: bool,
 
+    /// baseline strategy: check that tests pass in an unmutated tree before testing mutants.
+    #[arg(long, value_enum, default_value_t = BaselineStrategy::Run)]
+    baseline: BaselineStrategy,
+
     /// print mutants that were caught by tests.
     #[arg(long, short = 'v')]
     caught: bool,

src/options.rs

@@ -22,6 +22,9 @@ use crate::*;
 /// config file.
 #[derive(Default, Debug, Clone)]
 pub struct Options {
+    /// Run tests in an unmutated tree?
+    pub baseline: BaselineStrategy,
+
     /// Don't run the tests, just see if each mutant builds.
     pub check_only: bool,
 
@@ -138,6 +141,7 @@ impl Options {
                 &args.cargo_test_args,
                 &config.additional_cargo_test_args,
             ),
+            baseline: args.baseline,
             check_only: args.check,
             colors: true, // TODO: An option for this and use CLICOLORS.
             emit_json: args.json,
@@ -228,6 +232,21 @@ mod test {
         assert_eq!(options.test_tool, TestTool::Nextest);
     }
 
+    #[test]
+    fn options_from_baseline_arg() {
+        let args = Args::parse_from(["mutants", "--baseline", "skip"]);
+        let options = Options::new(&args, &Config::default()).unwrap();
+        assert_eq!(options.baseline, BaselineStrategy::Skip);
+
+        let args = Args::parse_from(["mutants", "--baseline", "run"]);
+        let options = Options::new(&args, &Config::default()).unwrap();
+        assert_eq!(options.baseline, BaselineStrategy::Run);
+
+        let args = Args::parse_from(["mutants"]);
+        let options = Options::new(&args, &Config::default()).unwrap();
+        assert_eq!(options.baseline, BaselineStrategy::Run);
+    }
+
     #[test]
     fn test_tool_from_config() {
         let config = indoc! { r#"

src/outcome.rs

@@ -198,6 +198,17 @@ impl ScenarioOutcome {
         &self.phase_results
     }
 
+    /// Return the total time spent in commands for one phase.
+    ///
+    /// If the phase was not run, returns zero.
+    pub fn total_phase_duration(&self, phase: Phase) -> Duration {
+        self.phase_results
+            .iter()
+            .filter(|pr| pr.phase == phase)
+            .map(|pr| pr.duration)
+            .sum()
+    }
+
     /// True if this status indicates the user definitely needs to see the logs, because a task
     /// failed that should not have failed.
     pub fn should_show_logs(&self) -> bool {

src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap

@@ -232,11 +232,12 @@ src/in_diff.rs: replace += with *= in partial_new_file
 src/interrupt.rs: replace install_handler with ()
 src/lab.rs: replace test_mutants -> Result<LabOutcome> with Ok(Default::default())
 src/lab.rs: replace test_mutants -> Result<LabOutcome> with Err(::anyhow::anyhow!("mutated!"))
-src/lab.rs: replace == with != in test_mutants
 src/lab.rs: replace - with + in test_mutants
 src/lab.rs: replace - with / in test_mutants
 src/lab.rs: replace == with != in test_mutants
 src/lab.rs: replace == with != in test_mutants
+src/lab.rs: replace test_timeout -> Duration with Default::default()
+src/lab.rs: replace == with != in test_timeout
 src/lab.rs: replace test_scenario -> Result<ScenarioOutcome> with Ok(Default::default())
 src/lab.rs: replace test_scenario -> Result<ScenarioOutcome> with Err(::anyhow::anyhow!("mutated!"))
 src/lab.rs: replace || with && in test_scenario
@@ -381,6 +382,8 @@ src/outcome.rs: replace ScenarioOutcome::last_phase -> Phase with Default::defau
 src/outcome.rs: replace ScenarioOutcome::last_phase_result -> ProcessStatus with Default::default()
 src/outcome.rs: replace ScenarioOutcome::phase_results -> &[PhaseResult] with Vec::leak(Vec::new())
 src/outcome.rs: replace ScenarioOutcome::phase_results -> &[PhaseResult] with Vec::leak(vec![Default::default()])
+src/outcome.rs: replace ScenarioOutcome::total_phase_duration -> Duration with Default::default()
+src/outcome.rs: replace == with != in ScenarioOutcome::total_phase_duration
 src/outcome.rs: replace ScenarioOutcome::should_show_logs -> bool with true
 src/outcome.rs: replace ScenarioOutcome::should_show_logs -> bool with false
 src/outcome.rs: replace && with || in ScenarioOutcome::should_show_logs

tests/cli/main.rs

@@ -528,6 +528,32 @@ fn small_well_tested_tree_is_clean() {
     assert!(log_content.contains("factorial(6) = 0"));
 }
 
+#[test]
+fn test_small_well_tested_tree_with_baseline_skip() {
+    let tmp_src_dir = copy_of_testdata("small_well_tested");
+    run()
+        .arg("mutants")
+        .args(["--no-times", "--no-shuffle", "-v", "-V", "--baseline=skip"])
+        .arg("-d")
+        .arg(tmp_src_dir.path())
+        .assert()
+        .success()
+        .stdout(predicate::function(|stdout| {
+            insta::assert_snapshot!(stdout);
+            true
+        }))
+        .stderr(
+            predicate::str::contains(
+                "An explicit timeout is recommended when using --baseline=skip",
+            )
+            .and(predicate::str::contains("Unmutated baseline in").not()),
+        );
+    assert!(!tmp_src_dir
+        .path()
+        .join("mutants.out/log/baseline.log")
+        .exists());
+}
+
 #[test]
 fn cdylib_tree_is_well_tested() {
     let tmp_src_dir = copy_of_testdata("cdylib");

tests/cli/snapshots/cli__small_well_tested_tree_with_baseline_skip.snap

@@ -0,0 +1,11 @@
+---
+source: tests/cli/main.rs
+expression: stdout
+---
+Found 4 mutants to test
+caught   src/lib.rs:5:5: replace factorial -> u32 with 0
+caught   src/lib.rs:5:5: replace factorial -> u32 with 1
+caught   src/lib.rs:7:11: replace *= with += in factorial
+caught   src/lib.rs:7:11: replace *= with /= in factorial
+4 mutants tested: 4 caught
+