From 2fde3e7afc9152384ead7a94e3228ecdc91e1c80 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 15:46:20 -0600 Subject: [PATCH 01/10] Better error message about finding workspace paths Should help make situations like #143 easier to understand, although this alone will not fix it. --- src/cargo.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index b27958be..e969d98a 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -78,11 +78,13 @@ impl Tool for CargoTool { for package_metadata in &metadata.workspace_packages() { check_interrupted()?; let _span = debug_span!("package", name = %package_metadata.name).entered(); - debug!(manifest_path = %package_metadata.manifest_path, "walk package"); - let relative_manifest_path = package_metadata - .manifest_path + let manifest_path = &package_metadata.manifest_path; + debug!(%manifest_path, "walk package"); + let relative_manifest_path = manifest_path .strip_prefix(source_root_path) - .expect("manifest_path is within source_root_path") + .with_context(|| format!( + "manifest path {manifest_path} is not within the detected source root path {source_root_path}" + ))? .to_owned(); let package = Arc::new(Package { name: package_metadata.name.clone(), From 4f8819de66b9363a955c08b50af8362bb8e0883d Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 20:11:29 -0600 Subject: [PATCH 02/10] Clean up error messages --- src/cargo.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index e969d98a..dcb22124 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -5,8 +5,7 @@ use std::env; use std::sync::Arc; -#[allow(unused_imports)] -use anyhow::{anyhow, bail, Context, Result}; +use anyhow::{anyhow, ensure, Context, Result}; use camino::{Utf8Path, Utf8PathBuf}; use serde_json::Value; use tracing::debug_span; @@ -82,9 +81,12 @@ impl Tool for CargoTool { debug!(%manifest_path, "walk package"); let relative_manifest_path = manifest_path .strip_prefix(source_root_path) - .with_context(|| format!( - "manifest path {manifest_path} is not within the detected source root path {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(), @@ -196,17 +198,15 @@ fn rustflags() -> String { /// Run `cargo locate-project` to find the path of the `Cargo.toml` enclosing this path. fn locate_cargo_toml(path: &Utf8Path) -> Result { - let cargo_bin = cargo_bin(); - if !path.is_dir() { - bail!("{} is not a directory", path); - } + 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"]; 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() - .context("cargo locate-project output has no root: {stdout:?}")? + .with_context(|| format!("cargo locate-project output has no root: {stdout:?}"))? .to_owned() .into(); assert!(cargo_toml_path.is_file()); From 8d388913e8addbdbdb59c60b19062a31bb2050c1 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 20:21:58 -0600 Subject: [PATCH 03/10] Pass --workspace to cargo locate-project We need to find the overall root of the workspace to copy it correctly. Not sure why existing tests don't catch this. Fixes #143 but needs tests. --- src/cargo.rs | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/src/cargo.rs b/src/cargo.rs index dcb22124..752610ec 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -43,12 +43,31 @@ impl Tool for CargoTool { } fn find_root(&self, path: &Utf8Path) -> Result { - let cargo_toml_path = locate_cargo_toml(path)?; + 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() - .expect("cargo_toml_path has a parent") + .ok_or_else(|| anyhow!("cargo locate-project root {cargo_toml_path:?} has no parent"))? .to_owned(); - assert!(root.is_dir()); + ensure!( + root.is_dir(), + "apparent project root directory {root:?} is not a directory" + ); Ok(root) } @@ -66,7 +85,7 @@ impl Tool for CargoTool { /// all source files. fn root_files(&self, source_root_path: &Utf8Path) -> Result>> { let cargo_toml_path = source_root_path.join("Cargo.toml"); - debug!(?cargo_toml_path, ?source_root_path, "find root files"); + debug!(?cargo_toml_path, ?source_root_path, "Find root files"); check_interrupted()?; let metadata = cargo_metadata::MetadataCommand::new() .manifest_path(&cargo_toml_path) @@ -196,23 +215,6 @@ fn rustflags() -> String { rustflags.join("\x1f") } -/// Run `cargo locate-project` to find the path of the `Cargo.toml` enclosing this path. -fn locate_cargo_toml(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"]; - 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(); - assert!(cargo_toml_path.is_file()); - Ok(cargo_toml_path) -} - /// 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 ce70a8eeb6bacb6fd340d1b830a654d330423088 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 20:46:24 -0600 Subject: [PATCH 04/10] Tests for finding workspace from subdirectory --- src/cargo.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/cargo.rs b/src/cargo.rs index 752610ec..117fa467 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -256,6 +256,7 @@ fn should_mutate_target(target: &cargo_metadata::Target) -> bool { mod test { use std::ffi::OsStr; + use itertools::Itertools; use pretty_assertions::assert_eq; use crate::{Options, Phase}; @@ -369,4 +370,32 @@ mod test { 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 = CargoTool::new() + .find_root(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_root_files_from_subdirectory_of_workspace() { + let tool = CargoTool::new(); + let root_dir = tool + .find_root(Utf8Path::new("testdata/tree/workspace/main")) + .expect("Find workspace root"); + let root_files = tool.root_files(&root_dir).expect("Find root files"); + println!("{root_files:#?}"); + assert_eq!(root_files.len(), 3); + let paths: Vec = root_files + .iter() + .map(|sf| sf.tree_relative_path.to_string()) + .sorted() + .collect_vec(); + assert_eq!( + paths, + ["main/src/main.rs", "main2/src/main.rs", "utils/src/lib.rs"] + ); + } } From 150ba23b540b1944ad7a15653affed71f41881c6 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 20:54:14 -0600 Subject: [PATCH 05/10] Trailing newline in --list-files --json --- src/main.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 65434e0e..7be538fd 100644 --- a/src/main.rs +++ b/src/main.rs @@ -270,7 +270,8 @@ fn list_files(tool: &dyn Tool, source: &Utf8Path, options: &Options, json: bool) }) .collect(), ); - serde_json::to_writer_pretty(out, &json_list)?; + serde_json::to_writer_pretty(&mut out, &json_list)?; + writeln!(out)?; } else { for file in discovered.files { writeln!(out, "{}", file.tree_relative_path)?; From 54c29b19d620d4e4b5c5439cdd104138b330c079 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 20:58:56 -0600 Subject: [PATCH 06/10] News for workspace fix --- NEWS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NEWS.md b/NEWS.md index daf57fd4..50b238a7 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,10 @@ # cargo-mutants changelog +## Unreleased + +- Fixed a bug causing an assertion failure when cargo-mutants was run from a + subdirectory of a workspace. + ## 23.6.0 - Generate `Box::leak(Box::new(...))` as a mutation of functions returning From f61df87e2e49e4d3de075706bfd18ebe942f7d2f Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 20:59:10 -0600 Subject: [PATCH 07/10] Test --list-files --json in workspace subdir --- tests/cli/main.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/cli/main.rs b/tests/cli/main.rs index abcba247..6e88f014 100644 --- a/tests/cli/main.rs +++ b/tests/cli/main.rs @@ -457,6 +457,30 @@ fn list_files_json_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"); From 9735525abb6784f5af9f07129146e3904c4a21ae Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 21:04:19 -0600 Subject: [PATCH 08/10] More docs about workspaces --- book/src/workspaces.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/book/src/workspaces.md b/book/src/workspaces.md index cddb655a..b58fad64 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -2,6 +2,12 @@ cargo-mutants supports testing Cargo workspaces that contain multiple packages. The entire workspace tree is copied. -All source files in all packages in the workspace are tested. +By default, all source files in all packages in the workspace are tested. -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. +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. From f102ae2c93f97dd4c7cdac6731ff00cf0b557bd6 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 21:04:51 -0600 Subject: [PATCH 09/10] Thanks Adam! --- NEWS.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/NEWS.md b/NEWS.md index 50b238a7..2e31f1cc 100644 --- a/NEWS.md +++ b/NEWS.md @@ -3,8 +3,8 @@ ## Unreleased - Fixed a bug causing an assertion failure when cargo-mutants was run from a - subdirectory of a workspace. - + subdirectory of a workspace. Thanks to Adam Chalmers! + ## 23.6.0 - Generate `Box::leak(Box::new(...))` as a mutation of functions returning From 4749607856e524b5b5a6ff0c446d736282832eb0 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Sep 2023 21:21:40 -0600 Subject: [PATCH 10/10] Rename to top_source_files Calling them `root_files` is too confusing with the concept of a workspace root. --- book/src/workspaces.md | 2 ++ src/cargo.rs | 12 ++++++------ src/tool.rs | 17 +++++++++++++---- src/visit.rs | 2 +- 4 files changed, 22 insertions(+), 11 deletions(-) diff --git a/book/src/workspaces.md b/book/src/workspaces.md index b58fad64..5bd9995a 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -4,6 +4,8 @@ cargo-mutants supports testing Cargo workspaces that contain multiple packages. By default, all source files in all packages in the workspace are tested. +**NOTE: This behavior might not be the best choice, and this may change in future.** + 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 117fa467..5cd32166 100644 --- a/src/cargo.rs +++ b/src/cargo.rs @@ -83,7 +83,7 @@ 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 root_files(&self, source_root_path: &Utf8Path) -> Result>> { + fn top_source_files(&self, source_root_path: &Utf8Path) -> Result>> { let cargo_toml_path = source_root_path.join("Cargo.toml"); debug!(?cargo_toml_path, ?source_root_path, "Find root files"); check_interrupted()?; @@ -380,15 +380,15 @@ mod test { } #[test] - fn find_root_files_from_subdirectory_of_workspace() { + 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")) .expect("Find workspace root"); - let root_files = tool.root_files(&root_dir).expect("Find root files"); - println!("{root_files:#?}"); - assert_eq!(root_files.len(), 3); - let paths: Vec = root_files + let top_source_files = tool.top_source_files(&root_dir).expect("Find root files"); + println!("{top_source_files:#?}"); + assert_eq!(top_source_files.len(), 3); + let paths: Vec = top_source_files .iter() .map(|sf| sf.tree_relative_path.to_string()) .sorted() diff --git a/src/tool.rs b/src/tool.rs index 97ac9d37..dac42645 100644 --- a/src/tool.rs +++ b/src/tool.rs @@ -21,19 +21,28 @@ 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 root of the package enclosing a given path. + /// 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 all the root files from whence source discovery should begin. + /// 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. - fn root_files(&self, path: &Utf8Path) -> Result>>; + /// in the manifest for each package. + /// + /// From each of these top files, we can discover more source by following `mod` + /// statements. + fn top_source_files(&self, path: &Utf8Path) -> Result>>; /// Compose argv to run one phase in this tool. fn compose_argv( diff --git a/src/visit.rs b/src/visit.rs index e867aec3..1a914624 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -47,7 +47,7 @@ pub fn walk_tree(tool: &dyn Tool, root: &Utf8Path, options: &Options) -> Result< .collect::>>()?; let mut mutants = Vec::new(); let mut files: Vec> = Vec::new(); - let mut file_queue: VecDeque> = tool.root_files(root)?.into(); + let mut file_queue: VecDeque> = tool.top_source_files(root)?.into(); while let Some(source_file) = file_queue.pop_front() { check_interrupted()?; let (mut file_mutants, more_files) =