diff --git a/NEWS.md b/NEWS.md index e60d40c5..7cb5fdb1 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,8 @@ ## Unreleased +- New: `--iterate` option skips mutants that were previously caught or unviable. + - New: cargo-mutants starts a GNU jobserver, shared across all children, so that running multiple `--jobs` does not spawn an excessive number of compiler processes. The jobserver is on by default and can be turned off with `--jobserver false`. - Fixed: Don't error on diffs containing a "Binary files differ" message. diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index 24dd1508..2dd284f0 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -20,6 +20,7 @@ - [Using nextest](nextest.md) - [Baseline tests](baseline.md) - [Testing in-place](in-place.md) + - [Iterating on missed mutants](iterate.md) - [Strict lints](lints.md) - [Generating mutants](mutants.md) - [Error values](error-values.md) diff --git a/book/src/iterate.md b/book/src/iterate.md new file mode 100644 index 00000000..e258cb61 --- /dev/null +++ b/book/src/iterate.md @@ -0,0 +1,31 @@ +# Iterating on missed mutants + +When you're working to improve test coverage in a tree, you might use a process like this: + +1. Run `cargo-mutants` to find code that's untested, possibly filtering to some selected files. + +2. Think about why some mutants are missed, and then write tests that will catch them. + +3. Run cargo-mutants again to learn whether your tests caught all the mutants, or if any remain. + +4. Repeat until everything is caught. + +You can speed up this process by using the `--iterate` option. This tells cargo-mutants to skip mutants that were either caught or unviable in a previous run, and to accumulate the results. + +You can run repeatedly with `--iterate`, adding tests each time, until all the missed mutants are caught (or skipped.) + +## How it works + +When `--iterate` is given, cargo-mutants reads `mutants.out/caught.txt`, `previously_caught.txt`, and `unviable.txt` before renaming that directory to `mutants.out.old`. If those files don't exist, the lists are assumed to be empty. + +Mutants are then tested as usual, but excluding all the mutants named in those files. `--list --iterate` also applies this exclusion and shows you which mutants will be tested. + +Mutants are matched based on their file name, line, column, and description, just as shown in `--list` and in those files. As a result, if you insert or move text in a source file, some mutants may be re-tested. + +After testing, all the previously caught, caught, and unviable are written into `previously_caught.txt` so that they'll be excluded on future runs. + +`previously_caught.txt` is only written when `--iterate` is given. + +## Caution + +`--iterate` is a heuristic, and makes the assumption that any new changes you make won't reduce coverage, which might not be true. After you think you've caught all the mutants, you should run again without `--iterate` to make sure. diff --git a/book/src/mutants-out.md b/book/src/mutants-out.md index b58c2f71..32450ffe 100644 --- a/book/src/mutants-out.md +++ b/book/src/mutants-out.md @@ -25,8 +25,10 @@ The output directory contains: * `caught.txt`, `missed.txt`, `timeout.txt`, `unviable.txt`, each listing mutants with the corresponding outcome. +* `previously_caught.txt` accumulates a list of mutants caught in previous runs with [`--iterate`](iterate.md). + The contents of the directory and the format of these files is subject to change in future versions. These files are incrementally updated while cargo-mutants runs, so other programs can read them to follow progress. -There is generally no reason to include this directory in version control, so it is recommended that you add `/mutants.out*` to your `.gitignore` file. This will exclude both `mutants.out` and `mutants.out.old`. \ No newline at end of file +There is generally no reason to include this directory in version control, so it is recommended that you add `/mutants.out*` to your `.gitignore` file. This will exclude both `mutants.out` and `mutants.out.old`. diff --git a/src/build_dir.rs b/src/build_dir.rs index 1a84fcb0..d2132bf5 100644 --- a/src/build_dir.rs +++ b/src/build_dir.rs @@ -2,6 +2,8 @@ //! A directory containing mutated source to run cargo builds and tests. +use std::fmt::{self, Debug}; + use tempfile::TempDir; use tracing::info; @@ -13,7 +15,6 @@ use crate::*; /// /// 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, @@ -23,6 +24,14 @@ 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 a new build dir, copying from a source directory, subject to exclusions. pub fn copy_from( diff --git a/src/lab.rs b/src/lab.rs index 70e78fce..8fad797c 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -1,6 +1,7 @@ // Copyright 2021-2024 Martin Pool -//! Successively apply mutations to the source code and run cargo to check, build, and test them. +//! Successively apply mutations to the source code and run cargo to check, +//! build, and test them. use std::cmp::{max, min}; use std::panic::resume_unwind; @@ -10,9 +11,7 @@ use std::thread; use std::time::Instant; use itertools::Itertools; -use tracing::warn; -#[allow(unused)] -use tracing::{debug, debug_span, error, info, trace}; +use tracing::{debug, debug_span, error, trace, warn}; use crate::cargo::run_cargo; use crate::outcome::LabOutcome; @@ -31,16 +30,11 @@ use crate::*; pub fn test_mutants( mut mutants: Vec, workspace_dir: &Utf8Path, + output_dir: OutputDir, options: Options, console: &Console, ) -> Result { let start_time = Instant::now(); - let output_dir = OutputDir::new( - options - .output_in_dir - .as_ref() - .map_or(workspace_dir, |p| p.as_path()), - )?; console.set_debug_log(output_dir.open_debug_log()?); let jobserver = options .jobserver @@ -51,7 +45,6 @@ pub fn test_mutants( }) .transpose() .context("Start jobserver")?; - if options.shuffle { fastrand::shuffle(&mut mutants); } diff --git a/src/main.rs b/src/main.rs index e699b95c..4cbc0efb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,7 +2,7 @@ //! `cargo-mutants`: Find test gaps by inserting bugs. //! -//! See for more information. +//! See for the manual and more information. mod build_dir; mod cargo; @@ -47,7 +47,8 @@ use clap::builder::Styles; use clap::{ArgAction, CommandFactory, Parser, ValueEnum}; use clap_complete::{generate, Shell}; use color_print::cstr; -use tracing::debug; +use output::{load_previously_caught, OutputDir}; +use tracing::{debug, info}; use crate::build_dir::BuildDir; use crate::console::Console; @@ -198,6 +199,10 @@ pub struct Args { )] in_place: bool, + /// Skip mutants that were caught in previous runs. + #[arg(long, help_heading = "Filters")] + iterate: bool, + /// Run this many cargo build/test jobs in parallel. #[arg( long, @@ -418,7 +423,26 @@ fn main() -> Result<()> { } else { PackageFilter::Auto(start_dir.to_owned()) }; - let discovered = workspace.discover(&package_filter, &options, &console)?; + + let output_parent_dir = options + .output_in_dir + .clone() + .unwrap_or_else(|| workspace.dir.clone()); + + let mut discovered = workspace.discover(&package_filter, &options, &console)?; + + let previously_caught = if args.iterate { + let previously_caught = load_previously_caught(&output_parent_dir)?; + info!( + "Iteration excludes {} previously caught or unviable mutants", + previously_caught.len() + ); + discovered.remove_previously_caught(&previously_caught); + Some(previously_caught) + } else { + None + }; + console.clear(); if args.list_files { list_files(FmtToIoWrite::new(io::stdout()), &discovered.files, &options)?; @@ -437,7 +461,12 @@ fn main() -> Result<()> { if args.list { list_mutants(FmtToIoWrite::new(io::stdout()), &mutants, &options)?; } else { - let lab_outcome = test_mutants(mutants, &workspace.dir, options, &console)?; + let output_dir = OutputDir::new(&output_parent_dir)?; + if let Some(previously_caught) = previously_caught { + output_dir.write_previously_caught(&previously_caught)?; + } + console.set_debug_log(output_dir.open_debug_log()?); + let lab_outcome = test_mutants(mutants, &workspace.dir, output_dir, options, &console)?; exit(lab_outcome.exit_code()); } Ok(()) diff --git a/src/output.rs b/src/output.rs index 722ceb7d..a78020bb 100644 --- a/src/output.rs +++ b/src/output.rs @@ -1,4 +1,4 @@ -// Copyright 2021-2023 Martin Pool +// Copyright 2021-2024 Martin Pool //! A `mutants.out` directory holding logs and other output. @@ -13,7 +13,7 @@ use path_slash::PathExt; use serde::Serialize; use time::format_description::well_known::Rfc3339; use time::OffsetDateTime; -use tracing::info; +use tracing::{info, trace}; use crate::outcome::{LabOutcome, SummaryOutcome}; use crate::*; @@ -22,6 +22,9 @@ const OUTDIR_NAME: &str = "mutants.out"; const ROTATED_NAME: &str = "mutants.out.old"; const LOCK_JSON: &str = "lock.json"; const LOCK_POLL: Duration = Duration::from_millis(100); +static CAUGHT_TXT: &str = "caught.txt"; +static PREVIOUSLY_CAUGHT_TXT: &str = "previously_caught.txt"; +static UNVIABLE_TXT: &str = "unviable.txt"; /// The contents of a `lock.json` written into the output directory and used as /// a lock file to ensure that two cargo-mutants invocations don't try to write @@ -139,10 +142,10 @@ impl OutputDir { .open(output_dir.join("missed.txt")) .context("create missed.txt")?; let caught_list = list_file_options - .open(output_dir.join("caught.txt")) + .open(output_dir.join(CAUGHT_TXT)) .context("create caught.txt")?; let unviable_list = list_file_options - .open(output_dir.join("unviable.txt")) + .open(output_dir.join(UNVIABLE_TXT)) .context("create unviable.txt")?; let timeout_list = list_file_options .open(output_dir.join("timeout.txt")) @@ -222,19 +225,58 @@ impl OutputDir { pub fn take_lab_outcome(self) -> LabOutcome { self.lab_outcome } + + pub fn write_previously_caught(&self, caught: &[String]) -> Result<()> { + let p = self.path.join(PREVIOUSLY_CAUGHT_TXT); + // TODO: with_capacity when mutants knows to skip that; https://github.com/sourcefrog/cargo-mutants/issues/315 + // let mut b = String::with_capacity(caught.iter().map(|l| l.len() + 1).sum()); + let mut b = String::new(); + for l in caught { + b.push_str(l); + b.push('\n'); + } + File::options() + .create_new(true) + .write(true) + .open(&p) + .and_then(|mut f| f.write_all(b.as_bytes())) + .with_context(|| format!("Write {p:?}")) + } +} + +/// Return the string names of mutants previously caught in this output directory, including +/// unviable mutants. +/// +/// Returns an empty vec if there are none. +pub fn load_previously_caught(output_parent_dir: &Utf8Path) -> Result> { + let mut r = Vec::new(); + for filename in [CAUGHT_TXT, UNVIABLE_TXT, PREVIOUSLY_CAUGHT_TXT] { + let p = output_parent_dir.join(OUTDIR_NAME).join(filename); + trace!(?p, "read previously caught"); + if p.is_file() { + r.extend( + read_to_string(&p) + .with_context(|| format!("Read previously caught mutants from {p:?}"))? + .lines() + .map(|s| s.to_owned()), + ); + } + } + Ok(r) } #[cfg(test)] mod test { + use fs::write; use indoc::indoc; use itertools::Itertools; use pretty_assertions::assert_eq; - use tempfile::TempDir; + use tempfile::{tempdir, TempDir}; use super::*; fn minimal_source_tree() -> TempDir { - let tmp = tempfile::tempdir().unwrap(); + let tmp = tempdir().unwrap(); let path = tmp.path(); fs::write( path.join("Cargo.toml"), @@ -297,7 +339,7 @@ mod test { #[test] fn rotate() { - let temp_dir = tempfile::TempDir::new().unwrap(); + let temp_dir = TempDir::new().unwrap(); let temp_dir_path = Utf8Path::from_path(temp_dir.path()).unwrap(); // Create an initial output dir with one log. @@ -338,4 +380,45 @@ mod test { .join("mutants.out.old/log/baseline.log") .is_file()); } + + #[test] + fn track_previously_caught() { + let temp_dir = TempDir::new().unwrap(); + let parent = Utf8Path::from_path(temp_dir.path()).unwrap(); + + let example = "src/process.rs:213:9: replace ProcessStatus::is_success -> bool with true +src/process.rs:248:5: replace get_command_output -> Result with Ok(String::new()) +"; + + // Read from an empty dir: succeeds. + assert!(load_previously_caught(parent) + .expect("load succeeds") + .is_empty()); + + let output_dir = OutputDir::new(parent).unwrap(); + assert!(load_previously_caught(parent) + .expect("load succeeds") + .is_empty()); + + write(parent.join("mutants.out/caught.txt"), example.as_bytes()).unwrap(); + let previously_caught = load_previously_caught(parent).expect("load succeeds"); + assert_eq!( + previously_caught.iter().collect_vec(), + example.lines().collect_vec() + ); + + // make a new output dir, moving away the old one, and write this + drop(output_dir); + let output_dir = OutputDir::new(parent).unwrap(); + output_dir + .write_previously_caught(&previously_caught) + .unwrap(); + assert_eq!( + read_to_string(parent.join("mutants.out/caught.txt")).expect("read caught.txt"), + "" + ); + assert!(parent.join("mutants.out/previously_caught.txt").is_file()); + let now = load_previously_caught(parent).expect("load succeeds"); + assert_eq!(now.iter().collect_vec(), example.lines().collect_vec()); + } } diff --git a/src/visit.rs b/src/visit.rs index 1d8ff6f5..9c32596e 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -35,6 +35,19 @@ pub struct Discovered { pub files: Vec, } +impl Discovered { + pub(crate) fn remove_previously_caught(&mut self, previously_caught: &[String]) { + self.mutants.retain(|m| { + let name = m.name(true, false); + let c = previously_caught.contains(&name); + if c { + trace!(?name, "skip previously caught mutant"); + } + !c + }) + } +} + /// Discover all mutants and all source files. /// /// The list of source files includes even those with no mutants. diff --git a/tests/iterate.rs b/tests/iterate.rs new file mode 100644 index 00000000..2252bd9d --- /dev/null +++ b/tests/iterate.rs @@ -0,0 +1,267 @@ +// Copyright 2024 Martin Pool + +//! Tests for `--iterate` + +mod util; + +use std::fs::{create_dir, read_to_string, write, File}; +use std::io::Write; + +use indoc::indoc; +use predicates::prelude::*; +use pretty_assertions::assert_eq; +use tempfile::tempdir; + +use self::util::run; + +#[test] +fn iterate() { + let temp = tempdir().unwrap(); + + write( + temp.path().join("Cargo.toml"), + indoc! { r#" + [package] + name = "cargo_mutants_iterate" + edition = "2021" + version = "0.0.0" + publish = false + "# }, + ) + .unwrap(); + create_dir(temp.path().join("src")).unwrap(); + create_dir(temp.path().join("tests")).unwrap(); + + // First, write some untested code, and expect that the mutant is missed. + write( + temp.path().join("src/lib.rs"), + indoc! { r#" + pub fn is_two(a: usize) -> bool { a == 2 } + "#}, + ) + .unwrap(); + + run() + .arg("mutants") + .arg("-d") + .arg(temp.path()) + .arg("--list") + .assert() + .success() + .stderr(predicate::str::is_empty()) + .stdout(indoc! { r#" + src/lib.rs:1:35: replace is_two -> bool with true + src/lib.rs:1:35: replace is_two -> bool with false + src/lib.rs:1:37: replace == with != in is_two + "# }); + + run() + .arg("mutants") + .arg("--no-shuffle") + .arg("-d") + .arg(temp.path()) + .assert() + .code(2); // missed mutants + + assert_eq!( + read_to_string(temp.path().join("mutants.out/missed.txt")).unwrap(), + indoc! { r#" + src/lib.rs:1:35: replace is_two -> bool with true + src/lib.rs:1:35: replace is_two -> bool with false + src/lib.rs:1:37: replace == with != in is_two + "# } + ); + assert_eq!( + read_to_string(temp.path().join("mutants.out/caught.txt")).unwrap(), + "" + ); + assert!(!temp + .path() + .join("mutants.out/previously_caught.txt") + .is_file()); + + // Now add a test that should catch this. + write( + temp.path().join("tests/main.rs"), + indoc! { r#" + use cargo_mutants_iterate::*; + + #[test] + fn some_test() { + assert!(is_two(2)); + assert!(!is_two(4)); + } + "#}, + ) + .unwrap(); + + run() + .arg("mutants") + .arg("--no-shuffle") + .arg("-d") + .arg(temp.path()) + .assert() + .code(0); // caught it + + assert_eq!( + read_to_string(temp.path().join("mutants.out/missed.txt")).unwrap(), + "" + ); + assert_eq!( + read_to_string(temp.path().join("mutants.out/caught.txt")).unwrap(), + indoc! { r#" + src/lib.rs:1:35: replace is_two -> bool with true + src/lib.rs:1:35: replace is_two -> bool with false + src/lib.rs:1:37: replace == with != in is_two + "# } + ); + + // Now that everything's caught, run tests again and there should be nothing to test, + // on both the first and second run with --iterate + for _ in 0..2 { + run() + .arg("mutants") + .args(["--list", "--iterate"]) + .arg("-d") + .arg(temp.path()) + .assert() + .success() + .stdout(""); + run() + .arg("mutants") + .args(["--no-shuffle", "--iterate", "--in-place"]) + .arg("-d") + .arg(temp.path()) + .assert() + .success() + .stderr(predicate::str::contains( + "No mutants found under the active filters", + )) + .stdout(predicate::str::contains("Found 0 mutants to test")); + assert_eq!( + read_to_string(temp.path().join("mutants.out/caught.txt")).unwrap(), + "" + ); + assert_eq!( + read_to_string(temp.path().join("mutants.out/previously_caught.txt")) + .unwrap() + .lines() + .count(), + 3 + ); + } + + // Add some more code and it should be seen as untested. + let mut src = File::options() + .append(true) + .open(temp.path().join("src/lib.rs")) + .unwrap(); + src.write_all("pub fn not_two(a: usize) -> bool { !is_two(a) }\n".as_bytes()) + .unwrap(); + drop(src); + + // We should see only the new function as untested + let added_mutants = indoc! { r#" + src/lib.rs:2:36: replace not_two -> bool with true + src/lib.rs:2:36: replace not_two -> bool with false + src/lib.rs:2:36: delete ! in not_two + "# }; + run() + .arg("mutants") + .args(["--list", "--iterate"]) + .arg("-d") + .arg(temp.path()) + .assert() + .success() + .stdout(added_mutants); + + // These are missed by a new incremental run + run() + .arg("mutants") + .args(["--no-shuffle", "--iterate", "--in-place"]) + .arg("-d") + .arg(temp.path()) + .assert() + .code(2); + assert_eq!( + read_to_string(temp.path().join("mutants.out/missed.txt")).unwrap(), + added_mutants + ); + + // Add a new test that catches some but not all mutants + File::options() + .append(true) + .open(temp.path().join("tests/main.rs")) + .unwrap() + .write_all("#[test] fn three_is_not_two() { assert!(not_two(3)); }\n".as_bytes()) + .unwrap(); + run() + .arg("mutants") + .args(["--no-shuffle", "--iterate", "--in-place"]) + .arg("-d") + .arg(temp.path()) + .assert() + .code(2); + assert_eq!( + read_to_string(temp.path().join("mutants.out/missed.txt")).unwrap(), + "src/lib.rs:2:36: replace not_two -> bool with true\n" + ); + + // There should only be one more mutant to test + run() + .arg("mutants") + .args(["--list", "--iterate", "--in-place"]) + .arg("-d") + .arg(temp.path()) + .assert() + .success() + .stdout("src/lib.rs:2:36: replace not_two -> bool with true\n"); + + // Add another test + File::options() + .append(true) + .open(temp.path().join("tests/main.rs")) + .unwrap() + .write_all("#[test] fn two_is_not_not_two() { assert!(!not_two(2)); }\n".as_bytes()) + .unwrap(); + run() + .arg("mutants") + .args(["--list", "--iterate", "--in-place"]) + .arg("-d") + .arg(temp.path()) + .assert() + .success() + .stdout("src/lib.rs:2:36: replace not_two -> bool with true\n"); + run() + .arg("mutants") + .args(["--no-shuffle", "--iterate", "--in-place"]) + .arg("-d") + .arg(temp.path()) + .assert() + .success(); + assert_eq!( + read_to_string(temp.path().join("mutants.out/missed.txt")).unwrap(), + "" + ); + assert_eq!( + read_to_string(temp.path().join("mutants.out/caught.txt")).unwrap(), + "src/lib.rs:2:36: replace not_two -> bool with true\n" + ); + assert_eq!( + read_to_string(temp.path().join("mutants.out/previously_caught.txt")) + .unwrap() + .lines() + .count(), + 5 + ); + + // nothing more is missed + run() + .arg("mutants") + .args(["--list", "--iterate", "--in-place"]) + .arg("-d") + .arg(temp.path()) + .assert() + .success() + .stdout(""); +}