From 21c19f98804072df8097d93e54b93a168f5b97e9 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 4 Nov 2023 11:03:50 -0700 Subject: [PATCH 01/36] Unify calls to walk_tree Discover all mutants up front the same way, before listing or testing them --- src/list.rs | 21 +++++++-------------- src/main.rs | 19 +++++-------------- src/visit.rs | 18 ++++++++---------- 3 files changed, 20 insertions(+), 38 deletions(-) diff --git a/src/list.rs b/src/list.rs index 9eae357c..1b9edcfd 100644 --- a/src/list.rs +++ b/src/list.rs @@ -5,13 +5,12 @@ use std::fmt; use std::io; -use camino::Utf8Path; use serde_json::{json, Value}; use crate::console::style_mutant; -use crate::console::Console; use crate::path::Utf8PathSlashes; -use crate::{walk_tree, Options, Result, Tool}; +use crate::visit::Discovered; +use crate::{Options, Result}; /// Convert `fmt::Write` to `io::Write`. pub(crate) struct FmtToIoWrite(W); @@ -30,13 +29,9 @@ impl fmt::Write for FmtToIoWrite { pub(crate) fn list_mutants( mut out: W, - tool: &dyn Tool, - source_tree_root: &Utf8Path, + discovered: Discovered, options: &Options, - console: &Console, ) -> Result<()> { - let discovered = walk_tree(tool, source_tree_root, options, console)?; - console.clear(); if options.emit_json { let mut list: Vec = Vec::new(); for mutant in discovered.mutants { @@ -66,15 +61,13 @@ pub(crate) fn list_mutants( pub(crate) fn list_files( mut out: W, - tool: &dyn Tool, - source: &Utf8Path, + discovered: Discovered, options: &Options, - console: &Console, ) -> Result<()> { - let files = walk_tree(tool, source, options, console)?.files; if options.emit_json { let json_list = Value::Array( - files + discovered + .files .iter() .map(|source_file| { json!({ @@ -86,7 +79,7 @@ pub(crate) fn list_files( ); writeln!(out, "{}", serde_json::to_string_pretty(&json_list)?)?; } else { - for file in files { + for file in discovered.files { writeln!(out, "{}", file.tree_relative_path.to_slash_path())?; } } diff --git a/src/main.rs b/src/main.rs index 35720f15..e06e2adb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -235,22 +235,13 @@ fn main() -> Result<()> { } let options = Options::new(&args, &config)?; debug!(?options); + let discovered = walk_tree(&tool, &source_tree_root, &options, &console)?; if args.list_files { - list_files( - FmtToIoWrite::new(io::stdout()), - &tool, - &source_tree_root, - &options, - &console, - )?; + console.clear(); + list_files(FmtToIoWrite::new(io::stdout()), discovered, &options)?; } else if args.list { - list_mutants( - FmtToIoWrite::new(io::stdout()), - &tool, - &source_tree_root, - &options, - &console, - )?; + console.clear(); + list_mutants(FmtToIoWrite::new(io::stdout()), discovered, &options)?; } else { let lab_outcome = lab::test_unmutated_then_all_mutants(&tool, &source_tree_root, options, &console)?; diff --git a/src/visit.rs b/src/visit.rs index 1046dfe4..6319859e 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -411,16 +411,14 @@ mod test { ..Default::default() }; let mut list_output = String::new(); - crate::list_mutants( - &mut list_output, - &CargoTool::new(), - &Utf8Path::new(".") - .canonicalize_utf8() - .expect("Canonicalize source path"), - &options, - &Console::new(), - ) - .expect("Discover mutants in own source tree"); + let source_tree_root = &Utf8Path::new(".") + .canonicalize_utf8() + .expect("Canonicalize source path"); + let console = Console::new(); + let discovered = walk_tree(&CargoTool::new(), source_tree_root, &options, &console) + .expect("Discover mutants"); + crate::list_mutants(&mut list_output, discovered, &options) + .expect("Discover mutants in own source tree"); // Strip line numbers so this is not too brittle. let line_re = Regex::new(r"(?m)^([^:]+:)\d+:( .*)$").unwrap(); From 328db6e95374bddb48dfc9ab2181bd8b0a010702 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 4 Nov 2023 11:05:04 -0700 Subject: [PATCH 02/36] Clean up --- src/main.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main.rs b/src/main.rs index e06e2adb..3e9764b5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -207,6 +207,7 @@ fn main() -> Result<()> { exit(exit_code::USAGE); } }; + let console = Console::new(); console.setup_global_trace(args.level)?; interrupt::install_handler(); @@ -226,13 +227,12 @@ fn main() -> Result<()> { }; let tool = CargoTool::new(); let source_tree_root = tool.find_root(source_path)?; - let config; - if args.no_config { - config = config::Config::default(); + let config = if args.no_config { + config::Config::default() } else { - config = config::Config::read_tree_config(&source_tree_root)?; - debug!(?config); - } + config::Config::read_tree_config(&source_tree_root)? + }; + debug!(?config); let options = Options::new(&args, &config)?; debug!(?options); let discovered = walk_tree(&tool, &source_tree_root, &options, &console)?; From aa5fd9398b00b83ca0a853dcdb19d02d4a8b0d77 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 4 Nov 2023 11:05:34 -0700 Subject: [PATCH 03/36] Show version or completions earlier --- src/main.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main.rs b/src/main.rs index 3e9764b5..48b59469 100644 --- a/src/main.rs +++ b/src/main.rs @@ -208,10 +208,6 @@ fn main() -> Result<()> { } }; - let console = Console::new(); - console.setup_global_trace(args.level)?; - interrupt::install_handler(); - if args.version { println!("{NAME} {VERSION}"); return Ok(()); @@ -220,6 +216,10 @@ fn main() -> Result<()> { return Ok(()); } + let console = Console::new(); + console.setup_global_trace(args.level)?; + interrupt::install_handler(); + let source_path: &Utf8Path = if let Some(p) = &args.dir { p } else { From 141749563f607a209f5a2d8bbb140eb79ecf2702 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 4 Nov 2023 11:12:16 -0700 Subject: [PATCH 04/36] Also unify walk_tree call for testing --- src/lab.rs | 4 +++- src/main.rs | 9 +++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 3c1aa05f..27861b28 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -18,6 +18,7 @@ use crate::outcome::{LabOutcome, Phase, PhaseResult, ScenarioOutcome}; use crate::output::OutputDir; use crate::process::Process; use crate::source::Package; +use crate::visit::Discovered; use crate::*; /// Run all possible mutation experiments. @@ -26,6 +27,7 @@ use crate::*; /// mutations applied. pub fn test_unmutated_then_all_mutants( tool: &dyn Tool, + discovered: Discovered, source_tree: &Utf8Path, options: Options, console: &Console, @@ -37,8 +39,8 @@ pub fn test_unmutated_then_all_mutants( .map_or(source_tree, |p| p.as_path()); let output_dir = OutputDir::new(output_in_dir)?; console.set_debug_log(output_dir.open_debug_log()?); + let mut mutants = discovered.mutants; - let mut mutants = walk_tree(tool, source_tree, &options, console)?.mutants; if options.shuffle { fastrand::shuffle(&mut mutants); } diff --git a/src/main.rs b/src/main.rs index 48b59469..01cbfd5c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -243,8 +243,13 @@ fn main() -> Result<()> { console.clear(); list_mutants(FmtToIoWrite::new(io::stdout()), discovered, &options)?; } else { - let lab_outcome = - lab::test_unmutated_then_all_mutants(&tool, &source_tree_root, options, &console)?; + let lab_outcome = lab::test_unmutated_then_all_mutants( + &tool, + discovered, + &source_tree_root, + options, + &console, + )?; exit(lab_outcome.exit_code()); } Ok(()) From e2bcd210f625b4f9aac1fa55371ec15002f3194e Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 4 Nov 2023 11:22:18 -0700 Subject: [PATCH 05/36] Template by Tool type At present it's always Cargo so not much value in using a pointer --- src/lab.rs | 8 ++++---- src/visit.rs | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 27861b28..7604f939 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -25,8 +25,8 @@ use crate::*; /// /// Before testing the mutants, the lab checks that the source tree passes its tests with no /// mutations applied. -pub fn test_unmutated_then_all_mutants( - tool: &dyn Tool, +pub fn test_unmutated_then_all_mutants( + tool: &T, discovered: Discovered, source_tree: &Utf8Path, options: Options, @@ -177,8 +177,8 @@ pub fn test_unmutated_then_all_mutants( )] // Yes, it's a lot of arguments, but it does use them all and I don't think creating objects // just to group them would help... -fn test_scenario( - tool: &dyn Tool, +fn test_scenario( + tool: &T, build_dir: &mut BuildDir, output_mutex: &Mutex, options: &Options, diff --git a/src/visit.rs b/src/visit.rs index 6319859e..017c9f8d 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -34,8 +34,8 @@ pub struct Discovered { /// Discover all mutants and all source files. /// /// The list of source files includes even those with no mutants. -pub fn walk_tree( - tool: &dyn Tool, +pub fn walk_tree( + tool: &T, root: &Utf8Path, options: &Options, console: &Console, From e191bfa15b4d1d5c95223826a42360fe6278860a Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 4 Nov 2023 11:28:38 -0700 Subject: [PATCH 06/36] Clean up --- src/lab.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 7604f939..4799b938 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -2,12 +2,12 @@ //! Successively apply mutations to the source code and run cargo to check, build, and test them. -use std::cmp::max; +use std::cmp::{max, min}; use std::sync::Mutex; use std::thread; use std::time::{Duration, Instant}; -use anyhow::{anyhow, Context, Result}; +use anyhow::{ensure, Context, Result}; use itertools::Itertools; use tracing::warn; #[allow(unused)] @@ -46,9 +46,7 @@ pub fn test_unmutated_then_all_mutants( } output_dir.write_mutants_list(&mutants)?; console.discovered_mutants(&mutants); - if mutants.is_empty() { - return Err(anyhow!("No mutants found")); - } + ensure!(!mutants.is_empty(), "No mutants found"); let all_packages = mutants.iter().map(|m| m.package()).unique().collect_vec(); let output_mutex = Mutex::new(output_dir); @@ -100,7 +98,7 @@ pub fn test_unmutated_then_all_mutants( Duration::MAX }; - let jobs = std::cmp::max(1, std::cmp::min(options.jobs.unwrap_or(1), mutants.len())); + let jobs = max(1, min(options.jobs.unwrap_or(1), mutants.len())); console.build_dirs_start(jobs - 1); for i in 1..jobs { debug!("copy build dir {i}"); @@ -183,7 +181,7 @@ fn test_scenario( output_mutex: &Mutex, options: &Options, scenario: &Scenario, - packages: &[&Package], + test_packages: &[&Package], test_timeout: Duration, console: &Console, ) -> Result { @@ -212,7 +210,7 @@ fn test_scenario( Phase::Test => test_timeout, _ => Duration::MAX, }; - let argv = tool.compose_argv(build_dir, Some(packages), phase, options)?; + let argv = tool.compose_argv(build_dir, Some(test_packages), phase, options)?; let env = tool.compose_env()?; let process_status = Process::run( &argv, From 18ca888b5c70eaa32e2190f3aa7861651cbe7c8e Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sat, 4 Nov 2023 11:57:36 -0700 Subject: [PATCH 07/36] Add --package option --- DESIGN.md | 2 ++ NEWS.md | 2 ++ book/src/workspaces.md | 2 ++ src/cargo.rs | 60 ++++++++++++++++++++++++++++++++++++++++-- src/main.rs | 12 ++++++++- src/mutate.rs | 26 +++++++++++++----- src/tool.rs | 8 +++++- src/visit.rs | 8 ++++-- 8 files changed, 107 insertions(+), 13 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index a17875e7..4265a23c 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -67,6 +67,8 @@ A source tree may be a single crate or a Cargo workspace. There are several leve * Each source file contains some functions. * For each function we generate some mutants. +At the discovery stage we can filter to only some packages of interest using the `--package` option. + The name of the containing package is passed through to the `SourceFile` and the `Mutant` objects. For source tree and baseline builds and tests, we pass Cargo `--workspace` to build and test everything. For mutant builds and tests, we pass `--package` to build and test only the package containing the mutant, on the assumption that each mutant should be caught by its own package's tests. diff --git a/NEWS.md b/NEWS.md index c499d6e3..bc7e73e3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -4,6 +4,8 @@ - New: generate key-value map values. +- New: `--package` option tests only mutants from that package. + ## 23.10.0 - The baseline test (with no mutants) now tests only the packages in which diff --git a/book/src/workspaces.md b/book/src/workspaces.md index 2afb4bcc..b70cf40d 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -6,6 +6,8 @@ By default, all source files in all packages in the workspace are tested. **NOTE: This behavior is likely to change in future: see .** +With the `--package` option, only mutants from the package with the given name are testeg. The effect can be seen in `--list`, etc. This option can be repeated. + You can 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 diff --git a/src/cargo.rs b/src/cargo.rs index 58d74783..ff6dfe76 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -83,7 +83,11 @@ impl Tool for CargoTool { /// After this, there is one more level of discovery, by walking those root files /// to find `mod` statements, and then recursively walking those files to find /// all source files. - fn top_source_files(&self, source_root_path: &Utf8Path) -> Result>> { + fn top_source_files( + &self, + source_root_path: &Utf8Path, + packages: &[String], + ) -> Result>> { let cargo_toml_path = source_root_path.join("Cargo.toml"); debug!(?cargo_toml_path, ?source_root_path, "Find root files"); check_interrupted()?; @@ -97,6 +101,7 @@ impl Tool for CargoTool { for package_metadata in metadata .workspace_packages() .iter() + .filter(|p| packages.is_empty() || packages.contains(&p.name)) .sorted_by_key(|p| &p.name) { check_interrupted()?; @@ -391,7 +396,9 @@ mod test { let root_dir = tool .find_root(Utf8Path::new("testdata/tree/workspace/main")) .expect("Find workspace root"); - let top_source_files = tool.top_source_files(&root_dir).expect("Find root files"); + let top_source_files = tool + .top_source_files(&root_dir, &[]) + .expect("Find root files"); println!("{top_source_files:#?}"); let paths = top_source_files .iter() @@ -404,4 +411,53 @@ mod test { ["utils/src/lib.rs", "main/src/main.rs", "main2/src/main.rs"] ); } + + #[test] + fn filter_by_single_package() { + let tool = CargoTool::new(); + let root_dir = tool + .find_root(Utf8Path::new("testdata/tree/workspace/main")) + .expect("Find workspace root"); + assert_eq!( + root_dir.file_name(), + Some("workspace"), + "found the workspace root" + ); + let top_source_files = tool + .top_source_files(&root_dir, &["main".to_owned()]) + .expect("Find root files"); + println!("{top_source_files:#?}"); + assert_eq!(top_source_files.len(), 1); + assert_eq!( + top_source_files + .iter() + .map(|sf| sf.tree_relative_path.clone()) + .collect_vec(), + ["main/src/main.rs"] + ); + } + + #[test] + fn filter_by_multiple_packages() { + let tool = CargoTool::new(); + let root_dir = tool + .find_root(Utf8Path::new("testdata/tree/workspace/main")) + .expect("Find workspace root"); + assert_eq!( + root_dir.file_name(), + Some("workspace"), + "found the workspace root" + ); + let top_source_files = tool + .top_source_files(&root_dir, &["main".to_owned(), "main2".to_owned()]) + .expect("Find root files"); + println!("{top_source_files:#?}"); + assert_eq!( + top_source_files + .iter() + .map(|sf| sf.tree_relative_path.clone()) + .collect_vec(), + ["main/src/main.rs", "main2/src/main.rs"] + ); + } } diff --git a/src/main.rs b/src/main.rs index 01cbfd5c..bd5a3fd3 100644 --- a/src/main.rs +++ b/src/main.rs @@ -164,6 +164,10 @@ struct Args { #[arg(long, short = 'o')] output: Option, + /// only test mutants from these packages. + #[arg(id = "package", long, short = 'p')] + mutate_packages: Vec, + /// run mutants in random order. #[arg(long)] shuffle: bool, @@ -235,7 +239,13 @@ fn main() -> Result<()> { debug!(?config); let options = Options::new(&args, &config)?; debug!(?options); - let discovered = walk_tree(&tool, &source_tree_root, &options, &console)?; + let discovered = walk_tree( + &tool, + &source_tree_root, + &args.mutate_packages, + &options, + &console, + )?; if args.list_files { console.clear(); list_files(FmtToIoWrite::new(io::stdout()), discovered, &options)?; diff --git a/src/mutate.rs b/src/mutate.rs index eb427791..e566d048 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -211,7 +211,7 @@ mod test { let tool = CargoTool::new(); let source_tree = tool.find_root(tree_path).unwrap(); let options = Options::default(); - let mutants = walk_tree(&tool, &source_tree, &options, &Console::new()) + let mutants = walk_tree(&tool, &source_tree, &[], &options, &Console::new()) .unwrap() .mutants; assert_eq!(mutants.len(), 3); @@ -265,9 +265,15 @@ mod test { let tree_path = Utf8Path::new("testdata/tree/hang_avoided_by_attr"); let tool = CargoTool::new(); let source_tree = tool.find_root(tree_path).unwrap(); - let mutants = walk_tree(&tool, &source_tree, &Options::default(), &Console::new()) - .unwrap() - .mutants; + let mutants = walk_tree( + &tool, + &source_tree, + &[], + &Options::default(), + &Console::new(), + ) + .unwrap() + .mutants; let descriptions = mutants.iter().map(Mutant::describe_change).collect_vec(); insta::assert_snapshot!( descriptions.join("\n"), @@ -280,9 +286,15 @@ mod test { let tree_path = Utf8Path::new("testdata/tree/factorial"); let tool = CargoTool::new(); let source_tree = tool.find_root(tree_path).unwrap(); - let mutants = walk_tree(&tool, &source_tree, &Options::default(), &Console::new()) - .unwrap() - .mutants; + let mutants = walk_tree( + &tool, + &source_tree, + &[], + &Options::default(), + &Console::new(), + ) + .unwrap() + .mutants; assert_eq!(mutants.len(), 3); let mut mutated_code = mutants[0].mutated_code(); diff --git a/src/tool.rs b/src/tool.rs index 96cdbcfd..73d5a50c 100644 --- a/src/tool.rs +++ b/src/tool.rs @@ -42,7 +42,13 @@ pub trait Tool: Debug + Send + Sync { /// /// From each of these top files, we can discover more source by following `mod` /// statements. - fn top_source_files(&self, path: &Utf8Path) -> Result>>; + /// + /// If `packages` is non-empty, include only packages whose name is in this list. + fn top_source_files( + &self, + path: &Utf8Path, + packages: &[String], + ) -> Result>>; /// Compose argv to run one phase in this tool. fn compose_argv( diff --git a/src/visit.rs b/src/visit.rs index 017c9f8d..198cbbc7 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -34,9 +34,12 @@ pub struct Discovered { /// Discover all mutants and all source files. /// /// The list of source files includes even those with no mutants. +/// +/// `mutate_packages`: If non-empty, only generate mutants from these packages. pub fn walk_tree( tool: &T, root: &Utf8Path, + mutate_packages: &[String], options: &Options, console: &Console, ) -> Result { @@ -46,9 +49,10 @@ pub fn walk_tree( .map(|e| syn::parse_str(e).with_context(|| format!("Failed to parse error value {e:?}"))) .collect::>>()?; console.walk_tree_start(); + let mut file_queue: VecDeque> = + tool.top_source_files(root, mutate_packages)?.into(); let mut mutants = Vec::new(); let mut files: Vec> = Vec::new(); - let mut file_queue: VecDeque> = tool.top_source_files(root)?.into(); while let Some(source_file) = file_queue.pop_front() { console.walk_tree_update(files.len(), mutants.len()); check_interrupted()?; @@ -415,7 +419,7 @@ mod test { .canonicalize_utf8() .expect("Canonicalize source path"); let console = Console::new(); - let discovered = walk_tree(&CargoTool::new(), source_tree_root, &options, &console) + let discovered = walk_tree(&CargoTool::new(), source_tree_root, &[], &options, &console) .expect("Discover mutants"); crate::list_mutants(&mut list_output, discovered, &options) .expect("Discover mutants in own source tree"); From d425e945daadbf74654b7de441d4844d0af4f09a Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 11:50:13 -0800 Subject: [PATCH 08/36] More design detail on workspaces --- DESIGN.md | 60 ++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 16 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 4265a23c..0d10c4d2 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -50,33 +50,61 @@ the content based on those addresses. replacements. The interface to the `syn` parser is localized here, and also the core of cargo-mutants logic to guess at valid replacements. -## Relative dependencies +## Major processing stages -After copying the tree, cargo-mutants scans the top-level `Cargo.toml` and any -`.cargo/config.toml` for relative dependencies. If there are any, the paths are -rewritten to be absolute, so that they still work when cargo is run in the -scratch directory. +1. Find the workspace enclosing the start directory, and the packages within it. +1. Determine which packages to mutate. +1. Generate mutants by walking each package. +1. Copy the source tree. +1. Run baseline tests. +1. Test each mutant in parallel. + +### Finding the workspace and packages + +cargo-mutants is invoked from within, or given with `-d`, a single directory. To find mutants and run tests we first need to find the enclosing workspace and the packages within it. + +This is done basically by parsing the output of `cargo metadata`. + +For each package, cargo tells us the build targets including tests and the main library or binary. The tests are not considered for mutation, so this leaves us with +some targets of interest, and for each of them cargo tells us one top source file, typically something like `src/lib.rs` or `src/main.rs`. + +### Finding packages to mutate + +We may often want to test only one or a subset of packages in the workspace. -## Enumerating the source tree +This can be controlled by an explicit `--package` option. -A source tree may be a single crate or a Cargo workspace. There are several levels of nesting: +**UNIMPLEMENTED:** In non-workspace directories test only that one package. Match Cargo's heuristics for running tests from a workspace; add a `--workspace` option. -* A workspace contains one or more packages. -* A package contains one or more targets. -* Each target names one (or possibly-more) top level source files, whose directories are walked to find more source files. -* Each source file contains some functions. -* For each function we generate some mutants. +### Discovering mutants -At the discovery stage we can filter to only some packages of interest using the `--package` option. +After discovering packages and before running any tests, we discover all the potential mutants. -The name of the containing package is passed through to the `SourceFile` and the `Mutant` objects. +Starting from the top files for each package, we parse each source file using `syn` +and then walk its AST. In the course of that walk we can find three broad categories of patterns: -For source tree and baseline builds and tests, we pass Cargo `--workspace` to build and test everything. For mutant builds and tests, we pass `--package` to build and test only the package containing the mutant, on the assumption that each mutant should be caught by its own package's tests. +* A `mod` statement (without a block), which tells us of another source file we must remember + to walk. +* A source pattern that cargo-mutants knows how to mutate, such as a function returning a value. +* A pattern that tells cargo-mutants not to look further into this branch of the tree, such as `#[test]` or `#[mutants::skip]`. -Currently source files are discovered by finding any `bin` or `lib` targets in the package, then taking every `*.rs` file in the same directory as their top-level source file. (This is a bit approximate and will pick up files that might not actually be referenced by a `mod` statement, so may change in the future.) +For baseline builds and tests, we pass Cargo `--workspace` to build and test everything. For mutant builds and tests, we pass `--package` to build and test only the package containing the mutant, on the assumption that each mutant should be caught by its own package's tests. We may later mutate at a granularity smaller than a single function, for example by cutting out an `if` statement or a loop, but that is not yet implemented. () +### Copying the source tree + +Mutations are tested in copies of the source tree. (An option could be added to test in-place, which would be nice for CI.) + +Initially, one copy is made to run baseline tests; if they succeed then additional copies are made as necessary for each parallel job. + +After copying the tree, cargo-mutants scans the top-level `Cargo.toml` and any +`.cargo/config.toml` for relative dependencies. If there are any, the paths are +rewritten to be absolute, so that they still work when cargo is run in the +scratch directory. + +Currently, the whole workspace tree is copied. In future, possibly only the package to be mutated could be copied: this would require changes to the code that fixes up dependencies. + ## Handling timeouts Mutations can cause a program to go into an infinite (or just very long) loop: From 71281f65b31bed119bdb0352f66d24917ed948f1 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 11:50:20 -0800 Subject: [PATCH 09/36] Un-abstract find_workspace --- src/build_dir.rs | 4 +- src/cargo.rs | 75 +++++++++---------- src/config.rs | 4 +- src/lab.rs | 6 +- src/main.rs | 8 +- src/mutate.rs | 6 +- src/output.rs | 2 +- ..._expected_mutants_for_own_source_tree.snap | 4 +- src/tool.rs | 11 +-- src/visit.rs | 11 +-- 10 files changed, 58 insertions(+), 73 deletions(-) diff --git a/src/build_dir.rs b/src/build_dir.rs index 26038ed9..a2338966 100644 --- a/src/build_dir.rs +++ b/src/build_dir.rs @@ -152,9 +152,7 @@ mod test { #[test] fn build_dir_debug_form() { let options = Options::default(); - let root = CargoTool::new() - .find_root("testdata/tree/factorial".into()) - .unwrap(); + let root = cargo::find_workspace("testdata/tree/factorial".into()).unwrap(); let build_dir = BuildDir::new(&root, &options, &Console::new()).unwrap(); let debug_form = format!("{build_dir:?}"); assert!( diff --git a/src/cargo.rs b/src/cargo.rs index ff6dfe76..73cded02 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -42,35 +42,6 @@ impl Tool for CargoTool { "cargo" } - fn find_root(&self, path: &Utf8Path) -> Result { - ensure!(path.is_dir(), "{path:?} is not a directory"); - let cargo_bin = cargo_bin(); // needed for lifetime - let argv: Vec<&str> = vec![&cargo_bin, "locate-project", "--workspace"]; - let stdout = get_command_output(&argv, path) - .with_context(|| format!("run cargo locate-project in {path:?}"))?; - let val: Value = - serde_json::from_str(&stdout).context("parse cargo locate-project output")?; - let cargo_toml_path: Utf8PathBuf = val["root"] - .as_str() - .with_context(|| format!("cargo locate-project output has no root: {stdout:?}"))? - .to_owned() - .into(); - debug!(?cargo_toml_path, "Found workspace root manifest"); - ensure!( - cargo_toml_path.is_file(), - "cargo locate-project root {cargo_toml_path:?} is not a file" - ); - let root = cargo_toml_path - .parent() - .ok_or_else(|| anyhow!("cargo locate-project root {cargo_toml_path:?} has no parent"))? - .to_owned(); - ensure!( - root.is_dir(), - "apparent project root directory {root:?} is not a directory" - ); - Ok(root) - } - /// Find the root files for each relevant package in the source tree. /// /// A source tree might include multiple packages (e.g. in a Cargo workspace), @@ -148,6 +119,35 @@ impl Tool for CargoTool { } } +/// Return the path of the workspace directory enclosing a given directory. +pub fn find_workspace(path: &Utf8Path) -> Result { + ensure!(path.is_dir(), "{path:?} is not a directory"); + let cargo_bin = cargo_bin(); // needed for lifetime + let argv: Vec<&str> = vec![&cargo_bin, "locate-project", "--workspace"]; + let stdout = get_command_output(&argv, path) + .with_context(|| format!("run cargo locate-project in {path:?}"))?; + let val: Value = serde_json::from_str(&stdout).context("parse cargo locate-project output")?; + let cargo_toml_path: Utf8PathBuf = val["root"] + .as_str() + .with_context(|| format!("cargo locate-project output has no root: {stdout:?}"))? + .to_owned() + .into(); + debug!(?cargo_toml_path, "Found workspace root manifest"); + ensure!( + cargo_toml_path.is_file(), + "cargo locate-project root {cargo_toml_path:?} is not a file" + ); + let root = cargo_toml_path + .parent() + .ok_or_else(|| anyhow!("cargo locate-project root {cargo_toml_path:?} has no parent"))? + .to_owned(); + ensure!( + root.is_dir(), + "apparent project root directory {root:?} is not a directory" + ); + Ok(root) +} + /// Return the name of the cargo binary. fn cargo_bin() -> String { // When run as a Cargo subcommand, which is the usual/intended case, @@ -368,13 +368,12 @@ mod test { #[test] fn error_opening_outside_of_crate() { - CargoTool::new().find_root(Utf8Path::new("/")).unwrap_err(); + cargo::find_workspace(Utf8Path::new("/")).unwrap_err(); } #[test] fn open_subdirectory_of_crate_opens_the_crate() { - let root = CargoTool::new() - .find_root(Utf8Path::new("testdata/tree/factorial/src")) + let root = cargo::find_workspace(Utf8Path::new("testdata/tree/factorial/src")) .expect("open source tree from subdirectory"); assert!(root.is_dir()); assert!(root.join("Cargo.toml").is_file()); @@ -384,8 +383,7 @@ mod test { #[test] fn find_root_from_subdirectory_of_workspace_finds_the_workspace_root() { - let root = CargoTool::new() - .find_root(Utf8Path::new("testdata/tree/workspace/main")) + let root = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) .expect("Find root from within workspace/main"); assert_eq!(root.file_name(), Some("workspace"), "Wrong root: {root:?}"); } @@ -393,8 +391,7 @@ mod test { #[test] fn find_top_source_files_from_subdirectory_of_workspace() { let tool = CargoTool::new(); - let root_dir = tool - .find_root(Utf8Path::new("testdata/tree/workspace/main")) + let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) .expect("Find workspace root"); let top_source_files = tool .top_source_files(&root_dir, &[]) @@ -415,8 +412,7 @@ mod test { #[test] fn filter_by_single_package() { let tool = CargoTool::new(); - let root_dir = tool - .find_root(Utf8Path::new("testdata/tree/workspace/main")) + let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) .expect("Find workspace root"); assert_eq!( root_dir.file_name(), @@ -440,8 +436,7 @@ mod test { #[test] fn filter_by_multiple_packages() { let tool = CargoTool::new(); - let root_dir = tool - .find_root(Utf8Path::new("testdata/tree/workspace/main")) + let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) .expect("Find workspace root"); assert_eq!( root_dir.file_name(), diff --git a/src/config.rs b/src/config.rs index 620c573e..efcd4d62 100644 --- a/src/config.rs +++ b/src/config.rs @@ -50,8 +50,8 @@ impl Config { /// Read the config from a tree's `.cargo/mutants.toml`, and return a default (empty) /// Config is the file does not exist. - pub fn read_tree_config(source_tree_root: &Utf8Path) -> Result { - let path = source_tree_root.join(".cargo").join("mutants.toml"); + pub fn read_tree_config(workspace_dir: &Utf8Path) -> Result { + let path = workspace_dir.join(".cargo").join("mutants.toml"); if path.exists() { Config::read_file(&path) } else { diff --git a/src/lab.rs b/src/lab.rs index 4799b938..5d62c8c5 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -28,7 +28,7 @@ use crate::*; pub fn test_unmutated_then_all_mutants( tool: &T, discovered: Discovered, - source_tree: &Utf8Path, + workspace_dir: &Utf8Path, options: Options, console: &Console, ) -> Result { @@ -36,7 +36,7 @@ pub fn test_unmutated_then_all_mutants( let output_in_dir: &Utf8Path = options .output_in_dir .as_ref() - .map_or(source_tree, |p| p.as_path()); + .map_or(workspace_dir, |p| p.as_path()); let output_dir = OutputDir::new(output_in_dir)?; console.set_debug_log(output_dir.open_debug_log()?); let mut mutants = discovered.mutants; @@ -50,7 +50,7 @@ pub fn test_unmutated_then_all_mutants( 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)?]; + let mut build_dirs = vec![BuildDir::new(workspace_dir, &options, console)?]; let baseline_outcome = { let _span = debug_span!("baseline").entered(); test_scenario( diff --git a/src/main.rs b/src/main.rs index bd5a3fd3..f58c8468 100644 --- a/src/main.rs +++ b/src/main.rs @@ -230,18 +230,18 @@ fn main() -> Result<()> { Utf8Path::new(".") }; let tool = CargoTool::new(); - let source_tree_root = tool.find_root(source_path)?; + let workspace_dir = cargo::find_workspace(source_path)?; let config = if args.no_config { config::Config::default() } else { - config::Config::read_tree_config(&source_tree_root)? + config::Config::read_tree_config(&workspace_dir)? }; debug!(?config); let options = Options::new(&args, &config)?; debug!(?options); let discovered = walk_tree( &tool, - &source_tree_root, + &workspace_dir, &args.mutate_packages, &options, &console, @@ -256,7 +256,7 @@ fn main() -> Result<()> { let lab_outcome = lab::test_unmutated_then_all_mutants( &tool, discovered, - &source_tree_root, + &workspace_dir, options, &console, )?; diff --git a/src/mutate.rs b/src/mutate.rs index e566d048..9b3bb72e 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -209,7 +209,7 @@ mod test { fn discover_factorial_mutants() { let tree_path = Utf8Path::new("testdata/tree/factorial"); let tool = CargoTool::new(); - let source_tree = tool.find_root(tree_path).unwrap(); + let source_tree = cargo::find_workspace(tree_path).unwrap(); let options = Options::default(); let mutants = walk_tree(&tool, &source_tree, &[], &options, &Console::new()) .unwrap() @@ -264,7 +264,7 @@ mod test { fn filter_by_attributes() { let tree_path = Utf8Path::new("testdata/tree/hang_avoided_by_attr"); let tool = CargoTool::new(); - let source_tree = tool.find_root(tree_path).unwrap(); + let source_tree = cargo::find_workspace(tree_path).unwrap(); let mutants = walk_tree( &tool, &source_tree, @@ -285,7 +285,7 @@ mod test { fn mutate_factorial() { let tree_path = Utf8Path::new("testdata/tree/factorial"); let tool = CargoTool::new(); - let source_tree = tool.find_root(tree_path).unwrap(); + let source_tree = cargo::find_workspace(tree_path).unwrap(); let mutants = walk_tree( &tool, &source_tree, diff --git a/src/output.rs b/src/output.rs index 60716529..611da8b8 100644 --- a/src/output.rs +++ b/src/output.rs @@ -280,7 +280,7 @@ mod test { fn create_output_dir() { let tmp = minimal_source_tree(); let tmp_path = tmp.path().try_into().unwrap(); - let root = CargoTool::new().find_root(tmp_path).unwrap(); + let root = cargo::find_workspace(tmp_path).unwrap(); let output_dir = OutputDir::new(&root).unwrap(); assert_eq!( list_recursive(tmp.path()), 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 9689f8d3..75e8ee69 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 @@ -13,8 +13,6 @@ src/build_dir.rs: replace copy_tree -> Result with Ok(Default::default( src/build_dir.rs: replace copy_tree -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace ::name -> &str with "" src/cargo.rs: replace ::name -> &str with "xyzzy" -src/cargo.rs: replace ::find_root -> Result with Ok(Default::default()) -src/cargo.rs: replace ::find_root -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace ::top_source_files -> Result>> with Ok(vec![]) src/cargo.rs: replace ::top_source_files -> Result>> with Ok(vec![Arc::new(Default::default())]) src/cargo.rs: replace ::top_source_files -> Result>> with Err(::anyhow::anyhow!("mutated!")) @@ -28,6 +26,8 @@ src/cargo.rs: replace ::compose_env -> Result::compose_env -> Result> with Ok(vec![("xyzzy".into(), String::new())]) src/cargo.rs: replace ::compose_env -> Result> with Ok(vec![("xyzzy".into(), "xyzzy".into())]) src/cargo.rs: replace ::compose_env -> Result> with Err(::anyhow::anyhow!("mutated!")) +src/cargo.rs: replace find_workspace -> Result with Ok(Default::default()) +src/cargo.rs: replace find_workspace -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace cargo_bin -> String with String::new() src/cargo.rs: replace cargo_bin -> String with "xyzzy".into() src/cargo.rs: replace cargo_argv -> Vec with vec![] diff --git a/src/tool.rs b/src/tool.rs index 73d5a50c..69a3ebeb 100644 --- a/src/tool.rs +++ b/src/tool.rs @@ -10,7 +10,7 @@ use std::fmt::Debug; use std::marker::{Send, Sync}; use std::sync::Arc; -use camino::{Utf8Path, Utf8PathBuf}; +use camino::Utf8Path; #[allow(unused_imports)] use tracing::{debug, debug_span, trace}; @@ -24,15 +24,6 @@ pub trait Tool: Debug + Send + Sync { /// A short name for this tool, like "cargo". fn name(&self) -> &str; - /// Find the root of the source tree enclosing a given path. - /// - /// The root is the enclosing directory that needs to be copied to make a self-contained - /// scratch directory, and from where source discovery begins. - /// - /// This may include more directories than will actually be tested, sufficient to allow - /// the build to work. For Cargo, we copy the whole workspace. - fn find_root(&self, path: &Utf8Path) -> Result; - /// Find the top-level files for each package within a tree. /// /// The path is the root returned by [find_root]. diff --git a/src/visit.rs b/src/visit.rs index 198cbbc7..8967302c 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -38,7 +38,7 @@ pub struct Discovered { /// `mutate_packages`: If non-empty, only generate mutants from these packages. pub fn walk_tree( tool: &T, - root: &Utf8Path, + workspace_dir: &Utf8Path, mutate_packages: &[String], options: &Options, console: &Console, @@ -49,8 +49,9 @@ pub fn walk_tree( .map(|e| syn::parse_str(e).with_context(|| format!("Failed to parse error value {e:?}"))) .collect::>>()?; console.walk_tree_start(); - let mut file_queue: VecDeque> = - tool.top_source_files(root, mutate_packages)?.into(); + let mut file_queue: VecDeque> = tool + .top_source_files(workspace_dir, mutate_packages)? + .into(); let mut mutants = Vec::new(); let mut files: Vec> = Vec::new(); while let Some(source_file) = file_queue.pop_front() { @@ -62,9 +63,9 @@ pub fn walk_tree( // collect any mutants from them, and they don't count as "seen" for // `--list-files`. for mod_name in &external_mods { - if let Some(mod_path) = find_mod_source(root, &source_file, mod_name)? { + if let Some(mod_path) = find_mod_source(workspace_dir, &source_file, mod_name)? { file_queue.push_back(Arc::new(SourceFile::new( - root, + workspace_dir, mod_path, &source_file.package, )?)) From eb10fcc20208a6433089085257bca939642bf965 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 11:59:17 -0800 Subject: [PATCH 10/36] Un-abstract top_source_files --- src/cargo.rs | 143 +++++++++--------- src/main.rs | 8 +- src/mutate.rs | 23 +-- ..._expected_mutants_for_own_source_tree.snap | 6 +- src/tool.rs | 20 --- src/visit.rs | 17 +-- 6 files changed, 86 insertions(+), 131 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index 73cded02..6c63d9b0 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -42,68 +42,6 @@ impl Tool for CargoTool { "cargo" } - /// Find the root files for each relevant package in the source tree. - /// - /// A source tree might include multiple packages (e.g. in a Cargo workspace), - /// and each package might have multiple targets (e.g. a bin and lib). Test targets - /// are excluded here: we run them, but we don't mutate them. - /// - /// Each target has one root file, typically but not necessarily called `src/lib.rs` - /// or `src/main.rs`. This function returns a list of all those files. - /// - /// After this, there is one more level of discovery, by walking those root files - /// to find `mod` statements, and then recursively walking those files to find - /// all source files. - fn top_source_files( - &self, - source_root_path: &Utf8Path, - packages: &[String], - ) -> Result>> { - let cargo_toml_path = source_root_path.join("Cargo.toml"); - debug!(?cargo_toml_path, ?source_root_path, "Find root files"); - check_interrupted()?; - let metadata = cargo_metadata::MetadataCommand::new() - .manifest_path(&cargo_toml_path) - .exec() - .context("run cargo metadata")?; - - let mut r = Vec::new(); - // cargo-metadata output is not obviously ordered so make it deterministic. - for package_metadata in metadata - .workspace_packages() - .iter() - .filter(|p| packages.is_empty() || packages.contains(&p.name)) - .sorted_by_key(|p| &p.name) - { - check_interrupted()?; - let _span = debug_span!("package", name = %package_metadata.name).entered(); - let manifest_path = &package_metadata.manifest_path; - debug!(%manifest_path, "walk package"); - let relative_manifest_path = manifest_path - .strip_prefix(source_root_path) - .map_err(|_| { - anyhow!( - "manifest path {manifest_path:?} for package {name:?} is not within the detected source root path {source_root_path:?}", - name = package_metadata.name - ) - })? - .to_owned(); - let package = Arc::new(Package { - name: package_metadata.name.clone(), - relative_manifest_path, - }); - for source_path in direct_package_sources(source_root_path, package_metadata)? { - check_interrupted()?; - r.push(Arc::new(SourceFile::new( - source_root_path, - source_path, - &package, - )?)); - } - } - Ok(r) - } - fn compose_argv( &self, build_dir: &BuildDir, @@ -148,6 +86,69 @@ pub fn find_workspace(path: &Utf8Path) -> Result { Ok(root) } +/// Find the root files for each relevant package in the source tree. +/// +/// A source tree might include multiple packages (e.g. in a Cargo workspace), +/// and each package might have multiple targets (e.g. a bin and lib). Test targets +/// are excluded here: we run them, but we don't mutate them. +/// +/// Each target has one root file, typically but not necessarily called `src/lib.rs` +/// or `src/main.rs`. This function returns a list of all those files. +/// +/// After this, there is one more level of discovery, by walking those root files +/// to find `mod` statements, and then recursively walking those files to find +/// all source files. +/// +/// Packages are only included if their name is in `include_packages`. +pub fn top_source_files( + workspace_dir: &Utf8Path, + include_packages: &[String], +) -> Result>> { + let cargo_toml_path = workspace_dir.join("Cargo.toml"); + debug!(?cargo_toml_path, ?workspace_dir, "Find root files"); + check_interrupted()?; + let metadata = cargo_metadata::MetadataCommand::new() + .manifest_path(&cargo_toml_path) + .exec() + .context("run cargo metadata")?; + + let mut r = Vec::new(); + // cargo-metadata output is not obviously ordered so make it deterministic. + for package_metadata in metadata + .workspace_packages() + .iter() + .filter(|p| include_packages.is_empty() || include_packages.contains(&p.name)) + .sorted_by_key(|p| &p.name) + { + check_interrupted()?; + let _span = debug_span!("package", name = %package_metadata.name).entered(); + let manifest_path = &package_metadata.manifest_path; + debug!(%manifest_path, "walk package"); + let relative_manifest_path = manifest_path + .strip_prefix(workspace_dir) + .map_err(|_| { + anyhow!( + "manifest path {manifest_path:?} for package {name:?} is not within the detected source root path {workspace_dir:?}", + name = package_metadata.name + ) + })? + .to_owned(); + let package = Arc::new(Package { + name: package_metadata.name.clone(), + relative_manifest_path, + }); + for source_path in direct_package_sources(workspace_dir, package_metadata)? { + check_interrupted()?; + r.push(Arc::new(SourceFile::new( + workspace_dir, + source_path, + &package, + )?)); + } + } + Ok(r) +} + /// Return the name of the cargo binary. fn cargo_bin() -> String { // When run as a Cargo subcommand, which is the usual/intended case, @@ -390,12 +391,9 @@ mod test { #[test] fn find_top_source_files_from_subdirectory_of_workspace() { - let tool = CargoTool::new(); let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) .expect("Find workspace root"); - let top_source_files = tool - .top_source_files(&root_dir, &[]) - .expect("Find root files"); + let top_source_files = top_source_files(&root_dir, &[]).expect("Find root files"); println!("{top_source_files:#?}"); let paths = top_source_files .iter() @@ -411,7 +409,6 @@ mod test { #[test] fn filter_by_single_package() { - let tool = CargoTool::new(); let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) .expect("Find workspace root"); assert_eq!( @@ -419,9 +416,8 @@ mod test { Some("workspace"), "found the workspace root" ); - let top_source_files = tool - .top_source_files(&root_dir, &["main".to_owned()]) - .expect("Find root files"); + let top_source_files = + top_source_files(&root_dir, &["main".to_owned()]).expect("Find root files"); println!("{top_source_files:#?}"); assert_eq!(top_source_files.len(), 1); assert_eq!( @@ -435,7 +431,6 @@ mod test { #[test] fn filter_by_multiple_packages() { - let tool = CargoTool::new(); let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) .expect("Find workspace root"); assert_eq!( @@ -443,9 +438,9 @@ mod test { Some("workspace"), "found the workspace root" ); - let top_source_files = tool - .top_source_files(&root_dir, &["main".to_owned(), "main2".to_owned()]) - .expect("Find root files"); + let top_source_files = + top_source_files(&root_dir, &["main".to_owned(), "main2".to_owned()]) + .expect("Find root files"); println!("{top_source_files:#?}"); assert_eq!( top_source_files diff --git a/src/main.rs b/src/main.rs index f58c8468..f9950547 100644 --- a/src/main.rs +++ b/src/main.rs @@ -239,13 +239,7 @@ fn main() -> Result<()> { debug!(?config); let options = Options::new(&args, &config)?; debug!(?options); - let discovered = walk_tree( - &tool, - &workspace_dir, - &args.mutate_packages, - &options, - &console, - )?; + let discovered = walk_tree(&workspace_dir, &args.mutate_packages, &options, &console)?; if args.list_files { console.clear(); list_files(FmtToIoWrite::new(io::stdout()), discovered, &options)?; diff --git a/src/mutate.rs b/src/mutate.rs index 9b3bb72e..d804c52c 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -208,10 +208,9 @@ mod test { #[test] fn discover_factorial_mutants() { let tree_path = Utf8Path::new("testdata/tree/factorial"); - let tool = CargoTool::new(); - let source_tree = cargo::find_workspace(tree_path).unwrap(); + let workspace_dir = cargo::find_workspace(tree_path).unwrap(); let options = Options::default(); - let mutants = walk_tree(&tool, &source_tree, &[], &options, &Console::new()) + let mutants = walk_tree(&workspace_dir, &[], &options, &Console::new()) .unwrap() .mutants; assert_eq!(mutants.len(), 3); @@ -263,11 +262,8 @@ mod test { #[test] fn filter_by_attributes() { let tree_path = Utf8Path::new("testdata/tree/hang_avoided_by_attr"); - let tool = CargoTool::new(); - let source_tree = cargo::find_workspace(tree_path).unwrap(); let mutants = walk_tree( - &tool, - &source_tree, + &cargo::find_workspace(tree_path).unwrap(), &[], &Options::default(), &Console::new(), @@ -284,17 +280,10 @@ mod test { #[test] fn mutate_factorial() { let tree_path = Utf8Path::new("testdata/tree/factorial"); - let tool = CargoTool::new(); let source_tree = cargo::find_workspace(tree_path).unwrap(); - let mutants = walk_tree( - &tool, - &source_tree, - &[], - &Options::default(), - &Console::new(), - ) - .unwrap() - .mutants; + let mutants = walk_tree(&source_tree, &[], &Options::default(), &Console::new()) + .unwrap() + .mutants; assert_eq!(mutants.len(), 3); let mut mutated_code = mutants[0].mutated_code(); 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 75e8ee69..ecf90b42 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 @@ -13,9 +13,6 @@ src/build_dir.rs: replace copy_tree -> Result with Ok(Default::default( src/build_dir.rs: replace copy_tree -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace ::name -> &str with "" src/cargo.rs: replace ::name -> &str with "xyzzy" -src/cargo.rs: replace ::top_source_files -> Result>> with Ok(vec![]) -src/cargo.rs: replace ::top_source_files -> Result>> with Ok(vec![Arc::new(Default::default())]) -src/cargo.rs: replace ::top_source_files -> Result>> with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec![]) src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec![String::new()]) src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec!["xyzzy".into()]) @@ -28,6 +25,9 @@ src/cargo.rs: replace ::compose_env -> Result::compose_env -> Result> with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace find_workspace -> Result with Ok(Default::default()) src/cargo.rs: replace find_workspace -> Result with Err(::anyhow::anyhow!("mutated!")) +src/cargo.rs: replace top_source_files -> Result>> with Ok(vec![]) +src/cargo.rs: replace top_source_files -> Result>> with Ok(vec![Arc::new(Default::default())]) +src/cargo.rs: replace top_source_files -> Result>> with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace cargo_bin -> String with String::new() src/cargo.rs: replace cargo_bin -> String with "xyzzy".into() src/cargo.rs: replace cargo_argv -> Vec with vec![] diff --git a/src/tool.rs b/src/tool.rs index 69a3ebeb..9cf022d2 100644 --- a/src/tool.rs +++ b/src/tool.rs @@ -8,39 +8,19 @@ use std::fmt::Debug; use std::marker::{Send, Sync}; -use std::sync::Arc; -use camino::Utf8Path; #[allow(unused_imports)] use tracing::{debug, debug_span, trace}; use crate::options::Options; use crate::outcome::Phase; use crate::source::Package; -use crate::SourceFile; use crate::{build_dir, Result}; pub trait Tool: Debug + Send + Sync { /// A short name for this tool, like "cargo". fn name(&self) -> &str; - /// Find the top-level files for each package within a tree. - /// - /// The path is the root returned by [find_root]. - /// - /// For Cargo, this is files like `src/bin/*.rs`, `src/lib.rs` identified by targets - /// in the manifest for each package. - /// - /// From each of these top files, we can discover more source by following `mod` - /// statements. - /// - /// If `packages` is non-empty, include only packages whose name is in this list. - fn top_source_files( - &self, - path: &Utf8Path, - packages: &[String], - ) -> Result>>; - /// Compose argv to run one phase in this tool. fn compose_argv( &self, diff --git a/src/visit.rs b/src/visit.rs index 8967302c..fec886f9 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -17,6 +17,7 @@ use syn::visit::Visit; use syn::{Attribute, Expr, ItemFn, ReturnType}; use tracing::{debug, debug_span, trace, trace_span, warn}; +use crate::cargo::top_source_files; use crate::fnvalue::return_type_replacements; use crate::pretty::ToPrettyString; use crate::source::SourceFile; @@ -36,8 +37,7 @@ pub struct Discovered { /// The list of source files includes even those with no mutants. /// /// `mutate_packages`: If non-empty, only generate mutants from these packages. -pub fn walk_tree( - tool: &T, +pub fn walk_tree( workspace_dir: &Utf8Path, mutate_packages: &[String], options: &Options, @@ -49,9 +49,8 @@ pub fn walk_tree( .map(|e| syn::parse_str(e).with_context(|| format!("Failed to parse error value {e:?}"))) .collect::>>()?; console.walk_tree_start(); - let mut file_queue: VecDeque> = tool - .top_source_files(workspace_dir, mutate_packages)? - .into(); + let mut file_queue: VecDeque> = + top_source_files(workspace_dir, mutate_packages)?.into(); let mut mutants = Vec::new(); let mut files: Vec> = Vec::new(); while let Some(source_file) = file_queue.pop_front() { @@ -398,8 +397,6 @@ fn attr_is_mutants_skip(attr: &Attribute) -> bool { mod test { use regex::Regex; - use crate::cargo::CargoTool; - use super::*; /// As a generic protection against regressions in discovery, the the mutants @@ -416,12 +413,12 @@ mod test { ..Default::default() }; let mut list_output = String::new(); - let source_tree_root = &Utf8Path::new(".") + let workspace_dir = &Utf8Path::new(".") .canonicalize_utf8() .expect("Canonicalize source path"); let console = Console::new(); - let discovered = walk_tree(&CargoTool::new(), source_tree_root, &[], &options, &console) - .expect("Discover mutants"); + let discovered = + walk_tree(workspace_dir, &[], &options, &console).expect("Discover mutants"); crate::list_mutants(&mut list_output, discovered, &options) .expect("Discover mutants in own source tree"); From 1859863b22120c84ada94db0979aa67e45a90ada Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 12:00:38 -0800 Subject: [PATCH 11/36] rm Tool::name --- src/cargo.rs | 4 ---- src/lab.rs | 3 +-- ...ts__visit__test__expected_mutants_for_own_source_tree.snap | 2 -- src/tool.rs | 3 --- 4 files changed, 1 insertion(+), 11 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index 6c63d9b0..3e5d930a 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -38,10 +38,6 @@ impl CargoTool { } impl Tool for CargoTool { - fn name(&self) -> &str { - "cargo" - } - fn compose_argv( &self, build_dir: &BuildDir, diff --git a/src/lab.rs b/src/lab.rs index 5d62c8c5..88329307 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -66,8 +66,7 @@ pub fn test_unmutated_then_all_mutants( }; if !baseline_outcome.success() { error!( - "{} {} failed in an unmutated tree, so no mutants were tested", - tool.name(), + "cargo {} failed in an unmutated tree, so no mutants were tested", baseline_outcome.last_phase(), ); // TODO: Maybe should be Err, but it would need to be an error that can map to the right 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 ecf90b42..3f54fc11 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 @@ -11,8 +11,6 @@ src/build_dir.rs: replace ::fmt -> fmt::Result with Ok( src/build_dir.rs: replace ::fmt -> fmt::Result with Err(::anyhow::anyhow!("mutated!")) src/build_dir.rs: replace copy_tree -> Result with Ok(Default::default()) src/build_dir.rs: replace copy_tree -> Result with Err(::anyhow::anyhow!("mutated!")) -src/cargo.rs: replace ::name -> &str with "" -src/cargo.rs: replace ::name -> &str with "xyzzy" src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec![]) src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec![String::new()]) src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec!["xyzzy".into()]) diff --git a/src/tool.rs b/src/tool.rs index 9cf022d2..2d83de24 100644 --- a/src/tool.rs +++ b/src/tool.rs @@ -18,9 +18,6 @@ use crate::source::Package; use crate::{build_dir, Result}; pub trait Tool: Debug + Send + Sync { - /// A short name for this tool, like "cargo". - fn name(&self) -> &str; - /// Compose argv to run one phase in this tool. fn compose_argv( &self, From 13a15f60e06a11c5399d9fda21050e40734a44da Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 12:16:58 -0800 Subject: [PATCH 12/36] rm Tool Not helping very much as an abstraction, and I don't really know where to put the layer while this only supports cargo. --- src/cargo.rs | 67 +++++++++---------- src/lab.rs | 36 +++------- src/main.rs | 13 +--- src/outcome.rs | 6 ++ ..._expected_mutants_for_own_source_tree.snap | 14 ++-- src/tool.rs | 31 --------- 6 files changed, 55 insertions(+), 112 deletions(-) delete mode 100644 src/tool.rs diff --git a/src/cargo.rs b/src/cargo.rs index 3e5d930a..2a943f36 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -4,6 +4,7 @@ use std::env; use std::sync::Arc; +use std::time::{Duration, Instant}; use anyhow::{anyhow, ensure, Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; @@ -13,44 +14,40 @@ use tracing::debug_span; #[allow(unused_imports)] use tracing::{debug, error, info, span, trace, warn, Level}; -use crate::process::get_command_output; +use crate::outcome::PhaseResult; +use crate::process::{get_command_output, Process}; use crate::source::Package; -use crate::tool::Tool; use crate::*; -#[derive(Debug)] -pub struct CargoTool { - // environment is currently constant across all invocations. - env: Vec<(String, String)>, -} - -impl CargoTool { - pub fn new() -> CargoTool { - let env = vec![ - ("CARGO_ENCODED_RUSTFLAGS".to_owned(), rustflags()), - // 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 - // updates and then let the test pass. - ("INSTA_UPDATE".to_owned(), "no".to_owned()), - ]; - CargoTool { env } - } -} - -impl Tool for CargoTool { - fn compose_argv( - &self, - build_dir: &BuildDir, - packages: Option<&[&Package]>, - phase: Phase, - options: &Options, - ) -> Result> { - Ok(cargo_argv(build_dir.path(), packages, phase, options)) - } - - fn compose_env(&self) -> Result> { - Ok(self.env.clone()) - } +/// Run cargo build, check, or test. +pub fn run_cargo( + build_dir: &BuildDir, + packages: Option<&[&Package]>, + phase: Phase, + timeout: Duration, + log_file: &mut LogFile, + options: &Options, + console: &Console, +) -> Result { + let _span = debug_span!("run", ?phase).entered(); + let start = Instant::now(); + let argv = cargo_argv(build_dir.path(), packages, phase, options); + let env = vec![ + ("CARGO_ENCODED_RUSTFLAGS".to_owned(), rustflags()), + // 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 + // updates and then let the test pass. + ("INSTA_UPDATE".to_owned(), "no".to_owned()), + ]; + let process_status = Process::run(&argv, &env, build_dir.path(), timeout, log_file, console)?; + check_interrupted()?; + debug!(?process_status, elapsed = ?start.elapsed()); + Ok(PhaseResult { + phase, + duration: start.elapsed(), + process_status, + argv, + }) } /// Return the path of the workspace directory enclosing a given directory. diff --git a/src/lab.rs b/src/lab.rs index 88329307..629df1f8 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -13,10 +13,10 @@ use tracing::warn; #[allow(unused)] use tracing::{debug, debug_span, error, info, trace}; +use crate::cargo::run_cargo; use crate::console::Console; -use crate::outcome::{LabOutcome, Phase, PhaseResult, ScenarioOutcome}; +use crate::outcome::{LabOutcome, Phase, ScenarioOutcome}; use crate::output::OutputDir; -use crate::process::Process; use crate::source::Package; use crate::visit::Discovered; use crate::*; @@ -25,8 +25,7 @@ use crate::*; /// /// Before testing the mutants, the lab checks that the source tree passes its tests with no /// mutations applied. -pub fn test_unmutated_then_all_mutants( - tool: &T, +pub fn test_unmutated_then_all_mutants( discovered: Discovered, workspace_dir: &Utf8Path, options: Options, @@ -54,7 +53,6 @@ pub fn test_unmutated_then_all_mutants( let baseline_outcome = { let _span = debug_span!("baseline").entered(); test_scenario( - tool, &mut build_dirs[0], &output_mutex, &options, @@ -126,7 +124,6 @@ pub fn test_unmutated_then_all_mutants( 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, &mut build_dir, &output_mutex, &options, @@ -174,8 +171,7 @@ pub fn test_unmutated_then_all_mutants( )] // Yes, it's a lot of arguments, but it does use them all and I don't think creating objects // just to group them would help... -fn test_scenario( - tool: &T, +fn test_scenario( build_dir: &mut BuildDir, output_mutex: &Mutex, options: &Options, @@ -202,34 +198,24 @@ fn test_scenario( &[Phase::Build, Phase::Test] }; for &phase in phases { - let _span = debug_span!("run", ?phase).entered(); - let start = Instant::now(); console.scenario_phase_started(scenario, phase); let timeout = match phase { Phase::Test => test_timeout, _ => Duration::MAX, }; - let argv = tool.compose_argv(build_dir, Some(test_packages), phase, options)?; - let env = tool.compose_env()?; - let process_status = Process::run( - &argv, - &env, - build_dir.path(), + let phase_result = run_cargo( + build_dir, + Some(test_packages), + phase, timeout, &mut log_file, + options, console, )?; - check_interrupted()?; - debug!(?process_status, elapsed = ?start.elapsed()); - let phase_result = PhaseResult { - phase, - duration: start.elapsed(), - process_status, - argv, - }; + let success = phase_result.is_success(); outcome.add_phase_result(phase_result); console.scenario_phase_finished(scenario, phase); - if (phase == Phase::Check && options.check_only) || !process_status.success() { + if (phase == Phase::Check && options.check_only) || !success { break; } } diff --git a/src/main.rs b/src/main.rs index f9950547..e6b59935 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,7 +23,6 @@ mod process; mod scenario; mod source; mod textedit; -mod tool; mod visit; use std::env; @@ -41,7 +40,6 @@ use tracing::debug; // Imports of public names from this crate. use crate::build_dir::BuildDir; -use crate::cargo::CargoTool; use crate::console::Console; use crate::interrupt::check_interrupted; use crate::list::{list_files, list_mutants, FmtToIoWrite}; @@ -53,7 +51,6 @@ use crate::outcome::{Phase, ScenarioOutcome}; use crate::path::Utf8PathSlashes; use crate::scenario::Scenario; use crate::source::SourceFile; -use crate::tool::Tool; use crate::visit::walk_tree; const VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -229,7 +226,6 @@ fn main() -> Result<()> { } else { Utf8Path::new(".") }; - let tool = CargoTool::new(); let workspace_dir = cargo::find_workspace(source_path)?; let config = if args.no_config { config::Config::default() @@ -247,13 +243,8 @@ fn main() -> Result<()> { console.clear(); list_mutants(FmtToIoWrite::new(io::stdout()), discovered, &options)?; } else { - let lab_outcome = lab::test_unmutated_then_all_mutants( - &tool, - discovered, - &workspace_dir, - options, - &console, - )?; + let lab_outcome = + lab::test_unmutated_then_all_mutants(discovered, &workspace_dir, options, &console)?; exit(lab_outcome.exit_code()); } Ok(()) diff --git a/src/outcome.rs b/src/outcome.rs index daeb5751..3505a69b 100644 --- a/src/outcome.rs +++ b/src/outcome.rs @@ -273,6 +273,12 @@ pub struct PhaseResult { pub argv: Vec, } +impl PhaseResult { + pub fn is_success(&self) -> bool { + self.process_status.success() + } +} + impl Serialize for PhaseResult { fn serialize(&self, serializer: S) -> Result where 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 3f54fc11..26acd737 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 @@ -11,16 +11,8 @@ src/build_dir.rs: replace ::fmt -> fmt::Result with Ok( src/build_dir.rs: replace ::fmt -> fmt::Result with Err(::anyhow::anyhow!("mutated!")) src/build_dir.rs: replace copy_tree -> Result with Ok(Default::default()) src/build_dir.rs: replace copy_tree -> Result with Err(::anyhow::anyhow!("mutated!")) -src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec![]) -src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec![String::new()]) -src/cargo.rs: replace ::compose_argv -> Result> with Ok(vec!["xyzzy".into()]) -src/cargo.rs: replace ::compose_argv -> Result> with Err(::anyhow::anyhow!("mutated!")) -src/cargo.rs: replace ::compose_env -> Result> with Ok(vec![]) -src/cargo.rs: replace ::compose_env -> Result> with Ok(vec![(String::new(), String::new())]) -src/cargo.rs: replace ::compose_env -> Result> with Ok(vec![(String::new(), "xyzzy".into())]) -src/cargo.rs: replace ::compose_env -> Result> with Ok(vec![("xyzzy".into(), String::new())]) -src/cargo.rs: replace ::compose_env -> Result> with Ok(vec![("xyzzy".into(), "xyzzy".into())]) -src/cargo.rs: replace ::compose_env -> Result> with Err(::anyhow::anyhow!("mutated!")) +src/cargo.rs: replace run_cargo -> Result with Ok(Default::default()) +src/cargo.rs: replace run_cargo -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace find_workspace -> Result with Ok(Default::default()) src/cargo.rs: replace find_workspace -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace top_source_files -> Result>> with Ok(vec![]) @@ -257,6 +249,8 @@ src/outcome.rs: replace ScenarioOutcome::mutant_caught -> bool with false src/outcome.rs: replace ScenarioOutcome::mutant_missed -> bool with true src/outcome.rs: replace ScenarioOutcome::mutant_missed -> bool with false src/outcome.rs: replace ScenarioOutcome::summary -> SummaryOutcome with Default::default() +src/outcome.rs: replace PhaseResult::is_success -> bool with true +src/outcome.rs: replace PhaseResult::is_success -> bool with false src/outcome.rs: replace ::serialize -> Result with Ok(Default::default()) src/outcome.rs: replace ::serialize -> Result with Err(::anyhow::anyhow!("mutated!")) src/output.rs: replace LockFile::acquire_lock -> Result with Ok(Default::default()) diff --git a/src/tool.rs b/src/tool.rs deleted file mode 100644 index 2d83de24..00000000 --- a/src/tool.rs +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright 2023 Martin Pool. - -//! A generic interface for running a tool such as Cargo that can determine the tree -//! shape, build it, and run tests. -//! -//! At present only Cargo is supported, but this interface aims to leave a place to -//! support for example Bazel in future. - -use std::fmt::Debug; -use std::marker::{Send, Sync}; - -#[allow(unused_imports)] -use tracing::{debug, debug_span, trace}; - -use crate::options::Options; -use crate::outcome::Phase; -use crate::source::Package; -use crate::{build_dir, Result}; - -pub trait Tool: Debug + Send + Sync { - /// Compose argv to run one phase in this tool. - fn compose_argv( - &self, - build_dir: &build_dir::BuildDir, - packages: Option<&[&Package]>, - phase: Phase, - options: &Options, - ) -> Result>; - - fn compose_env(&self) -> Result>; -} From ee1c1084827eb1ff199e38904e0df13fa0f47c41 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 12:25:02 -0800 Subject: [PATCH 13/36] Clean up test_mutants --- src/lab.rs | 14 +++----------- src/main.rs | 4 ++-- ...test__expected_mutants_for_own_source_tree.snap | 4 ++-- 3 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 629df1f8..82ac1fda 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -18,15 +18,14 @@ use crate::console::Console; use crate::outcome::{LabOutcome, Phase, ScenarioOutcome}; use crate::output::OutputDir; use crate::source::Package; -use crate::visit::Discovered; use crate::*; /// Run all possible mutation experiments. /// /// Before testing the mutants, the lab checks that the source tree passes its tests with no /// mutations applied. -pub fn test_unmutated_then_all_mutants( - discovered: Discovered, +pub fn test_mutants( + mut mutants: Vec, workspace_dir: &Utf8Path, options: Options, console: &Console, @@ -38,7 +37,6 @@ pub fn test_unmutated_then_all_mutants( .map_or(workspace_dir, |p| p.as_path()); let output_dir = OutputDir::new(output_in_dir)?; console.set_debug_log(output_dir.open_debug_log()?); - let mut mutants = discovered.mutants; if options.shuffle { fastrand::shuffle(&mut mutants); @@ -164,13 +162,7 @@ pub fn test_unmutated_then_all_mutants( /// /// The [BuildDir] is passed as mutable because it's for the exclusive use of this function for the /// duration of the test. -#[allow( - unknown_lints, - clippy::needless_pass_by_ref_mut, - clippy::too_many_arguments -)] -// Yes, it's a lot of arguments, but it does use them all and I don't think creating objects -// just to group them would help... +#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] fn test_scenario( build_dir: &mut BuildDir, output_mutex: &Mutex, diff --git a/src/main.rs b/src/main.rs index e6b59935..84c57760 100644 --- a/src/main.rs +++ b/src/main.rs @@ -42,6 +42,7 @@ use tracing::debug; use crate::build_dir::BuildDir; use crate::console::Console; use crate::interrupt::check_interrupted; +use crate::lab::test_mutants; use crate::list::{list_files, list_mutants, FmtToIoWrite}; use crate::log_file::{last_line, LogFile}; use crate::manifest::fix_manifest; @@ -243,8 +244,7 @@ fn main() -> Result<()> { console.clear(); list_mutants(FmtToIoWrite::new(io::stdout()), discovered, &options)?; } else { - let lab_outcome = - lab::test_unmutated_then_all_mutants(discovered, &workspace_dir, options, &console)?; + let lab_outcome = test_mutants(discovered.mutants, &workspace_dir, options, &console)?; exit(lab_outcome.exit_code()); } Ok(()) 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 26acd737..e3d2e0b1 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 @@ -143,8 +143,8 @@ src/fnvalue.rs: replace path_is_nonzero_unsigned -> bool with false src/fnvalue.rs: replace match_first_type_arg -> Option<&'p Type> with None src/fnvalue.rs: replace match_first_type_arg -> Option<&'p Type> with Some(&Default::default()) src/interrupt.rs: replace install_handler with () -src/lab.rs: replace test_unmutated_then_all_mutants -> Result with Ok(Default::default()) -src/lab.rs: replace test_unmutated_then_all_mutants -> Result with Err(::anyhow::anyhow!("mutated!")) +src/lab.rs: replace test_mutants -> Result with Ok(Default::default()) +src/lab.rs: replace test_mutants -> Result with Err(::anyhow::anyhow!("mutated!")) src/lab.rs: replace test_scenario -> Result with Ok(Default::default()) src/lab.rs: replace test_scenario -> Result with Err(::anyhow::anyhow!("mutated!")) src/list.rs: replace >::write_str -> Result<(), fmt::Error> with Ok(()) From c8e6d2bca7e327480ac1e726badcb739d374a8e1 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 12:29:15 -0800 Subject: [PATCH 14/36] Pass BuildDir as &mut --- src/lab.rs | 1 - src/mutate.rs | 9 ++++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 82ac1fda..78597426 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -162,7 +162,6 @@ pub fn test_mutants( /// /// The [BuildDir] is passed as mutable because it's for the exclusive use of this function for the /// duration of the test. -#[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] fn test_scenario( build_dir: &mut BuildDir, output_mutex: &Mutex, diff --git a/src/mutate.rs b/src/mutate.rs index d804c52c..2a59f42d 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -121,15 +121,18 @@ impl Mutant { .to_string() } - pub fn apply(&self, build_dir: &BuildDir) -> Result<()> { + /// Apply this mutant to the relevant file within a BuildDir. + pub fn apply(&self, build_dir: &mut BuildDir) -> Result<()> { self.write_in_dir(build_dir, &self.mutated_code()) } - pub fn unapply(&self, build_dir: &BuildDir) -> Result<()> { + pub fn unapply(&self, build_dir: &mut BuildDir) -> Result<()> { self.write_in_dir(build_dir, self.original_code()) } - fn write_in_dir(&self, build_dir: &BuildDir, code: &str) -> Result<()> { + #[allow(unknown_lints, clippy::needless_pass_by_ref_mut)] + // The Rust object is not mutated, but the BuildDir on disk should be exclusively owned for this to be safe. + fn write_in_dir(&self, build_dir: &mut BuildDir, code: &str) -> Result<()> { let path = build_dir.path().join(&self.source_file.tree_relative_path); // for safety, don't follow symlinks ensure!(path.is_file(), "{path:?} is not a file"); From 2b269f2b4c255f744616d4b8a8f550e5cdbc9bc2 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 12:35:34 -0800 Subject: [PATCH 15/36] Pass Options second last by convention --- src/lab.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 78597426..241acdc2 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -45,6 +45,7 @@ pub fn test_mutants( console.discovered_mutants(&mutants); ensure!(!mutants.is_empty(), "No mutants found"); let all_packages = mutants.iter().map(|m| m.package()).unique().collect_vec(); + debug!(?all_packages); let output_mutex = Mutex::new(output_dir); let mut build_dirs = vec![BuildDir::new(workspace_dir, &options, console)?]; @@ -53,10 +54,10 @@ pub fn test_mutants( test_scenario( &mut build_dirs[0], &output_mutex, - &options, &Scenario::Baseline, &all_packages, options.test_timeout.unwrap_or(Duration::MAX), + &options, console, )? }; @@ -124,10 +125,10 @@ pub fn test_mutants( let _outcome = test_scenario( &mut build_dir, &output_mutex, - &options, &Scenario::Mutant(mutant), &[&package], mutated_test_timeout, + &options, console, ) .expect("scenario test"); @@ -165,10 +166,10 @@ pub fn test_mutants( fn test_scenario( build_dir: &mut BuildDir, output_mutex: &Mutex, - options: &Options, scenario: &Scenario, test_packages: &[&Package], test_timeout: Duration, + options: &Options, console: &Console, ) -> Result { let mut log_file = output_mutex From 77e2a7ef105f9d74edeeae995c59d905ca69cd97 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 15:32:00 -0800 Subject: [PATCH 16/36] Warn if --package options don't match --- src/cargo.rs | 5 +++++ tests/cli/main.rs | 1 + tests/cli/workspace.rs | 23 +++++++++++++++++++++++ 3 files changed, 29 insertions(+) create mode 100644 tests/cli/workspace.rs diff --git a/src/cargo.rs b/src/cargo.rs index 2a943f36..02bf6a8d 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -139,6 +139,11 @@ pub fn top_source_files( )?)); } } + for p in include_packages { + if !r.iter().any(|sf| sf.package.name == *p) { + warn!("package {p} not found in source tree"); + } + } Ok(r) } diff --git a/tests/cli/main.rs b/tests/cli/main.rs index 7f71dbfe..f9927f7b 100644 --- a/tests/cli/main.rs +++ b/tests/cli/main.rs @@ -27,6 +27,7 @@ mod jobs; mod trace; #[cfg(windows)] mod windows; +mod workspace; /// A timeout for a `cargo mutants` invocation from the test suite. Needs to be /// long enough that even commands that do a lot of work can pass even on slow diff --git a/tests/cli/workspace.rs b/tests/cli/workspace.rs new file mode 100644 index 00000000..9a8f0068 --- /dev/null +++ b/tests/cli/workspace.rs @@ -0,0 +1,23 @@ +// Copyright 2023 Martin Pool + +//! Tests for cargo workspaces with multiple packages. + +use super::run; + +#[test] +fn list_warns_about_unmatched_packages() { + run() + .args([ + "mutants", + "--list", + "-d", + "testdata/tree/workspace", + "-p", + "notapackage", + ]) + .assert() + .stdout(predicates::str::contains( + "package notapackage not found in source tree", + )) + .code(0); +} From 9b146014e29b5414bde1f6143e360ffbf531552a Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 15:38:17 -0800 Subject: [PATCH 17/36] Split out workspace tests --- tests/cli/main.rs | 153 +-------------------------------------- tests/cli/workspace.rs | 157 ++++++++++++++++++++++++++++++++++++++++- 2 files changed, 157 insertions(+), 153 deletions(-) diff --git a/tests/cli/main.rs b/tests/cli/main.rs index f9927f7b..eaeb59af 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, read_to_string}; +use std::fs::{self, read_dir}; use std::io::Read; use std::path::{Path, PathBuf}; use std::thread::sleep; @@ -463,157 +463,6 @@ fn list_files_json_well_tested() { .current_dir("testdata/tree/well_tested") .assert_insta("list_files_json_well_tested"); } - -#[test] -fn list_files_json_workspace() { - // Demonstrates that we get package names in the json listing. - run() - .args(["mutants", "--list-files", "--json"]) - .current_dir("testdata/tree/workspace") - .assert_insta("list_files_json_workspace"); -} - -#[test] -fn list_files_as_json_in_workspace_subdir() { - run() - .args(["mutants", "--list-files", "--json"]) - .current_dir("testdata/tree/workspace/main2") - .assert() - .stdout(indoc! {r#" - [ - { - "package": "cargo_mutants_testdata_workspace_utils", - "path": "utils/src/lib.rs" - }, - { - "package": "main", - "path": "main/src/main.rs" - }, - { - "package": "main2", - "path": "main2/src/main.rs" - } - ] - "#}); -} - -#[test] -fn workspace_tree_is_well_tested() { - let tmp_src_dir = copy_of_testdata("workspace"); - run() - .args(["mutants", "-d"]) - .arg(tmp_src_dir.path()) - .assert() - .success(); - // The outcomes.json has some summary data - let json_str = - fs::read_to_string(tmp_src_dir.path().join("mutants.out/outcomes.json")).unwrap(); - println!("outcomes.json:\n{json_str}"); - let json: serde_json::Value = json_str.parse().unwrap(); - assert_eq!(json["total_mutants"].as_u64().unwrap(), 8); - assert_eq!(json["caught"].as_u64().unwrap(), 8); - assert_eq!(json["missed"].as_u64().unwrap(), 0); - assert_eq!(json["timeout"].as_u64().unwrap(), 0); - let outcomes = json["outcomes"].as_array().unwrap(); - - { - let baseline = outcomes[0].as_object().unwrap(); - assert_eq!(baseline["scenario"].as_str().unwrap(), "Baseline"); - assert_eq!(baseline["summary"], "Success"); - let baseline_phases = baseline["phase_results"].as_array().unwrap(); - assert_eq!(baseline_phases.len(), 2); - assert_eq!(baseline_phases[0]["process_status"], "Success"); - assert_eq!( - baseline_phases[0]["argv"].as_array().unwrap().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() - .iter() - .map(|v| v.as_str().unwrap()) - .skip(1) - .collect_vec() - .join(" "), - "test --package cargo_mutants_testdata_workspace_utils --package main --package main2" - ); - } - - assert_eq!(outcomes.len(), 9); - for outcome in &outcomes[1..] { - let mutant = &outcome["scenario"]["Mutant"]; - let package_name = mutant["package"].as_str().unwrap(); - assert!(!package_name.is_empty()); - assert_eq!(outcome["summary"], "CaughtMutant"); - let mutant_phases = outcome["phase_results"].as_array().unwrap(); - assert_eq!(mutant_phases.len(), 2); - assert_eq!(mutant_phases[0]["process_status"], "Success"); - assert_eq!( - mutant_phases[0]["argv"].as_array().unwrap()[1..=3], - ["build", "--tests", "--manifest-path"] - ); - assert_eq!(mutant_phases[1]["process_status"], "Failure"); - assert_eq!( - mutant_phases[1]["argv"].as_array().unwrap()[1..=2], - ["test", "--manifest-path"], - ); - } - { - let baseline = json["outcomes"][0].as_object().unwrap(); - assert_eq!(baseline["scenario"].as_str().unwrap(), "Baseline"); - assert_eq!(baseline["summary"], "Success"); - let baseline_phases = baseline["phase_results"].as_array().unwrap(); - assert_eq!(baseline_phases.len(), 2); - assert_eq!(baseline_phases[0]["process_status"], "Success"); - assert_eq!( - baseline_phases[0]["argv"].as_array().unwrap()[1..].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..] - .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 -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, diff --git a/tests/cli/workspace.rs b/tests/cli/workspace.rs index 9a8f0068..1ee0f472 100644 --- a/tests/cli/workspace.rs +++ b/tests/cli/workspace.rs @@ -2,7 +2,12 @@ //! Tests for cargo workspaces with multiple packages. -use super::run; +use std::fs::{self, read_to_string}; + +use indoc::indoc; +use itertools::Itertools; + +use super::{copy_of_testdata, run, CommandInstaExt}; #[test] fn list_warns_about_unmatched_packages() { @@ -21,3 +26,153 @@ fn list_warns_about_unmatched_packages() { )) .code(0); } + +#[test] +fn list_files_json_workspace() { + // Demonstrates that we get package names in the json listing. + run() + .args(["mutants", "--list-files", "--json"]) + .current_dir("testdata/tree/workspace") + .assert_insta("list_files_json_workspace"); +} + +#[test] +fn list_files_as_json_in_workspace_subdir() { + run() + .args(["mutants", "--list-files", "--json"]) + .current_dir("testdata/tree/workspace/main2") + .assert() + .stdout(indoc! {r#" + [ + { + "package": "cargo_mutants_testdata_workspace_utils", + "path": "utils/src/lib.rs" + }, + { + "package": "main", + "path": "main/src/main.rs" + }, + { + "package": "main2", + "path": "main2/src/main.rs" + } + ] + "#}); +} + +#[test] +fn workspace_tree_is_well_tested() { + let tmp_src_dir = copy_of_testdata("workspace"); + run() + .args(["mutants", "-d"]) + .arg(tmp_src_dir.path()) + .assert() + .success(); + // The outcomes.json has some summary data + let json_str = + fs::read_to_string(tmp_src_dir.path().join("mutants.out/outcomes.json")).unwrap(); + println!("outcomes.json:\n{json_str}"); + let json: serde_json::Value = json_str.parse().unwrap(); + assert_eq!(json["total_mutants"].as_u64().unwrap(), 8); + assert_eq!(json["caught"].as_u64().unwrap(), 8); + assert_eq!(json["missed"].as_u64().unwrap(), 0); + assert_eq!(json["timeout"].as_u64().unwrap(), 0); + let outcomes = json["outcomes"].as_array().unwrap(); + + { + let baseline = outcomes[0].as_object().unwrap(); + assert_eq!(baseline["scenario"].as_str().unwrap(), "Baseline"); + assert_eq!(baseline["summary"], "Success"); + let baseline_phases = baseline["phase_results"].as_array().unwrap(); + assert_eq!(baseline_phases.len(), 2); + assert_eq!(baseline_phases[0]["process_status"], "Success"); + assert_eq!( + baseline_phases[0]["argv"].as_array().unwrap().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() + .iter() + .map(|v| v.as_str().unwrap()) + .skip(1) + .collect_vec() + .join(" "), + "test --package cargo_mutants_testdata_workspace_utils --package main --package main2" + ); + } + + assert_eq!(outcomes.len(), 9); + for outcome in &outcomes[1..] { + let mutant = &outcome["scenario"]["Mutant"]; + let package_name = mutant["package"].as_str().unwrap(); + assert!(!package_name.is_empty()); + assert_eq!(outcome["summary"], "CaughtMutant"); + let mutant_phases = outcome["phase_results"].as_array().unwrap(); + assert_eq!(mutant_phases.len(), 2); + assert_eq!(mutant_phases[0]["process_status"], "Success"); + assert_eq!( + mutant_phases[0]["argv"].as_array().unwrap()[1..=3], + ["build", "--tests", "--manifest-path"] + ); + assert_eq!(mutant_phases[1]["process_status"], "Failure"); + assert_eq!( + mutant_phases[1]["argv"].as_array().unwrap()[1..=2], + ["test", "--manifest-path"], + ); + } + { + let baseline = json["outcomes"][0].as_object().unwrap(); + assert_eq!(baseline["scenario"].as_str().unwrap(), "Baseline"); + assert_eq!(baseline["summary"], "Success"); + let baseline_phases = baseline["phase_results"].as_array().unwrap(); + assert_eq!(baseline_phases.len(), 2); + assert_eq!(baseline_phases[0]["process_status"], "Success"); + assert_eq!( + baseline_phases[0]["argv"].as_array().unwrap()[1..].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..] + .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 +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(), + "" + ); +} From 033a8ef7d6a68ba21419b5e7e5df16694cea053e Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 15:42:47 -0800 Subject: [PATCH 18/36] Check that baseline only tests relevant packages --- tests/cli/workspace.rs | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/tests/cli/workspace.rs b/tests/cli/workspace.rs index 1ee0f472..09377798 100644 --- a/tests/cli/workspace.rs +++ b/tests/cli/workspace.rs @@ -149,7 +149,7 @@ fn workspace_tree_is_well_tested() { /// Baseline tests in a workspace only test the packages that will later /// be mutated. /// See -fn in_workspace_only_relevant_packages_included_in_baseline_tests() { +fn in_workspace_only_relevant_packages_included_in_baseline_tests_by_file_filter() { let tmp = copy_of_testdata("package_fails"); run() .args(["mutants", "-f", "passing/src/lib.rs", "--no-shuffle", "-d"]) @@ -176,3 +176,40 @@ fn in_workspace_only_relevant_packages_included_in_baseline_tests() { "" ); } + +/// Even the baseline test only tests the explicitly selected packages, +/// so it doesn't fail if some packages don't build. +#[test] +fn baseline_test_respects_package_options() { + let tmp = copy_of_testdata("package_fails"); + run() + .args([ + "mutants", + "--package", + "cargo-mutants-testdata-package-fails-passing", + "--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(), + "" + ); +} From fd9165c84d76a9318ff2245e2e185a2f1a7bdf05 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Sun, 5 Nov 2023 15:46:20 -0800 Subject: [PATCH 19/36] Simplify --- src/main.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main.rs b/src/main.rs index 84c57760..c5453c1d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -222,11 +222,7 @@ fn main() -> Result<()> { console.setup_global_trace(args.level)?; interrupt::install_handler(); - let source_path: &Utf8Path = if let Some(p) = &args.dir { - p - } else { - Utf8Path::new(".") - }; + let source_path: &Utf8Path = args.dir.as_deref().unwrap_or(Utf8Path::new(".")); let workspace_dir = cargo::find_workspace(source_path)?; let config = if args.no_config { config::Config::default() From 146b5dd5a9800deb939e6122acf5db3544cc4573 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 7 Nov 2023 13:46:58 -0800 Subject: [PATCH 20/36] Add Workspace and PackageFilter Moving towards being able to auto-select packages from workspace subdir --- src/build_dir.rs | 4 +- src/cargo.rs | 222 +---------- src/lab.rs | 2 +- src/main.rs | 17 +- src/mutate.rs | 35 +- src/output.rs | 11 +- src/package.rs | 23 ++ ..._expected_mutants_for_own_source_tree.snap | 40 +- src/source.rs | 12 +- src/visit.rs | 22 +- src/workspace.rs | 357 ++++++++++++++++++ 11 files changed, 468 insertions(+), 277 deletions(-) create mode 100644 src/package.rs create mode 100644 src/workspace.rs diff --git a/src/build_dir.rs b/src/build_dir.rs index a2338966..db7d2e4c 100644 --- a/src/build_dir.rs +++ b/src/build_dir.rs @@ -152,8 +152,8 @@ mod test { #[test] fn build_dir_debug_form() { let options = Options::default(); - let root = cargo::find_workspace("testdata/tree/factorial".into()).unwrap(); - let build_dir = BuildDir::new(&root, &options, &Console::new()).unwrap(); + let workspace = Workspace::open(Utf8Path::new("testdata/tree/factorial")).unwrap(); + let build_dir = BuildDir::new(&workspace.dir, &options, &Console::new()).unwrap(); let debug_form = format!("{build_dir:?}"); assert!( Regex::new(r#"^BuildDir \{ path: "[^"]*[/\\]cargo-mutants-factorial[^"]*" \}$"#) diff --git a/src/cargo.rs b/src/cargo.rs index 02bf6a8d..cb959a7a 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -8,6 +8,7 @@ use std::time::{Duration, Instant}; use anyhow::{anyhow, ensure, Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; +use cargo_metadata::Metadata; use itertools::Itertools; use serde_json::Value; use tracing::debug_span; @@ -15,8 +16,8 @@ use tracing::debug_span; use tracing::{debug, error, info, span, trace, warn, Level}; use crate::outcome::PhaseResult; +use crate::package::Package; use crate::process::{get_command_output, Process}; -use crate::source::Package; use crate::*; /// Run cargo build, check, or test. @@ -50,105 +51,20 @@ pub fn run_cargo( }) } -/// Return the path of the workspace directory enclosing a given directory. -pub fn find_workspace(path: &Utf8Path) -> Result { - ensure!(path.is_dir(), "{path:?} is not a directory"); - let cargo_bin = cargo_bin(); // needed for lifetime - let argv: Vec<&str> = vec![&cargo_bin, "locate-project", "--workspace"]; - let stdout = get_command_output(&argv, path) - .with_context(|| format!("run cargo locate-project in {path:?}"))?; - let val: Value = serde_json::from_str(&stdout).context("parse cargo locate-project output")?; - let cargo_toml_path: Utf8PathBuf = val["root"] - .as_str() - .with_context(|| format!("cargo locate-project output has no root: {stdout:?}"))? - .to_owned() - .into(); - debug!(?cargo_toml_path, "Found workspace root manifest"); - ensure!( - cargo_toml_path.is_file(), - "cargo locate-project root {cargo_toml_path:?} is not a file" - ); - let root = cargo_toml_path - .parent() - .ok_or_else(|| anyhow!("cargo locate-project root {cargo_toml_path:?} has no parent"))? - .to_owned(); - ensure!( - root.is_dir(), - "apparent project root directory {root:?} is not a directory" - ); - Ok(root) -} - -/// Find the root files for each relevant package in the source tree. -/// -/// A source tree might include multiple packages (e.g. in a Cargo workspace), -/// and each package might have multiple targets (e.g. a bin and lib). Test targets -/// are excluded here: we run them, but we don't mutate them. -/// -/// Each target has one root file, typically but not necessarily called `src/lib.rs` -/// or `src/main.rs`. This function returns a list of all those files. -/// -/// After this, there is one more level of discovery, by walking those root files -/// to find `mod` statements, and then recursively walking those files to find -/// all source files. -/// -/// Packages are only included if their name is in `include_packages`. -pub fn top_source_files( - workspace_dir: &Utf8Path, - include_packages: &[String], -) -> Result>> { +pub fn run_cargo_metadata(workspace_dir: &Utf8Path) -> Result { let cargo_toml_path = workspace_dir.join("Cargo.toml"); - debug!(?cargo_toml_path, ?workspace_dir, "Find root files"); + debug!(?cargo_toml_path, ?workspace_dir, "run cargo metadata"); check_interrupted()?; let metadata = cargo_metadata::MetadataCommand::new() .manifest_path(&cargo_toml_path) .exec() .context("run cargo metadata")?; - - let mut r = Vec::new(); - // cargo-metadata output is not obviously ordered so make it deterministic. - for package_metadata in metadata - .workspace_packages() - .iter() - .filter(|p| include_packages.is_empty() || include_packages.contains(&p.name)) - .sorted_by_key(|p| &p.name) - { - check_interrupted()?; - let _span = debug_span!("package", name = %package_metadata.name).entered(); - let manifest_path = &package_metadata.manifest_path; - debug!(%manifest_path, "walk package"); - let relative_manifest_path = manifest_path - .strip_prefix(workspace_dir) - .map_err(|_| { - anyhow!( - "manifest path {manifest_path:?} for package {name:?} is not within the detected source root path {workspace_dir:?}", - name = package_metadata.name - ) - })? - .to_owned(); - let package = Arc::new(Package { - name: package_metadata.name.clone(), - relative_manifest_path, - }); - for source_path in direct_package_sources(workspace_dir, package_metadata)? { - check_interrupted()?; - r.push(Arc::new(SourceFile::new( - workspace_dir, - source_path, - &package, - )?)); - } - } - for p in include_packages { - if !r.iter().any(|sf| sf.package.name == *p) { - warn!("package {p} not found in source tree"); - } - } - Ok(r) + check_interrupted()?; + Ok(metadata) } /// Return the name of the cargo binary. -fn cargo_bin() -> String { +pub fn cargo_bin() -> String { // When run as a Cargo subcommand, which is the usual/intended case, // $CARGO tells us the right way to call back into it, so that we get // the matching toolchain etc. @@ -222,46 +138,6 @@ fn rustflags() -> String { rustflags.join("\x1f") } -/// 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. -fn direct_package_sources( - workspace_root: &Utf8Path, - package_metadata: &cargo_metadata::Package, -) -> Result> { - let mut found = Vec::new(); - let pkg_dir = package_metadata.manifest_path.parent().unwrap(); - for target in &package_metadata.targets { - if should_mutate_target(target) { - if let Ok(relpath) = target - .src_path - .strip_prefix(workspace_root) - .map(ToOwned::to_owned) - { - debug!( - "found mutation target {} of kind {:?}", - relpath, target.kind - ); - found.push(relpath); - } else { - warn!("{:?} is not in {:?}", target.src_path, pkg_dir); - } - } else { - debug!( - "skipping target {:?} of kinds {:?}", - target.name, target.kind - ); - } - } - found.sort(); - found.dedup(); - Ok(found) -} - -fn should_mutate_target(target: &cargo_metadata::Target) -> bool { - target.kind.iter().any(|k| k.ends_with("lib") || k == "bin") -} - #[cfg(test)] mod test { use std::ffi::OsStr; @@ -364,88 +240,4 @@ mod test { ] ); } - - #[test] - fn error_opening_outside_of_crate() { - cargo::find_workspace(Utf8Path::new("/")).unwrap_err(); - } - - #[test] - fn open_subdirectory_of_crate_opens_the_crate() { - let root = cargo::find_workspace(Utf8Path::new("testdata/tree/factorial/src")) - .expect("open source tree from subdirectory"); - assert!(root.is_dir()); - assert!(root.join("Cargo.toml").is_file()); - assert!(root.join("src/bin/factorial.rs").is_file()); - assert_eq!(root.file_name().unwrap(), OsStr::new("factorial")); - } - - #[test] - fn find_root_from_subdirectory_of_workspace_finds_the_workspace_root() { - let root = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) - .expect("Find root from within workspace/main"); - assert_eq!(root.file_name(), Some("workspace"), "Wrong root: {root:?}"); - } - - #[test] - fn find_top_source_files_from_subdirectory_of_workspace() { - let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) - .expect("Find workspace root"); - let top_source_files = top_source_files(&root_dir, &[]).expect("Find root files"); - println!("{top_source_files:#?}"); - let paths = top_source_files - .iter() - .map(|sf| sf.tree_relative_path.to_slash_path()) - .collect_vec(); - // The order here might look strange, but they're actually deterministically - // sorted by the package name, not the path name. - assert_eq!( - paths, - ["utils/src/lib.rs", "main/src/main.rs", "main2/src/main.rs"] - ); - } - - #[test] - fn filter_by_single_package() { - let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) - .expect("Find workspace root"); - assert_eq!( - root_dir.file_name(), - Some("workspace"), - "found the workspace root" - ); - let top_source_files = - top_source_files(&root_dir, &["main".to_owned()]).expect("Find root files"); - println!("{top_source_files:#?}"); - assert_eq!(top_source_files.len(), 1); - assert_eq!( - top_source_files - .iter() - .map(|sf| sf.tree_relative_path.clone()) - .collect_vec(), - ["main/src/main.rs"] - ); - } - - #[test] - fn filter_by_multiple_packages() { - let root_dir = cargo::find_workspace(Utf8Path::new("testdata/tree/workspace/main")) - .expect("Find workspace root"); - assert_eq!( - root_dir.file_name(), - Some("workspace"), - "found the workspace root" - ); - let top_source_files = - top_source_files(&root_dir, &["main".to_owned(), "main2".to_owned()]) - .expect("Find root files"); - println!("{top_source_files:#?}"); - assert_eq!( - top_source_files - .iter() - .map(|sf| sf.tree_relative_path.clone()) - .collect_vec(), - ["main/src/main.rs", "main2/src/main.rs"] - ); - } } diff --git a/src/lab.rs b/src/lab.rs index 241acdc2..7d2c5f82 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -17,7 +17,7 @@ use crate::cargo::run_cargo; use crate::console::Console; use crate::outcome::{LabOutcome, Phase, ScenarioOutcome}; use crate::output::OutputDir; -use crate::source::Package; +use crate::package::Package; use crate::*; /// Run all possible mutation experiments. diff --git a/src/main.rs b/src/main.rs index c5453c1d..ad69885e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -17,6 +17,7 @@ mod mutate; mod options; mod outcome; mod output; +mod package; mod path; mod pretty; mod process; @@ -24,6 +25,7 @@ mod scenario; mod source; mod textedit; mod visit; +pub mod workspace; use std::env; use std::io; @@ -53,6 +55,8 @@ use crate::path::Utf8PathSlashes; use crate::scenario::Scenario; use crate::source::SourceFile; use crate::visit::walk_tree; +use crate::workspace::PackageFilter; +use crate::workspace::Workspace; const VERSION: &str = env!("CARGO_PKG_VERSION"); const NAME: &str = env!("CARGO_PKG_NAME"); @@ -222,17 +226,20 @@ fn main() -> Result<()> { console.setup_global_trace(args.level)?; interrupt::install_handler(); - let source_path: &Utf8Path = args.dir.as_deref().unwrap_or(Utf8Path::new(".")); - let workspace_dir = cargo::find_workspace(source_path)?; + let start_dir: &Utf8Path = args.dir.as_deref().unwrap_or(Utf8Path::new(".")); + let workspace = Workspace::open(&start_dir)?; + // let discovered_workspace = discover_packages(start_dir, false, &args.mutate_packages)?; + // let workspace_dir = &discovered_workspace.workspace_dir; let config = if args.no_config { config::Config::default() } else { - config::Config::read_tree_config(&workspace_dir)? + config::Config::read_tree_config(&workspace.dir)? }; debug!(?config); let options = Options::new(&args, &config)?; debug!(?options); - let discovered = walk_tree(&workspace_dir, &args.mutate_packages, &options, &console)?; + let package_filter = PackageFilter::All; // TODO: From args + let discovered = workspace.discover(&package_filter, &options, &console)?; if args.list_files { console.clear(); list_files(FmtToIoWrite::new(io::stdout()), discovered, &options)?; @@ -240,7 +247,7 @@ fn main() -> Result<()> { console.clear(); list_mutants(FmtToIoWrite::new(io::stdout()), discovered, &options)?; } else { - let lab_outcome = test_mutants(discovered.mutants, &workspace_dir, options, &console)?; + let lab_outcome = test_mutants(discovered.mutants, &workspace.dir, options, &console)?; exit(lab_outcome.exit_code()); } Ok(()) diff --git a/src/mutate.rs b/src/mutate.rs index 2a59f42d..41d38704 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -14,7 +14,7 @@ use serde::Serialize; use similar::TextDiff; use crate::build_dir::BuildDir; -use crate::source::Package; +use crate::package::Package; use crate::source::SourceFile; use crate::textedit::{replace_region, Span}; @@ -211,11 +211,11 @@ mod test { #[test] fn discover_factorial_mutants() { let tree_path = Utf8Path::new("testdata/tree/factorial"); - let workspace_dir = cargo::find_workspace(tree_path).unwrap(); + let workspace = Workspace::open(tree_path).unwrap(); let options = Options::default(); - let mutants = walk_tree(&workspace_dir, &[], &options, &Console::new()) - .unwrap() - .mutants; + let mutants = workspace + .mutants(&PackageFilter::All, &options, &Console::new()) + .unwrap(); assert_eq!(mutants.len(), 3); assert_eq!( format!("{:?}", mutants[0]), @@ -264,15 +264,10 @@ mod test { #[test] fn filter_by_attributes() { - let tree_path = Utf8Path::new("testdata/tree/hang_avoided_by_attr"); - let mutants = walk_tree( - &cargo::find_workspace(tree_path).unwrap(), - &[], - &Options::default(), - &Console::new(), - ) - .unwrap() - .mutants; + let mutants = Workspace::open(Utf8Path::new("testdata/tree/hang_avoided_by_attr")) + .unwrap() + .mutants(&PackageFilter::All, &Options::default(), &Console::new()) + .unwrap(); let descriptions = mutants.iter().map(Mutant::describe_change).collect_vec(); insta::assert_snapshot!( descriptions.join("\n"), @@ -281,12 +276,13 @@ mod test { } #[test] - fn mutate_factorial() { + fn mutate_factorial() -> Result<()> { let tree_path = Utf8Path::new("testdata/tree/factorial"); - let source_tree = cargo::find_workspace(tree_path).unwrap(); - let mutants = walk_tree(&source_tree, &[], &Options::default(), &Console::new()) - .unwrap() - .mutants; + let mutants = Workspace::open(&tree_path)?.mutants( + &PackageFilter::All, + &Options::default(), + &Console::new(), + )?; assert_eq!(mutants.len(), 3); let mut mutated_code = mutants[0].mutated_code(); @@ -340,5 +336,6 @@ mod test { "# } ); + Ok(()) } } diff --git a/src/output.rs b/src/output.rs index 611da8b8..54e44183 100644 --- a/src/output.rs +++ b/src/output.rs @@ -279,13 +279,14 @@ mod test { #[test] fn create_output_dir() { let tmp = minimal_source_tree(); - let tmp_path = tmp.path().try_into().unwrap(); - let root = cargo::find_workspace(tmp_path).unwrap(); - let output_dir = OutputDir::new(&root).unwrap(); + let tmp_path: &Utf8Path = tmp.path().try_into().unwrap(); + let workspace = Workspace::open(tmp_path).unwrap(); + let output_dir = OutputDir::new(&workspace.dir).unwrap(); assert_eq!( list_recursive(tmp.path()), &[ "", + "Cargo.lock", "Cargo.toml", "mutants.out", "mutants.out/caught.txt", @@ -298,8 +299,8 @@ mod test { "src/lib.rs", ] ); - assert_eq!(output_dir.path(), root.join("mutants.out")); - assert_eq!(output_dir.log_dir, root.join("mutants.out/log")); + assert_eq!(output_dir.path(), workspace.dir.join("mutants.out")); + assert_eq!(output_dir.log_dir, workspace.dir.join("mutants.out/log")); assert!(output_dir.path().join("lock.json").is_file()); } diff --git a/src/package.rs b/src/package.rs new file mode 100644 index 00000000..c550b918 --- /dev/null +++ b/src/package.rs @@ -0,0 +1,23 @@ +// Copyright 2023 Martin Pool + +//! Discover and represent cargo packages within a workspace. + +use std::sync::Arc; + +use anyhow::{anyhow, Context}; +use camino::{Utf8Path, Utf8PathBuf}; +use itertools::Itertools; +use tracing::{debug_span, warn}; + +use crate::source::SourceFile; +use crate::*; + +/// A package built and tested as a unit. +#[derive(Debug, Eq, PartialEq, Hash, Clone)] +pub struct Package { + /// The short name of the package, like "mutants". + pub name: String, + + /// For Cargo, the path of the `Cargo.toml` manifest file, relative to the top of the tree. + pub relative_manifest_path: Utf8PathBuf, +} 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 e3d2e0b1..3823baf0 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 @@ -13,11 +13,8 @@ src/build_dir.rs: replace copy_tree -> Result with Ok(Default::default( src/build_dir.rs: replace copy_tree -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace run_cargo -> Result with Ok(Default::default()) src/cargo.rs: replace run_cargo -> Result with Err(::anyhow::anyhow!("mutated!")) -src/cargo.rs: replace find_workspace -> Result with Ok(Default::default()) -src/cargo.rs: replace find_workspace -> Result with Err(::anyhow::anyhow!("mutated!")) -src/cargo.rs: replace top_source_files -> Result>> with Ok(vec![]) -src/cargo.rs: replace top_source_files -> Result>> with Ok(vec![Arc::new(Default::default())]) -src/cargo.rs: replace top_source_files -> Result>> with Err(::anyhow::anyhow!("mutated!")) +src/cargo.rs: replace run_cargo_metadata -> Result with Ok(Default::default()) +src/cargo.rs: replace run_cargo_metadata -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace cargo_bin -> String with String::new() src/cargo.rs: replace cargo_bin -> String with "xyzzy".into() src/cargo.rs: replace cargo_argv -> Vec with vec![] @@ -25,11 +22,6 @@ src/cargo.rs: replace cargo_argv -> Vec with vec![String::new()] src/cargo.rs: replace cargo_argv -> Vec with vec!["xyzzy".into()] src/cargo.rs: replace rustflags -> String with String::new() src/cargo.rs: replace rustflags -> String with "xyzzy".into() -src/cargo.rs: replace direct_package_sources -> Result> with Ok(vec![]) -src/cargo.rs: replace direct_package_sources -> Result> with Ok(vec![Default::default()]) -src/cargo.rs: replace direct_package_sources -> Result> with Err(::anyhow::anyhow!("mutated!")) -src/cargo.rs: replace should_mutate_target -> bool with true -src/cargo.rs: replace should_mutate_target -> bool with false src/config.rs: replace Config::read_file -> Result with Ok(Default::default()) src/config.rs: replace Config::read_file -> Result with Err(::anyhow::anyhow!("mutated!")) src/config.rs: replace Config::read_tree_config -> Result with Ok(Default::default()) @@ -335,4 +327,32 @@ src/visit.rs: replace path_is -> bool with true src/visit.rs: replace path_is -> bool with false src/visit.rs: replace attr_is_mutants_skip -> bool with true src/visit.rs: replace attr_is_mutants_skip -> bool with false +src/workspace.rs: replace ::fmt -> fmt::Result with Ok(Default::default()) +src/workspace.rs: replace ::fmt -> fmt::Result with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace PackageFilter::explicit -> PackageFilter with Default::default() +src/workspace.rs: replace Workspace::open -> Result with Ok(Default::default()) +src/workspace.rs: replace Workspace::open -> Result with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace Workspace::packages -> Result>> with Ok(vec![]) +src/workspace.rs: replace Workspace::packages -> Result>> with Ok(vec![Arc::new(Default::default())]) +src/workspace.rs: replace Workspace::packages -> Result>> with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace Workspace::top_package_sources -> Result> with Ok(vec![]) +src/workspace.rs: replace Workspace::top_package_sources -> Result> with Ok(vec![Default::default()]) +src/workspace.rs: replace Workspace::top_package_sources -> Result> with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace Workspace::top_sources -> Result>> with Ok(vec![]) +src/workspace.rs: replace Workspace::top_sources -> Result>> with Ok(vec![Arc::new(Default::default())]) +src/workspace.rs: replace Workspace::top_sources -> Result>> with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace Workspace::discover -> Result with Ok(Default::default()) +src/workspace.rs: replace Workspace::discover -> Result with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace Workspace::mutants -> Result> with Ok(vec![]) +src/workspace.rs: replace Workspace::mutants -> Result> with Ok(vec![Default::default()]) +src/workspace.rs: replace Workspace::mutants -> Result> with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace filter_package_metadata -> Vec<&'m cargo_metadata::Package> with vec![] +src/workspace.rs: replace filter_package_metadata -> Vec<&'m cargo_metadata::Package> with vec![&Default::default()] +src/workspace.rs: replace direct_package_sources -> Result> with Ok(vec![]) +src/workspace.rs: replace direct_package_sources -> Result> with Ok(vec![Default::default()]) +src/workspace.rs: replace direct_package_sources -> Result> with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace should_mutate_target -> bool with true +src/workspace.rs: replace should_mutate_target -> bool with false +src/workspace.rs: replace find_workspace -> Result with Ok(Default::default()) +src/workspace.rs: replace find_workspace -> Result with Err(::anyhow::anyhow!("mutated!")) diff --git a/src/source.rs b/src/source.rs index 44eaa173..1984f12c 100644 --- a/src/source.rs +++ b/src/source.rs @@ -9,6 +9,7 @@ use camino::{Utf8Path, Utf8PathBuf}; #[allow(unused_imports)] use tracing::{debug, info, warn}; +use crate::package::Package; use crate::path::Utf8PathSlashes; /// A Rust source file within a source tree. @@ -23,7 +24,7 @@ pub struct SourceFile { /// Package within the workspace. pub package: Arc, - /// Path relative to the root of the tree. + /// Path of this source file relative to workspace. pub tree_relative_path: Utf8PathBuf, /// Full copy of the source. @@ -56,15 +57,6 @@ impl SourceFile { } } -/// A package built and tested as a unit. -#[derive(Debug, Eq, PartialEq, Hash, Clone)] -pub struct Package { - /// The short name of the package, like "mutants". - pub name: String, - /// For Cargo, the path of the `Cargo.toml` manifest file, relative to the top of the tree. - pub relative_manifest_path: Utf8PathBuf, -} - #[cfg(test)] mod test { use std::fs::File; diff --git a/src/visit.rs b/src/visit.rs index fec886f9..af2ebef6 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -17,7 +17,6 @@ use syn::visit::Visit; use syn::{Attribute, Expr, ItemFn, ReturnType}; use tracing::{debug, debug_span, trace, trace_span, warn}; -use crate::cargo::top_source_files; use crate::fnvalue::return_type_replacements; use crate::pretty::ToPrettyString; use crate::source::SourceFile; @@ -36,21 +35,20 @@ pub struct Discovered { /// /// The list of source files includes even those with no mutants. /// -/// `mutate_packages`: If non-empty, only generate mutants from these packages. pub fn walk_tree( workspace_dir: &Utf8Path, - mutate_packages: &[String], + top_source_files: &[Arc], options: &Options, console: &Console, ) -> Result { + // TODO: Lift up parsing the error expressions... let error_exprs = options .error_values .iter() .map(|e| syn::parse_str(e).with_context(|| format!("Failed to parse error value {e:?}"))) .collect::>>()?; console.walk_tree_start(); - let mut file_queue: VecDeque> = - top_source_files(workspace_dir, mutate_packages)?.into(); + let mut file_queue: VecDeque> = top_source_files.iter().cloned().collect(); let mut mutants = Vec::new(); let mut files: Vec> = Vec::new(); while let Some(source_file) = file_queue.pop_front() { @@ -413,12 +411,16 @@ mod test { ..Default::default() }; let mut list_output = String::new(); - let workspace_dir = &Utf8Path::new(".") - .canonicalize_utf8() - .expect("Canonicalize source path"); let console = Console::new(); - let discovered = - walk_tree(workspace_dir, &[], &options, &console).expect("Discover mutants"); + let workspace = Workspace::open( + &Utf8Path::new(".") + .canonicalize_utf8() + .expect("Canonicalize source path"), + ) + .unwrap(); + let discovered = workspace + .discover(&PackageFilter::All, &options, &console) + .expect("Discover mutants"); crate::list_mutants(&mut list_output, discovered, &options) .expect("Discover mutants in own source tree"); diff --git a/src/workspace.rs b/src/workspace.rs new file mode 100644 index 00000000..10ce8e78 --- /dev/null +++ b/src/workspace.rs @@ -0,0 +1,357 @@ +// Copyright 2023 Martin Pool + +use std::fmt; +use std::sync::Arc; + +use anyhow::{anyhow, ensure, Context}; +use camino::{Utf8Path, Utf8PathBuf}; +use itertools::Itertools; +use serde_json::Value; +use tracing::{debug, debug_span, 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::process::get_command_output; +use crate::source::SourceFile; +use crate::visit::{walk_tree, Discovered}; +use crate::Result; + +pub struct Workspace { + pub dir: Utf8PathBuf, + metadata: cargo_metadata::Metadata, +} + +impl fmt::Debug for Workspace { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("Workspace") + .field("dir", &self.dir) + // .field("metadata", &self.metadata) + .finish() + } +} + +pub enum PackageFilter { + All, + Explicit(Vec), + Auto(Utf8PathBuf), +} + +impl PackageFilter { + pub fn explicit>(names: I) -> PackageFilter { + PackageFilter::Explicit(names.into_iter().map(|s| s.to_string()).collect_vec()) + } +} + +impl Workspace { + pub fn open(start_dir: &Utf8Path) -> Result { + let dir = find_workspace(start_dir)?; + let cargo_toml_path = dir.join("Cargo.toml"); + debug!(?cargo_toml_path, ?dir, "Find root files"); + check_interrupted()?; + let metadata = cargo_metadata::MetadataCommand::new() + .manifest_path(&cargo_toml_path) + .exec() + .context("run cargo metadata")?; + Ok(Workspace { dir, metadata }) + } + + /// Find packages to mutate, subject to some filtering. + pub fn packages(&self, package_filter: &PackageFilter) -> Result>> { + let mut packages = Vec::new(); + for package_metadata in filter_package_metadata(&self.metadata, package_filter) + .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(&self.dir) + .map_err(|_| { + anyhow!( + "manifest path {manifest_path:?} for package {name:?} is not \ + within the detected source root path {dir:?}", + dir = self.dir + ) + })? + .to_owned(); + let package = Arc::new(Package { + name: package_metadata.name.clone(), + relative_manifest_path, + }); + packages.push(package); + } + if let PackageFilter::Explicit(names) = package_filter { + for wanted in names { + if !packages.iter().any(|found| found.name == *wanted) { + warn!("package {wanted:?} not found in source tree"); + } + } + } + Ok(packages) + } + + /// Return the top source files (like `src/lib.rs`) for a named package. + fn top_package_sources(&self, package_name: &str) -> Result> { + if let Some(package_metadata) = self + .metadata + .workspace_packages() + .iter() + .find(|p| p.name == package_name) + { + direct_package_sources(&self.dir, package_metadata) + } else { + Err(anyhow!( + "package {package_name:?} not found in workspace metadata" + )) + } + } + + /// Find all the top source files for selected packages. + pub fn top_sources(&self, package_filter: &PackageFilter) -> Result>> { + let mut sources = Vec::new(); + for package in self.packages(package_filter)? { + for source_path in self.top_package_sources(&package.name)? { + sources.push(Arc::new(SourceFile::new( + &self.dir, + source_path.to_owned(), + &package, + )?)); + } + } + Ok(sources) + } + + /// Make all the mutants from the filtered packages in this workspace. + pub fn discover( + &self, + package_filter: &PackageFilter, + options: &Options, + console: &Console, + ) -> Result { + walk_tree( + &self.dir, + &self.top_sources(package_filter)?, + options, + console, + ) + } + + pub fn mutants( + &self, + package_filter: &PackageFilter, + options: &Options, + console: &Console, + ) -> Result> { + Ok(self.discover(package_filter, options, console)?.mutants) + } +} + +fn filter_package_metadata<'m>( + metadata: &'m cargo_metadata::Metadata, + package_filter: &PackageFilter, +) -> Vec<&'m cargo_metadata::Package> { + metadata + .workspace_packages() + .iter() + .filter(move |pmeta| match package_filter { + PackageFilter::All => true, + PackageFilter::Explicit(include_names) => include_names.contains(&pmeta.name), + PackageFilter::Auto(..) => todo!(), + }) + .sorted_by_key(|pm| &pm.name) + .copied() + .collect() +} + +/// 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. +fn direct_package_sources( + workspace_root: &Utf8Path, + package_metadata: &cargo_metadata::Package, +) -> Result> { + let mut found = Vec::new(); + let pkg_dir = package_metadata.manifest_path.parent().unwrap(); + for target in &package_metadata.targets { + if should_mutate_target(target) { + if let Ok(relpath) = target + .src_path + .strip_prefix(workspace_root) + .map(ToOwned::to_owned) + { + debug!( + "found mutation target {} of kind {:?}", + relpath, target.kind + ); + found.push(relpath); + } else { + warn!("{:?} is not in {:?}", target.src_path, pkg_dir); + } + } else { + debug!( + "skipping target {:?} of kinds {:?}", + target.name, target.kind + ); + } + } + found.sort(); + found.dedup(); + Ok(found) +} + +fn should_mutate_target(target: &cargo_metadata::Target) -> bool { + target.kind.iter().any(|k| k.ends_with("lib") || k == "bin") +} + +/// Return the path of the workspace directory enclosing a given directory. +fn find_workspace(path: &Utf8Path) -> Result { + ensure!(path.is_dir(), "{path:?} is not a directory"); + let cargo_bin = cargo_bin(); // needed for lifetime + let argv: Vec<&str> = vec![&cargo_bin, "locate-project", "--workspace"]; + let stdout = get_command_output(&argv, path) + .with_context(|| format!("run cargo locate-project in {path:?}"))?; + let val: Value = serde_json::from_str(&stdout).context("parse cargo locate-project output")?; + let cargo_toml_path: Utf8PathBuf = val["root"] + .as_str() + .with_context(|| format!("cargo locate-project output has no root: {stdout:?}"))? + .to_owned() + .into(); + debug!(?cargo_toml_path, "Found workspace root manifest"); + ensure!( + cargo_toml_path.is_file(), + "cargo locate-project root {cargo_toml_path:?} is not a file" + ); + let root = cargo_toml_path + .parent() + .ok_or_else(|| anyhow!("cargo locate-project root {cargo_toml_path:?} has no parent"))? + .to_owned(); + ensure!( + root.is_dir(), + "apparent project root directory {root:?} is not a directory" + ); + Ok(root) +} + +#[cfg(test)] +mod test { + use std::ffi::OsStr; + + use camino::Utf8Path; + use itertools::Itertools; + + use crate::console::Console; + use crate::options::Options; + use crate::workspace::PackageFilter; + + use super::Workspace; + + #[test] + fn error_opening_outside_of_crate() { + Workspace::open(&Utf8Path::new("/")).unwrap_err(); + } + + #[test] + fn open_subdirectory_of_crate_opens_the_crate() { + let workspace = Workspace::open(Utf8Path::new("testdata/tree/factorial/src")) + .expect("open source tree from subdirectory"); + let root = &workspace.dir; + assert!(root.is_dir()); + assert!(root.join("Cargo.toml").is_file()); + assert!(root.join("src/bin/factorial.rs").is_file()); + assert_eq!(root.file_name().unwrap(), OsStr::new("factorial")); + } + + #[test] + fn find_root_from_subdirectory_of_workspace_finds_the_workspace_root() { + let root = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) + .expect("Find root from within workspace/main") + .dir; + assert_eq!(root.file_name(), Some("workspace"), "Wrong root: {root:?}"); + } + + #[test] + fn find_top_source_files_from_subdirectory_of_workspace() { + let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) + .expect("Find workspace root"); + assert_eq!( + workspace + .packages(&PackageFilter::All) + .unwrap() + .iter() + .map(|p| p.name.clone()) + .collect_vec(), + ["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(), + // ordered by package name + ["utils/src/lib.rs", "main/src/main.rs", "main2/src/main.rs"] + ); + } + + #[test] + fn filter_by_single_package() { + let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) + .expect("Find workspace root"); + let root_dir = &workspace.dir; + assert_eq!( + root_dir.file_name(), + Some("workspace"), + "found the workspace root" + ); + let filter = PackageFilter::explicit(["main"]); + assert_eq!( + workspace + .packages(&filter) + .unwrap() + .iter() + .map(|p| p.name.clone()) + .collect_vec(), + ["main"] + ); + let top_sources = workspace.top_sources(&filter).unwrap(); + println!("{top_sources:#?}"); + assert_eq!( + top_sources + .iter() + .map(|sf| sf.tree_relative_path.clone()) + .collect_vec(), + [Utf8Path::new("main/src/main.rs")] + ); + } + + #[test] + fn filter_by_multiple_packages() { + let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")).unwrap(); + assert_eq!( + workspace.dir.file_name(), + Some("workspace"), + "found the workspace root" + ); + let selection = PackageFilter::explicit(["main", "main2"]); + let discovered = workspace + .discover(&selection, &Options::default(), &Console::new()) + .unwrap(); + + assert_eq!( + discovered + .files + .iter() + .map(|sf| sf.tree_relative_path.clone()) + .collect_vec(), + ["main/src/main.rs", "main2/src/main.rs"] + ); + } +} From b723e4176c1ce9a1b3b146749d3b8303a53670dc Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 7 Nov 2023 17:01:01 -0800 Subject: [PATCH 21/36] impl Default for Console --- src/console.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/console.rs b/src/console.rs index ba0eb519..aa8279f2 100644 --- a/src/console.rs +++ b/src/console.rs @@ -267,6 +267,12 @@ impl Console { } } +impl Default for Console { + fn default() -> Self { + Self::new() + } +} + /// Write trace output to the terminal via the console. pub struct TerminalWriter { view: Arc>, From d52213af40050cc0fe2ccf8bbe304bccc9fb78c4 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 7 Nov 2023 17:01:18 -0800 Subject: [PATCH 22/36] Clean up --- src/cargo.rs | 28 ++-------- src/main.rs | 15 +++-- src/mutate.rs | 2 +- src/package.rs | 10 +--- ..._expected_mutants_for_own_source_tree.snap | 8 +-- src/workspace.rs | 55 +++++++++++-------- tests/cli/workspace.rs | 2 +- 7 files changed, 51 insertions(+), 69 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index cb959a7a..b4ee142b 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -3,21 +3,16 @@ //! Run Cargo as a subprocess, including timeouts and propagating signals. use std::env; -use std::sync::Arc; use std::time::{Duration, Instant}; -use anyhow::{anyhow, ensure, Context, Result}; -use camino::{Utf8Path, Utf8PathBuf}; -use cargo_metadata::Metadata; +use anyhow::Result; +use camino::Utf8Path; use itertools::Itertools; -use serde_json::Value; -use tracing::debug_span; -#[allow(unused_imports)] -use tracing::{debug, error, info, span, trace, warn, Level}; +use tracing::{debug, debug_span}; use crate::outcome::PhaseResult; use crate::package::Package; -use crate::process::{get_command_output, Process}; +use crate::process::Process; use crate::*; /// Run cargo build, check, or test. @@ -51,18 +46,6 @@ pub fn run_cargo( }) } -pub fn run_cargo_metadata(workspace_dir: &Utf8Path) -> Result { - let cargo_toml_path = workspace_dir.join("Cargo.toml"); - debug!(?cargo_toml_path, ?workspace_dir, "run cargo metadata"); - check_interrupted()?; - let metadata = cargo_metadata::MetadataCommand::new() - .manifest_path(&cargo_toml_path) - .exec() - .context("run cargo metadata")?; - check_interrupted()?; - Ok(metadata) -} - /// Return the name of the cargo binary. pub fn cargo_bin() -> String { // When run as a Cargo subcommand, which is the usual/intended case, @@ -140,9 +123,8 @@ fn rustflags() -> String { #[cfg(test)] mod test { - use std::ffi::OsStr; + use std::sync::Arc; - use itertools::Itertools; use pretty_assertions::assert_eq; use crate::{Options, Phase}; diff --git a/src/main.rs b/src/main.rs index ad69885e..1d7fbe5f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -53,10 +53,7 @@ use crate::options::Options; use crate::outcome::{Phase, ScenarioOutcome}; use crate::path::Utf8PathSlashes; use crate::scenario::Scenario; -use crate::source::SourceFile; -use crate::visit::walk_tree; -use crate::workspace::PackageFilter; -use crate::workspace::Workspace; +use crate::workspace::{PackageFilter, Workspace}; const VERSION: &str = env!("CARGO_PKG_VERSION"); const NAME: &str = env!("CARGO_PKG_NAME"); @@ -227,7 +224,7 @@ fn main() -> Result<()> { interrupt::install_handler(); let start_dir: &Utf8Path = args.dir.as_deref().unwrap_or(Utf8Path::new(".")); - let workspace = Workspace::open(&start_dir)?; + let workspace = Workspace::open(start_dir)?; // let discovered_workspace = discover_packages(start_dir, false, &args.mutate_packages)?; // let workspace_dir = &discovered_workspace.workspace_dir; let config = if args.no_config { @@ -238,7 +235,13 @@ fn main() -> Result<()> { debug!(?config); let options = Options::new(&args, &config)?; debug!(?options); - let package_filter = PackageFilter::All; // TODO: From args + let package_filter = if !args.mutate_packages.is_empty() { + PackageFilter::explicit(&args.mutate_packages) + } else { + // TODO: --workspace + // TODO: Actually, Auto(start_dir) if not otherwise set. + PackageFilter::All + }; let discovered = workspace.discover(&package_filter, &options, &console)?; if args.list_files { console.clear(); diff --git a/src/mutate.rs b/src/mutate.rs index 41d38704..9c56f457 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -278,7 +278,7 @@ mod test { #[test] fn mutate_factorial() -> Result<()> { let tree_path = Utf8Path::new("testdata/tree/factorial"); - let mutants = Workspace::open(&tree_path)?.mutants( + let mutants = Workspace::open(tree_path)?.mutants( &PackageFilter::All, &Options::default(), &Console::new(), diff --git a/src/package.rs b/src/package.rs index c550b918..3bb30947 100644 --- a/src/package.rs +++ b/src/package.rs @@ -2,15 +2,7 @@ //! Discover and represent cargo packages within a workspace. -use std::sync::Arc; - -use anyhow::{anyhow, Context}; -use camino::{Utf8Path, Utf8PathBuf}; -use itertools::Itertools; -use tracing::{debug_span, warn}; - -use crate::source::SourceFile; -use crate::*; +use camino::Utf8PathBuf; /// A package built and tested as a unit. #[derive(Debug, Eq, PartialEq, Hash, Clone)] 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 3823baf0..29b73f5d 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 @@ -13,8 +13,6 @@ src/build_dir.rs: replace copy_tree -> Result with Ok(Default::default( src/build_dir.rs: replace copy_tree -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace run_cargo -> Result with Ok(Default::default()) src/cargo.rs: replace run_cargo -> Result with Err(::anyhow::anyhow!("mutated!")) -src/cargo.rs: replace run_cargo_metadata -> Result with Ok(Default::default()) -src/cargo.rs: replace run_cargo_metadata -> Result with Err(::anyhow::anyhow!("mutated!")) src/cargo.rs: replace cargo_bin -> String with String::new() src/cargo.rs: replace cargo_bin -> String with "xyzzy".into() src/cargo.rs: replace cargo_argv -> Vec with vec![] @@ -335,9 +333,9 @@ src/workspace.rs: replace Workspace::open -> Result with Err(::anyhow::any src/workspace.rs: replace Workspace::packages -> Result>> with Ok(vec![]) src/workspace.rs: replace Workspace::packages -> Result>> with Ok(vec![Arc::new(Default::default())]) src/workspace.rs: replace Workspace::packages -> Result>> with Err(::anyhow::anyhow!("mutated!")) -src/workspace.rs: replace Workspace::top_package_sources -> Result> with Ok(vec![]) -src/workspace.rs: replace Workspace::top_package_sources -> Result> with Ok(vec![Default::default()]) -src/workspace.rs: replace Workspace::top_package_sources -> Result> with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace Workspace::package_tops -> Result> with Ok(vec![]) +src/workspace.rs: replace Workspace::package_tops -> Result> with Ok(vec![Default::default()]) +src/workspace.rs: replace Workspace::package_tops -> Result> with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace Workspace::top_sources -> Result>> with Ok(vec![]) src/workspace.rs: replace Workspace::top_sources -> Result>> with Ok(vec![Arc::new(Default::default())]) src/workspace.rs: replace Workspace::top_sources -> Result>> with Err(::anyhow::anyhow!("mutated!")) diff --git a/src/workspace.rs b/src/workspace.rs index 10ce8e78..174b50d8 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -46,6 +46,12 @@ impl PackageFilter { } } +/// A package and the top source files within it. +struct PackageTop { + package: Arc, + top_sources: Vec, +} + impl Workspace { pub fn open(start_dir: &Utf8Path) -> Result { let dir = find_workspace(start_dir)?; @@ -61,7 +67,16 @@ impl Workspace { /// Find packages to mutate, subject to some filtering. pub fn packages(&self, package_filter: &PackageFilter) -> Result>> { - let mut packages = Vec::new(); + Ok(self + .package_tops(package_filter)? + .into_iter() + .map(|pt| pt.package) + .collect()) + } + + /// Find all the packages and their top source files. + fn package_tops(&self, package_filter: &PackageFilter) -> Result> { + let mut tops = Vec::new(); for package_metadata in filter_package_metadata(&self.metadata, package_filter) .into_iter() .sorted_by_key(|p| &p.name) @@ -85,39 +100,30 @@ impl Workspace { name: package_metadata.name.clone(), relative_manifest_path, }); - packages.push(package); + tops.push(PackageTop { + package, + top_sources: direct_package_sources(&self.dir, package_metadata)?, + }); } if let PackageFilter::Explicit(names) = package_filter { for wanted in names { - if !packages.iter().any(|found| found.name == *wanted) { + if !tops.iter().any(|found| found.package.name == *wanted) { warn!("package {wanted:?} not found in source tree"); } } } - Ok(packages) - } - - /// Return the top source files (like `src/lib.rs`) for a named package. - fn top_package_sources(&self, package_name: &str) -> Result> { - if let Some(package_metadata) = self - .metadata - .workspace_packages() - .iter() - .find(|p| p.name == package_name) - { - direct_package_sources(&self.dir, package_metadata) - } else { - Err(anyhow!( - "package {package_name:?} not found in workspace metadata" - )) - } + Ok(tops) } /// Find all the top source files for selected packages. - pub fn top_sources(&self, package_filter: &PackageFilter) -> Result>> { + fn top_sources(&self, package_filter: &PackageFilter) -> Result>> { let mut sources = Vec::new(); - for package in self.packages(package_filter)? { - for source_path in self.top_package_sources(&package.name)? { + for PackageTop { + package, + top_sources, + } in self.package_tops(package_filter)? + { + for source_path in top_sources { sources.push(Arc::new(SourceFile::new( &self.dir, source_path.to_owned(), @@ -143,6 +149,7 @@ impl Workspace { ) } + /// Return all mutants generated from this workspace. pub fn mutants( &self, package_filter: &PackageFilter, @@ -254,7 +261,7 @@ mod test { #[test] fn error_opening_outside_of_crate() { - Workspace::open(&Utf8Path::new("/")).unwrap_err(); + Workspace::open(Utf8Path::new("/")).unwrap_err(); } #[test] diff --git a/tests/cli/workspace.rs b/tests/cli/workspace.rs index 09377798..3765d037 100644 --- a/tests/cli/workspace.rs +++ b/tests/cli/workspace.rs @@ -22,7 +22,7 @@ fn list_warns_about_unmatched_packages() { ]) .assert() .stdout(predicates::str::contains( - "package notapackage not found in source tree", + "package \"notapackage\" not found in source tree", )) .code(0); } From 7f0445c88993fb595781a966fff64134aedbfb53 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 7 Nov 2023 17:07:29 -0800 Subject: [PATCH 23/36] Factor out PackageFilter::matches --- ..._expected_mutants_for_own_source_tree.snap | 4 +-- src/workspace.rs | 32 ++++++++----------- 2 files changed, 16 insertions(+), 20 deletions(-) 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 29b73f5d..efa3fa9b 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 @@ -328,6 +328,8 @@ src/visit.rs: replace attr_is_mutants_skip -> bool with false src/workspace.rs: replace ::fmt -> fmt::Result with Ok(Default::default()) src/workspace.rs: replace ::fmt -> fmt::Result with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace PackageFilter::explicit -> PackageFilter with Default::default() +src/workspace.rs: replace PackageFilter::matches -> bool with true +src/workspace.rs: replace PackageFilter::matches -> bool with false src/workspace.rs: replace Workspace::open -> Result with Ok(Default::default()) src/workspace.rs: replace Workspace::open -> Result with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace Workspace::packages -> Result>> with Ok(vec![]) @@ -344,8 +346,6 @@ src/workspace.rs: replace Workspace::discover -> Result with Err(::a src/workspace.rs: replace Workspace::mutants -> Result> with Ok(vec![]) src/workspace.rs: replace Workspace::mutants -> Result> with Ok(vec![Default::default()]) src/workspace.rs: replace Workspace::mutants -> Result> with Err(::anyhow::anyhow!("mutated!")) -src/workspace.rs: replace filter_package_metadata -> Vec<&'m cargo_metadata::Package> with vec![] -src/workspace.rs: replace filter_package_metadata -> Vec<&'m cargo_metadata::Package> with vec![&Default::default()] src/workspace.rs: replace direct_package_sources -> Result> with Ok(vec![]) src/workspace.rs: replace direct_package_sources -> Result> with Ok(vec![Default::default()]) src/workspace.rs: replace direct_package_sources -> Result> with Err(::anyhow::anyhow!("mutated!")) diff --git a/src/workspace.rs b/src/workspace.rs index 174b50d8..af8adfef 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -44,6 +44,16 @@ impl PackageFilter { pub fn explicit>(names: I) -> PackageFilter { PackageFilter::Explicit(names.into_iter().map(|s| s.to_string()).collect_vec()) } + + pub fn matches(&self, package_metadata: &cargo_metadata::Package) -> bool { + match self { + PackageFilter::All => true, + PackageFilter::Explicit(include_names) => { + include_names.contains(&package_metadata.name) + } + PackageFilter::Auto(..) => todo!(), + } + } } /// A package and the top source files within it. @@ -77,8 +87,11 @@ impl Workspace { /// Find all the packages and their top source files. fn package_tops(&self, package_filter: &PackageFilter) -> Result> { let mut tops = Vec::new(); - for package_metadata in filter_package_metadata(&self.metadata, package_filter) + for package_metadata in self + .metadata + .workspace_packages() .into_iter() + .filter(|pmeta| package_filter.matches(pmeta)) .sorted_by_key(|p| &p.name) { check_interrupted()?; @@ -160,23 +173,6 @@ impl Workspace { } } -fn filter_package_metadata<'m>( - metadata: &'m cargo_metadata::Metadata, - package_filter: &PackageFilter, -) -> Vec<&'m cargo_metadata::Package> { - metadata - .workspace_packages() - .iter() - .filter(move |pmeta| match package_filter { - PackageFilter::All => true, - PackageFilter::Explicit(include_names) => include_names.contains(&pmeta.name), - PackageFilter::Auto(..) => todo!(), - }) - .sorted_by_key(|pm| &pm.name) - .copied() - .collect() -} - /// 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. From d214257ece12ec1c193da08b098bdfd22a7cbd12 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 7 Nov 2023 17:49:40 -0800 Subject: [PATCH 24/36] Attempted handling of auto package selection Needs tests --- src/main.rs | 5 +- ..._expected_mutants_for_own_source_tree.snap | 8 +- src/workspace.rs | 76 ++++++++++++++++--- 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/src/main.rs b/src/main.rs index 1d7fbe5f..17b8d202 100644 --- a/src/main.rs +++ b/src/main.rs @@ -238,9 +238,8 @@ fn main() -> Result<()> { let package_filter = if !args.mutate_packages.is_empty() { PackageFilter::explicit(&args.mutate_packages) } else { - // TODO: --workspace - // TODO: Actually, Auto(start_dir) if not otherwise set. - PackageFilter::All + // TODO: --workspace to ::All + PackageFilter::Auto(start_dir.to_owned()) }; let discovered = workspace.discover(&package_filter, &options, &console)?; if args.list_files { 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 efa3fa9b..3ca3a8c4 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 @@ -328,8 +328,8 @@ src/visit.rs: replace attr_is_mutants_skip -> bool with false src/workspace.rs: replace ::fmt -> fmt::Result with Ok(Default::default()) src/workspace.rs: replace ::fmt -> fmt::Result with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace PackageFilter::explicit -> PackageFilter with Default::default() -src/workspace.rs: replace PackageFilter::matches -> bool with true -src/workspace.rs: replace PackageFilter::matches -> bool with false +src/workspace.rs: replace PackageFilter::resolve_auto -> Result with Ok(Default::default()) +src/workspace.rs: replace PackageFilter::resolve_auto -> Result with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace Workspace::open -> Result with Ok(Default::default()) src/workspace.rs: replace Workspace::open -> Result with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace Workspace::packages -> Result>> with Ok(vec![]) @@ -351,6 +351,6 @@ src/workspace.rs: replace direct_package_sources -> Result> wit src/workspace.rs: replace direct_package_sources -> Result> with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace should_mutate_target -> bool with true src/workspace.rs: replace should_mutate_target -> bool with false -src/workspace.rs: replace find_workspace -> Result with Ok(Default::default()) -src/workspace.rs: replace find_workspace -> Result with Err(::anyhow::anyhow!("mutated!")) +src/workspace.rs: replace locate_project -> Result with Ok(Default::default()) +src/workspace.rs: replace locate_project -> Result with Err(::anyhow::anyhow!("mutated!")) diff --git a/src/workspace.rs b/src/workspace.rs index af8adfef..f1493739 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -34,9 +34,21 @@ impl fmt::Debug for Workspace { } } +/// Which packages to mutate in a workspace? +#[derive(Debug, Clone)] pub enum PackageFilter { + /// Include every package in the workspace. All, + /// Packages with given names, from `--package`. Explicit(Vec), + /// Automatic behavior when invoked from a subdirectory, as per + /// . + /// + /// If the directory is within a package directory, select that package. + /// + /// Otherwise, this is a "virtual workspace" directory, containing members but no + /// primary package. In this case, if there is a `default-members` field in the workspace, + /// use that list. Otherwise, apply to all members of the workspace. Auto(Utf8PathBuf), } @@ -45,13 +57,39 @@ impl PackageFilter { PackageFilter::Explicit(names.into_iter().map(|s| s.to_string()).collect_vec()) } - pub fn matches(&self, package_metadata: &cargo_metadata::Package) -> bool { - match self { - PackageFilter::All => true, - PackageFilter::Explicit(include_names) => { - include_names.contains(&package_metadata.name) + /// 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)?; + let workspace_dir = &metadata.workspace_root; + ensure!( + package_dir.strip_prefix(workspace_dir).is_ok(), + "package {package_dir:?} does not seem to be inside workspace root {workspace_dir:?}", + ); + 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])); + } } - PackageFilter::Auto(..) => todo!(), + // 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:?}?", + ); + // TODO: "This will panic if running with a version of Cargo older than 1.71."; will this break builds with old cargo? If + // so maybe we need to do this by hand, unfortunately? + let default_members = metadata.workspace_default_packages(); + if default_members.is_empty() { + Ok(PackageFilter::All) + } else { + Ok(PackageFilter::explicit( + default_members.into_iter().map(|pmeta| &pmeta.name), + )) + } + } else { + Ok(self.clone()) } } } @@ -64,7 +102,7 @@ struct PackageTop { impl Workspace { pub fn open(start_dir: &Utf8Path) -> Result { - let dir = find_workspace(start_dir)?; + let dir = locate_project(start_dir, true)?; let cargo_toml_path = dir.join("Cargo.toml"); debug!(?cargo_toml_path, ?dir, "Find root files"); check_interrupted()?; @@ -87,16 +125,21 @@ impl Workspace { /// Find all the packages and their top source files. fn package_tops(&self, package_filter: &PackageFilter) -> Result> { let mut tops = Vec::new(); + let package_filter = package_filter.resolve_auto(&self.metadata)?; for package_metadata in self .metadata .workspace_packages() .into_iter() - .filter(|pmeta| package_filter.matches(pmeta)) .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 @@ -118,7 +161,7 @@ impl Workspace { top_sources: direct_package_sources(&self.dir, package_metadata)?, }); } - if let PackageFilter::Explicit(names) = package_filter { + if let PackageFilter::Explicit(ref names) = package_filter { for wanted in names { if !tops.iter().any(|found| found.package.name == *wanted) { warn!("package {wanted:?} not found in source tree"); @@ -213,11 +256,14 @@ fn should_mutate_target(target: &cargo_metadata::Target) -> bool { target.kind.iter().any(|k| k.ends_with("lib") || k == "bin") } -/// Return the path of the workspace directory enclosing a given directory. -fn find_workspace(path: &Utf8Path) -> Result { +/// Return the path of the workspace or package directory enclosing a given directory. +fn locate_project(path: &Utf8Path, workspace: bool) -> Result { ensure!(path.is_dir(), "{path:?} is not a directory"); let cargo_bin = cargo_bin(); // needed for lifetime - let argv: Vec<&str> = vec![&cargo_bin, "locate-project", "--workspace"]; + let mut argv: Vec<&str> = vec![&cargo_bin, "locate-project"]; + if workspace { + argv.push("--workspace"); + } let stdout = get_command_output(&argv, path) .with_context(|| format!("run cargo locate-project in {path:?}"))?; let val: Value = serde_json::from_str(&stdout).context("parse cargo locate-project output")?; @@ -304,6 +350,12 @@ mod test { ); } + #[test] + fn auto_packages_in_workspace_subdir_finds_single_package() { + let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) + .expect("Find workspace root"); + } + #[test] fn filter_by_single_package() { let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) From 8e665ecc99f48e8f67d8498cb79a8fd671e6d56a Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 7 Nov 2023 17:52:05 -0800 Subject: [PATCH 25/36] Workspace::open can just take a str --- src/visit.rs | 2 +- src/workspace.rs | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/visit.rs b/src/visit.rs index af2ebef6..35a63cc7 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -413,7 +413,7 @@ mod test { let mut list_output = String::new(); let console = Console::new(); let workspace = Workspace::open( - &Utf8Path::new(".") + Utf8Path::new(".") .canonicalize_utf8() .expect("Canonicalize source path"), ) diff --git a/src/workspace.rs b/src/workspace.rs index f1493739..e9147d82 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -101,8 +101,8 @@ struct PackageTop { } impl Workspace { - pub fn open(start_dir: &Utf8Path) -> Result { - let dir = locate_project(start_dir, true)?; + pub fn open>(start_dir: P) -> Result { + let dir = locate_project(start_dir.as_ref(), true)?; let cargo_toml_path = dir.join("Cargo.toml"); debug!(?cargo_toml_path, ?dir, "Find root files"); check_interrupted()?; @@ -303,12 +303,12 @@ mod test { #[test] fn error_opening_outside_of_crate() { - Workspace::open(Utf8Path::new("/")).unwrap_err(); + Workspace::open("/").unwrap_err(); } #[test] fn open_subdirectory_of_crate_opens_the_crate() { - let workspace = Workspace::open(Utf8Path::new("testdata/tree/factorial/src")) + let workspace = Workspace::open("testdata/tree/factorial/src") .expect("open source tree from subdirectory"); let root = &workspace.dir; assert!(root.is_dir()); @@ -319,7 +319,7 @@ mod test { #[test] fn find_root_from_subdirectory_of_workspace_finds_the_workspace_root() { - let root = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) + let root = Workspace::open("testdata/tree/workspace/main") .expect("Find root from within workspace/main") .dir; assert_eq!(root.file_name(), Some("workspace"), "Wrong root: {root:?}"); @@ -327,8 +327,8 @@ mod test { #[test] fn find_top_source_files_from_subdirectory_of_workspace() { - let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) - .expect("Find workspace root"); + let workspace = + Workspace::open("testdata/tree/workspace/main").expect("Find workspace root"); assert_eq!( workspace .packages(&PackageFilter::All) @@ -352,14 +352,14 @@ mod test { #[test] fn auto_packages_in_workspace_subdir_finds_single_package() { - let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) - .expect("Find workspace root"); + let workspace = + Workspace::open("testdata/tree/workspace/main").expect("Find workspace root"); } #[test] fn filter_by_single_package() { - let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")) - .expect("Find workspace root"); + let workspace = + Workspace::open("testdata/tree/workspace/main").expect("Find workspace root"); let root_dir = &workspace.dir; assert_eq!( root_dir.file_name(), @@ -389,7 +389,7 @@ mod test { #[test] fn filter_by_multiple_packages() { - let workspace = Workspace::open(Utf8Path::new("testdata/tree/workspace/main")).unwrap(); + let workspace = Workspace::open("testdata/tree/workspace/main").unwrap(); assert_eq!( workspace.dir.file_name(), Some("workspace"), From c69d1e11266f8e86982c7abdb96634724b671e27 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 7 Nov 2023 18:00:49 -0800 Subject: [PATCH 26/36] More tests for packages and add --workspace --- book/src/workspaces.md | 9 +++++---- src/main.rs | 7 ++++++- src/workspace.rs | 32 ++++++++++++++++++++++++++++++-- tests/cli/workspace.rs | 2 +- 4 files changed, 42 insertions(+), 8 deletions(-) diff --git a/book/src/workspaces.md b/book/src/workspaces.md index b70cf40d..d049ec68 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -2,11 +2,12 @@ cargo-mutants supports testing Cargo workspaces that contain multiple packages. The entire workspace tree is copied. -By default, all source files in all packages in the workspace are tested. +By default, cargo-mutants has [the same behavior as Cargo](https://doc.rust-lang.org/cargo/reference/workspaces.html): -**NOTE: This behavior is likely to change in future: see .** - -With the `--package` option, only mutants from the package with the given name are testeg. The effect can be seen in `--list`, etc. This option can be repeated. +* 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. You can 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 diff --git a/src/main.rs b/src/main.rs index 17b8d202..dafe78b8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -191,6 +191,10 @@ struct Args { #[arg(long, action = clap::ArgAction::SetTrue)] version: bool, + /// test every package in the workspace. + #[arg(long)] + workspace: bool, + /// additional args for all cargo invocations. #[arg(long, short = 'C', allow_hyphen_values = true)] cargo_arg: Vec, @@ -237,8 +241,9 @@ fn main() -> Result<()> { debug!(?options); let package_filter = if !args.mutate_packages.is_empty() { PackageFilter::explicit(&args.mutate_packages) + } else if args.workspace { + PackageFilter::All } else { - // TODO: --workspace to ::All PackageFilter::Auto(start_dir.to_owned()) }; let discovered = workspace.discover(&package_filter, &options, &console)?; diff --git a/src/workspace.rs b/src/workspace.rs index e9147d82..27fa05d3 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -350,10 +350,38 @@ mod test { ); } + #[test] + fn package_filter_all_from_subdir_gets_everything() { + let subdir_path = Utf8Path::new("testdata/tree/workspace/main"); + let workspace = Workspace::open(subdir_path).expect("Find workspace root"); + let packages = workspace.packages(&PackageFilter::All).unwrap(); + assert_eq!( + packages.iter().map(|p| &p.name).collect_vec(), + ["cargo_mutants_testdata_workspace_utils", "main", "main2"] + ); + } + #[test] fn auto_packages_in_workspace_subdir_finds_single_package() { - let workspace = - Workspace::open("testdata/tree/workspace/main").expect("Find workspace root"); + let subdir_path = Utf8Path::new("testdata/tree/workspace/main"); + let workspace = Workspace::open(subdir_path).expect("Find workspace root"); + let packages = workspace + .packages(&PackageFilter::Auto(subdir_path.to_owned())) + .unwrap(); + assert_eq!(packages.iter().map(|p| &p.name).collect_vec(), ["main"]); + } + + #[test] + fn auto_packages_in_virtual_workspace_gets_everything() { + let path = Utf8Path::new("testdata/tree/workspace"); + let workspace = Workspace::open(path).expect("Find workspace root"); + let packages = workspace + .packages(&PackageFilter::Auto(path.to_owned())) + .unwrap(); + assert_eq!( + packages.iter().map(|p| &p.name).collect_vec(), + ["cargo_mutants_testdata_workspace_utils", "main", "main2"] + ); } #[test] diff --git a/tests/cli/workspace.rs b/tests/cli/workspace.rs index 3765d037..4393f65a 100644 --- a/tests/cli/workspace.rs +++ b/tests/cli/workspace.rs @@ -39,7 +39,7 @@ fn list_files_json_workspace() { #[test] fn list_files_as_json_in_workspace_subdir() { run() - .args(["mutants", "--list-files", "--json"]) + .args(["mutants", "--list-files", "--json", "--workspace"]) .current_dir("testdata/tree/workspace/main2") .assert() .stdout(indoc! {r#" From e26b85cebf595e7b1b12c47900bd4588b2f5864b Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 7 Nov 2023 18:11:16 -0800 Subject: [PATCH 27/36] Don't insist workspace member must be in a subdir --- src/workspace.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/workspace.rs b/src/workspace.rs index 27fa05d3..5a6abe60 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -62,10 +62,8 @@ impl PackageFilter { if let PackageFilter::Auto(dir) = &self { let package_dir = locate_project(dir, false)?; let workspace_dir = &metadata.workspace_root; - ensure!( - package_dir.strip_prefix(workspace_dir).is_ok(), - "package {package_dir:?} does not seem to be inside workspace root {workspace_dir:?}", - ); + // 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); From 492966e0b251119d9548e7795680b0d5a4f13ec5 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 17:08:54 -0800 Subject: [PATCH 28/36] Clean up --- src/lab.rs | 2 -- src/manifest.rs | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/lab.rs b/src/lab.rs index 7d2c5f82..bb560077 100644 --- a/src/lab.rs +++ b/src/lab.rs @@ -66,8 +66,6 @@ pub fn test_mutants( "cargo {} failed in an unmutated tree, so no mutants were tested", baseline_outcome.last_phase(), ); - // TODO: Maybe should be Err, but it would need to be an error that can map to the right - // exit code. return Ok(output_mutex .into_inner() .expect("lock output_dir") diff --git a/src/manifest.rs b/src/manifest.rs index 5061abb0..15b8db8c 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -161,7 +161,7 @@ mod test { use pretty_assertions::assert_eq; use toml::Table; - use super::fix_manifest_toml; + use super::{fix_cargo_config_toml, fix_manifest_toml}; #[test] fn fix_path_absolute_unchanged() { @@ -305,7 +305,7 @@ mod test { "/src/other", ]"# }; let source_dir = Utf8Path::new("/Users/jane/src/foo"); - let fixed_toml = super::fix_cargo_config_toml(cargo_config_toml, source_dir) + let fixed_toml = fix_cargo_config_toml(cargo_config_toml, source_dir) .unwrap() .expect("toml was modified"); println!("fixed toml:\n{fixed_toml}"); From dba039f6860b14e186f597503014b68b83053e57 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 17:30:55 -0800 Subject: [PATCH 29/36] Attempt to catch panic from workspace_default_packages --- src/workspace.rs | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/workspace.rs b/src/workspace.rs index 5a6abe60..deab31c3 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -1,6 +1,7 @@ // Copyright 2023 Martin Pool use std::fmt; +use std::panic::catch_unwind; use std::sync::Arc; use anyhow::{anyhow, ensure, Context}; @@ -76,15 +77,21 @@ impl PackageFilter { &package_dir == workspace_dir, "package {package_dir:?} doesn't match any child and doesn't match the workspace root {workspace_dir:?}?", ); - // TODO: "This will panic if running with a version of Cargo older than 1.71."; will this break builds with old cargo? If - // so maybe we need to do this by hand, unfortunately? - let default_members = metadata.workspace_default_packages(); - if default_members.is_empty() { - Ok(PackageFilter::All) - } else { - Ok(PackageFilter::explicit( - default_members.into_iter().map(|pmeta| &pmeta.name), - )) + // `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) + } } } else { Ok(self.clone()) From af8abaaa8982af156e8823fcda23932392fea6ff Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 18:34:26 -0800 Subject: [PATCH 30/36] Tests shouldn't fail if there are warnings Specifically there might be warnings about lacking support for default packages on old Cargo --- tests/cli/workspace.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/tests/cli/workspace.rs b/tests/cli/workspace.rs index 4393f65a..cb7688bc 100644 --- a/tests/cli/workspace.rs +++ b/tests/cli/workspace.rs @@ -7,7 +7,7 @@ use std::fs::{self, read_to_string}; use indoc::indoc; use itertools::Itertools; -use super::{copy_of_testdata, run, CommandInstaExt}; +use super::{copy_of_testdata, run}; #[test] fn list_warns_about_unmatched_packages() { @@ -21,7 +21,7 @@ fn list_warns_about_unmatched_packages() { "notapackage", ]) .assert() - .stdout(predicates::str::contains( + .stderr(predicates::str::contains( "package \"notapackage\" not found in source tree", )) .code(0); @@ -33,7 +33,23 @@ fn list_files_json_workspace() { run() .args(["mutants", "--list-files", "--json"]) .current_dir("testdata/tree/workspace") - .assert_insta("list_files_json_workspace"); + .assert() + .stdout(indoc! { r#"[ + { + "package": "cargo_mutants_testdata_workspace_utils", + "path": "utils/src/lib.rs" + }, + { + "package": "main", + "path": "main/src/main.rs" + }, + { + "package": "main2", + "path": "main2/src/main.rs" + } + ] + "# }) + .success(); } #[test] From 5b508ec9709ad32106ecf3ab16aa9cde975019b5 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 18:46:35 -0800 Subject: [PATCH 31/36] Factor out assert_bytes_eq_json --- tests/cli/config.rs | 9 +++++---- tests/cli/main.rs | 12 ++++++++++++ tests/cli/workspace.rs | 25 +++++++++++++++++-------- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/tests/cli/config.rs b/tests/cli/config.rs index 29d1a3e3..e1a3870a 100644 --- a/tests/cli/config.rs +++ b/tests/cli/config.rs @@ -4,6 +4,7 @@ use std::fs::{create_dir, write}; +use indoc::indoc; use predicates::prelude::*; use tempfile::TempDir; @@ -87,10 +88,10 @@ fn list_with_config_file_inclusion() { .arg(testdata.path()) .assert() .success() - .stdout(predicates::str::diff( - "src/inside_mod.rs -src/item_mod.rs\n", - )); + .stdout(predicates::str::diff(indoc! { "\ + src/inside_mod.rs + src/item_mod.rs + " })); run() .args(["mutants", "--list", "-d"]) .arg(testdata.path()) diff --git a/tests/cli/main.rs b/tests/cli/main.rs index d84f84b0..7e3c5d1d 100644 --- a/tests/cli/main.rs +++ b/tests/cli/main.rs @@ -2,6 +2,7 @@ //! Tests for cargo-mutants CLI layer. +use std::borrow::Borrow; use std::env; use std::fmt::Write; use std::fs::{self, read_dir}; @@ -97,6 +98,17 @@ fn redact_timestamps_sizes(s: &str) -> String { SIZE_RE.replace_all(&s, "xxx MB").to_string() } +/// Assert that some bytes, when parsed as json, equal a json value. +fn assert_bytes_eq_json>(actual: &[u8], expected: J) { + // The Borrow is so that you can pass either a value or a reference, for easier + // calling. + let actual_json = std::str::from_utf8(actual) + .expect("bytes are UTF-8") + .parse::() + .expect("bytes can be parsed as JSON"); + assert_eq!(&actual_json, expected.borrow()); +} + #[test] fn incorrect_cargo_subcommand() { // argv[1] "mutants" is missing here. diff --git a/tests/cli/workspace.rs b/tests/cli/workspace.rs index cb7688bc..79b4e3d5 100644 --- a/tests/cli/workspace.rs +++ b/tests/cli/workspace.rs @@ -6,8 +6,9 @@ use std::fs::{self, read_to_string}; use indoc::indoc; use itertools::Itertools; +use serde_json::json; -use super::{copy_of_testdata, run}; +use super::{assert_bytes_eq_json, copy_of_testdata, run}; #[test] fn list_warns_about_unmatched_packages() { @@ -30,11 +31,15 @@ fn list_warns_about_unmatched_packages() { #[test] fn list_files_json_workspace() { // Demonstrates that we get package names in the json listing. - run() + let cmd = run() .args(["mutants", "--list-files", "--json"]) .current_dir("testdata/tree/workspace") .assert() - .stdout(indoc! { r#"[ + .success(); + assert_bytes_eq_json( + &cmd.get_output().stdout, + json! { + [ { "package": "cargo_mutants_testdata_workspace_utils", "path": "utils/src/lib.rs" @@ -48,17 +53,20 @@ fn list_files_json_workspace() { "path": "main2/src/main.rs" } ] - "# }) - .success(); + }, + ); } #[test] fn list_files_as_json_in_workspace_subdir() { - run() + let cmd = run() .args(["mutants", "--list-files", "--json", "--workspace"]) .current_dir("testdata/tree/workspace/main2") .assert() - .stdout(indoc! {r#" + .success(); + assert_bytes_eq_json( + &cmd.get_output().stdout, + json! { [ { "package": "cargo_mutants_testdata_workspace_utils", @@ -73,7 +81,8 @@ fn list_files_as_json_in_workspace_subdir() { "path": "main2/src/main.rs" } ] - "#}); + }, + ); } #[test] From fcd92575d08bacffdd44d4ad95570405f69f85d6 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 18:49:44 -0800 Subject: [PATCH 32/36] Better news about package changes --- NEWS.md | 4 ++-- book/src/workspaces.md | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/NEWS.md b/NEWS.md index 5cc48a8a..5b6836be 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,12 +2,12 @@ ## Unreleased +- Changed: `cargo mutants` now tries to match the behavior of `cargo test` when run within a workspace. If run in a package directory, it tests only that package. If run in a workspace that is not a package (a "virtual workspace"), it tests the configured default packages, or otherwise all packages. This can all be overridden with the `--package` or `--workspace` options. + - New: generate key-value map values from types like `BTreeMap>`. - Changed: Send trace messages to stderr rather stdout, in part so that it won't pollute json output. -- New: `--package` option tests only mutants from that package. - ## 23.10.0 - The baseline test (with no mutants) now tests only the packages in which diff --git a/book/src/workspaces.md b/book/src/workspaces.md index d049ec68..b944093f 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -9,13 +9,13 @@ By default, cargo-mutants has [the same behavior as Cargo](https://doc.rust-lang * 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. -You can 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. - 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. The baseline tests exercise all and only the packages for which mutants will be generated. + +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. From c42944094cd595e478fdf47fdbbfb49eacf7971d Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 19:01:45 -0800 Subject: [PATCH 33/36] Update design doc --- DESIGN.md | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 0d10c4d2..7367d6e4 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -61,21 +61,15 @@ core of cargo-mutants logic to guess at valid replacements. ### Finding the workspace and packages -cargo-mutants is invoked from within, or given with `-d`, a single directory. To find mutants and run tests we first need to find the enclosing workspace and the packages within it. +cargo-mutants is invoked from within, or given with `-d`, a single directory, called the _start directory_. To find mutants and run tests we first need to find the enclosing workspace and the packages within it. -This is done basically by parsing the output of `cargo metadata`. +This is done basically by parsing the output of `cargo locate-project` and `cargo metadata`. + +We often want to test only one or a subset of packages in the workspace. This can be set explicitly with `--package` and `--workspace`, or heuristically depending on the project metadata and the start directory. For each package, cargo tells us the build targets including tests and the main library or binary. The tests are not considered for mutation, so this leaves us with some targets of interest, and for each of them cargo tells us one top source file, typically something like `src/lib.rs` or `src/main.rs`. -### Finding packages to mutate - -We may often want to test only one or a subset of packages in the workspace. - -This can be controlled by an explicit `--package` option. - -**UNIMPLEMENTED:** In non-workspace directories test only that one package. Match Cargo's heuristics for running tests from a workspace; add a `--workspace` option. - ### Discovering mutants After discovering packages and before running any tests, we discover all the potential mutants. @@ -88,7 +82,7 @@ and then walk its AST. In the course of that walk we can find three broad catego * A source pattern that cargo-mutants knows how to mutate, such as a function returning a value. * A pattern that tells cargo-mutants not to look further into this branch of the tree, such as `#[test]` or `#[mutants::skip]`. -For baseline builds and tests, we pass Cargo `--workspace` to build and test everything. For mutant builds and tests, we pass `--package` to build and test only the package containing the mutant, on the assumption that each mutant should be caught by its own package's tests. +For baseline builds and tests, we test all the packages that will later be mutated. For mutant builds and tests, we pass `--package` to build and test only the package containing the mutant, on the assumption that each mutant should be caught by its own package's tests. We may later mutate at a granularity smaller than a single function, for example by cutting out an `if` statement or a loop, but that is not yet implemented. () @@ -105,6 +99,8 @@ scratch directory. Currently, the whole workspace tree is copied. In future, possibly only the package to be mutated could be copied: this would require changes to the code that fixes up dependencies. +(This current approach assumes that all the packages are under the workspace directory, which is common but not actually required.) + ## Handling timeouts Mutations can cause a program to go into an infinite (or just very long) loop: From c1c93061d0f199894cacd966a215fe60dd1e0e8e Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 19:02:13 -0800 Subject: [PATCH 34/36] Clean up --- src/build_dir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/build_dir.rs b/src/build_dir.rs index db7d2e4c..76573aca 100644 --- a/src/build_dir.rs +++ b/src/build_dir.rs @@ -152,7 +152,7 @@ mod test { #[test] fn build_dir_debug_form() { let options = Options::default(); - let workspace = Workspace::open(Utf8Path::new("testdata/tree/factorial")).unwrap(); + let workspace = Workspace::open("testdata/tree/factorial").unwrap(); let build_dir = BuildDir::new(&workspace.dir, &options, &Console::new()).unwrap(); let debug_form = format!("{build_dir:?}"); assert!( From a87bc4387456de2904153f3861db27b8804c53fd Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 19:05:17 -0800 Subject: [PATCH 35/36] Don't mutate Debug for Workspace --- ...ants__visit__test__expected_mutants_for_own_source_tree.snap | 2 -- src/workspace.rs | 1 + 2 files changed, 1 insertion(+), 2 deletions(-) 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 3ca3a8c4..73524271 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 @@ -325,8 +325,6 @@ src/visit.rs: replace path_is -> bool with true src/visit.rs: replace path_is -> bool with false src/visit.rs: replace attr_is_mutants_skip -> bool with true src/visit.rs: replace attr_is_mutants_skip -> bool with false -src/workspace.rs: replace ::fmt -> fmt::Result with Ok(Default::default()) -src/workspace.rs: replace ::fmt -> fmt::Result with Err(::anyhow::anyhow!("mutated!")) src/workspace.rs: replace PackageFilter::explicit -> PackageFilter with Default::default() src/workspace.rs: replace PackageFilter::resolve_auto -> Result with Ok(Default::default()) src/workspace.rs: replace PackageFilter::resolve_auto -> Result with Err(::anyhow::anyhow!("mutated!")) diff --git a/src/workspace.rs b/src/workspace.rs index deab31c3..050f65ff 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -27,6 +27,7 @@ pub struct Workspace { } impl fmt::Debug for Workspace { + #[mutants::skip] fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Workspace") .field("dir", &self.dir) From 974ea0cd1cb755e9c2ea411e74ea766e3f7de671 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Fri, 10 Nov 2023 19:13:37 -0800 Subject: [PATCH 36/36] Source.code need not be in an Arc --- src/source.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/source.rs b/src/source.rs index 1984f12c..bf5ee6e5 100644 --- a/src/source.rs +++ b/src/source.rs @@ -28,7 +28,7 @@ pub struct SourceFile { pub tree_relative_path: Utf8PathBuf, /// Full copy of the source. - pub code: Arc, + pub code: String, } impl SourceFile { @@ -46,7 +46,7 @@ impl SourceFile { .replace("\r\n", "\n"); Ok(SourceFile { tree_relative_path, - code: Arc::new(code), + code, package: Arc::clone(package), }) } @@ -85,6 +85,6 @@ mod test { }), ) .unwrap(); - assert_eq!(*source_file.code, "fn main() {\n 640 << 10;\n}\n"); + assert_eq!(source_file.code, "fn main() {\n 640 << 10;\n}\n"); } }