diff --git a/NEWS.md b/NEWS.md index 9c3d87f5..dba5cfba 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,9 @@ ## Unreleased +- Fixed a bug causing an assertion failure when cargo-mutants was run from a + subdirectory of a workspace. Thanks to Adam Chalmers! + - Generate `HttpResponse::Ok().finish()` as a mutation of an Actix `HttpResponse`. ## 23.6.0 diff --git a/book/src/workspaces.md b/book/src/workspaces.md index cddb655a..5bd9995a 100644 --- a/book/src/workspaces.md +++ b/book/src/workspaces.md @@ -2,6 +2,14 @@ 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. +**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 +`--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. diff --git a/src/cargo.rs b/src/cargo.rs index b27958be..5cd32166 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; @@ -44,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) } @@ -65,9 +83,9 @@ 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"); + debug!(?cargo_toml_path, ?source_root_path, "Find root files"); check_interrupted()?; let metadata = cargo_metadata::MetadataCommand::new() .manifest_path(&cargo_toml_path) @@ -78,11 +96,16 @@ 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") + .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(), @@ -192,25 +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 { - let cargo_bin = cargo_bin(); - if !path.is_dir() { - bail!("{} is not a directory", path); - } - 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:?}")? - .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. @@ -252,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}; @@ -365,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_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 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() + .collect_vec(); + assert_eq!( + paths, + ["main/src/main.rs", "main2/src/main.rs", "utils/src/lib.rs"] + ); + } } 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)?; 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 64c76793..16db26ff 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) = 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");