From a508a64b6bd3eeec04ae31ad4648936c5684e212 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 12 Nov 2024 07:15:34 -0800 Subject: [PATCH 1/5] refactor: Create ExternalModRef type --- src/visit.rs | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/src/visit.rs b/src/visit.rs index 4f463e37..a4e45717 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -117,7 +117,7 @@ pub fn walk_tree( fn walk_file( source_file: &SourceFile, error_exprs: &[Expr], -) -> Result<(Vec, Vec>)> { +) -> Result<(Vec, Vec)> { let _span = debug_span!("source_file", path = source_file.tree_relative_slashes()).entered(); debug!("visit source file"); let syn_file = syn::parse_str::(source_file.code()) @@ -135,6 +135,17 @@ fn walk_file( Ok((visitor.mutants, visitor.external_mods)) } +/// Reference to an external module from a source file. +/// +/// This is approximately a list of namespace components like `["foo", "bar"]` for +/// `foo::bar`, but each may also be decorated with a `#[path="..."]` attribute, +/// and they're attributed to a location in the source. +#[derive(Clone, Debug, PartialEq, Eq)] +struct ExternalModRef { + /// Namespace components of the module path + parts: Vec, +} + /// Namespace for a module defined in a `mod foo { ... }` block or `mod foo;` statement /// /// In the context of resolving modules, a module "path" (and to some extent "name") is ambiguous: @@ -154,6 +165,7 @@ struct ModNamespace { /// Location of the module definition in the source file source_location: Span, } + impl ModNamespace { /// Returns the name of the module for filesystem purposes fn get_filesystem_name(&self) -> &Utf8Path { @@ -200,7 +212,7 @@ struct DiscoveryVisitor<'o> { /// The names from `mod foo;` statements that should be visited later, /// namespaced relative to the source file - external_mods: Vec>, + external_mods: Vec, /// Parsed error expressions, from the config file or command line. error_exprs: &'o [Expr], @@ -436,7 +448,9 @@ impl<'ast> Visit<'ast> for DiscoveryVisitor<'_> { if node.content.is_none() { // If we're already inside `mod a { ... }` and see `mod b;` then // remember [a, b] as an external module to visit later. - self.external_mods.push(self.mod_namespace_stack.clone()); + self.external_mods.push(ExternalModRef { + parts: self.mod_namespace_stack.clone(), + }); } self.in_namespace(&mod_namespace.name, |v| syn::visit::visit_item_mod(v, node)); assert_eq!(self.mod_namespace_stack.pop(), Some(mod_namespace)); @@ -534,7 +548,7 @@ fn function_body_span(block: &Block) -> Option { fn find_mod_source( tree_root: &Utf8Path, parent: &SourceFile, - mod_namespace: &[ModNamespace], + mod_namespace: &ExternalModRef, ) -> Result> { // First, work out whether the mod will be a sibling in the same directory, or // in a child directory. @@ -562,7 +576,10 @@ fn find_mod_source( // Having determined the right directory then we can follow the path attribute, or // if no path is specified, then look for either `foo.rs` or `foo/mod.rs`. - let (mod_child, mod_parents) = mod_namespace.split_last().expect("mod namespace is empty"); + let (mod_child, mod_parents) = mod_namespace + .parts + .split_last() + .expect("mod namespace is empty"); // TODO: Beyond #115, we should probably remove all special handling of // `mod.rs` here by remembering how we found this file, and whether it From 49cae57530e2423955fdf18c21f707200f904c63 Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 12 Nov 2024 07:42:17 -0800 Subject: [PATCH 2/5] Add Options::parsed_error_exprs --- src/options.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/options.rs b/src/options.rs index 00e256eb..67c1ccd0 100644 --- a/src/options.rs +++ b/src/options.rs @@ -11,6 +11,7 @@ use globset::GlobSet; use regex::RegexSet; use serde::Deserialize; use strum::{Display, EnumString}; +use syn::Expr; use tracing::warn; use crate::config::Config; @@ -307,6 +308,17 @@ impl Options { &[Phase::Build, Phase::Test] } } + + /// Return the syn ASTs for the error values, which should be inserted as return values + /// from functions returning `Result`. + pub(crate) fn parsed_error_exprs(&self) -> Result> { + self.error_values + .iter() + .map(|e| { + syn::parse_str(e).with_context(|| format!("Failed to parse error value {e:?}")) + }) + .collect() + } } /// If the first slices is non-empty, return that, otherwise the second. From d3736f1bc0761a2194bcab6ad7e48f5cbc950d9d Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 12 Nov 2024 07:42:27 -0800 Subject: [PATCH 3/5] Clean up --- src/workspace.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/workspace.rs b/src/workspace.rs index d4b4d933..62228ef8 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -259,12 +259,9 @@ fn packages_from_metadata(metadata: &Metadata) -> Result> { /// Find all the top source files for selected packages. fn top_sources(root: &Utf8Path, packages: &[Package]) -> Result> { let mut sources = Vec::new(); - for Package { - name, top_sources, .. - } in packages - { - for source_path in top_sources { - sources.extend(SourceFile::new(root, source_path.to_owned(), name, true)?); + for package in packages { + for source_path in &package.top_sources { + sources.extend(SourceFile::load(root, source_path, &package.name, true)?); } } Ok(sources) From 2f77802ff47c79db74085c5d3adebbcebf8167ae Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Tue, 12 Nov 2024 07:44:21 -0800 Subject: [PATCH 4/5] Add and use mutate_source_str Fixes #433 --- src/mutate.rs | 20 +++++++++++++++++++- src/source.rs | 42 +++++++++++++++++++++++++++++++----------- src/visit.rs | 42 +++++++++++++++++++++++++++++++++++------- 3 files changed, 85 insertions(+), 19 deletions(-) diff --git a/src/mutate.rs b/src/mutate.rs index d07074e2..f609118e 100644 --- a/src/mutate.rs +++ b/src/mutate.rs @@ -237,8 +237,9 @@ mod test { use indoc::indoc; use itertools::Itertools; use pretty_assertions::assert_eq; - use test_util::copy_of_testdata; + use crate::test_util::copy_of_testdata; + use crate::visit::mutate_source_str; use crate::*; #[test] @@ -326,6 +327,23 @@ mod test { ); } + #[test] + fn always_skip_constructors_called_new() { + let code = indoc! { r#" + struct S { + x: i32, + } + + impl S { + fn new(x: i32) -> Self { + Self { x } + } + } + "# }; + let mutants = mutate_source_str(code, &Options::default()).unwrap(); + assert_eq!(mutants, []); + } + #[test] fn mutate_factorial() -> Result<()> { let temp = copy_of_testdata("factorial"); diff --git a/src/source.rs b/src/source.rs index b441e428..0240cedd 100644 --- a/src/source.rs +++ b/src/source.rs @@ -2,6 +2,7 @@ //! Access to a Rust source tree and files. +use std::fs::read_to_string; use std::sync::Arc; use anyhow::{Context, Result}; @@ -42,33 +43,52 @@ impl SourceFile { /// Construct a SourceFile representing a file within a tree. /// /// This eagerly loads the text of the file. - pub fn new( + /// + /// This also skip files outside of the tree, returning `Ok(None)`. + pub fn load( tree_path: &Utf8Path, - tree_relative_path: Utf8PathBuf, + tree_relative_path: &Utf8Path, package_name: &str, is_top: bool, ) -> Result> { - if ascent(&tree_relative_path) > 0 { + // TODO: Perhaps the caller should be responsible for checking this? + if ascent(tree_relative_path) > 0 { warn!( "skipping source outside of tree: {:?}", tree_relative_path.to_slash_path() ); return Ok(None); } - let full_path = tree_path.join(&tree_relative_path); + let full_path = tree_path.join(tree_relative_path); let code = Arc::new( - std::fs::read_to_string(&full_path) + read_to_string(&full_path) .with_context(|| format!("failed to read source of {full_path:?}"))? .replace("\r\n", "\n"), ); Ok(Some(SourceFile { - tree_relative_path, + tree_relative_path: tree_relative_path.to_owned(), code, package_name: package_name.to_owned(), is_top, })) } + /// Construct from in-memory text. + #[cfg(test)] + pub fn from_str( + tree_relative_path: &Utf8Path, + code: &str, + package_name: &str, + is_top: bool, + ) -> SourceFile { + SourceFile { + tree_relative_path: tree_relative_path.to_owned(), + code: Arc::new(code.to_owned()), + package_name: package_name.to_owned(), + is_top, + } + } + /// Return the path of this file relative to the tree root, with forward slashes. pub fn tree_relative_slashes(&self) -> String { self.tree_relative_path.to_slash_path() @@ -109,9 +129,9 @@ mod test { .write_all(b"fn main() {\r\n 640 << 10;\r\n}\r\n") .unwrap(); - let source_file = SourceFile::new( + let source_file = SourceFile::load( temp_dir_path, - file_name.parse().unwrap(), + Utf8Path::new(file_name), "imaginary-package", true, ) @@ -122,9 +142,9 @@ mod test { #[test] fn skips_files_outside_of_workspace() { - let source_file = SourceFile::new( - &Utf8PathBuf::from("unimportant"), - "../outside_workspace.rs".parse().unwrap(), + let source_file = SourceFile::load( + Utf8Path::new("unimportant"), + Utf8Path::new("../outside_workspace.rs"), "imaginary-package", true, ) diff --git a/src/visit.rs b/src/visit.rs index a4e45717..642fe887 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -58,11 +58,7 @@ pub fn walk_tree( options: &Options, console: &Console, ) -> Result { - let error_exprs = options - .error_values - .iter() - .map(|e| syn::parse_str(e).with_context(|| format!("Failed to parse error value {e:?}"))) - .collect::>>()?; + let error_exprs = options.parsed_error_exprs()?; console.walk_tree_start(); let mut file_queue: VecDeque = top_source_files.iter().cloned().collect(); let mut mutants = Vec::new(); @@ -77,9 +73,9 @@ pub fn walk_tree( // `--list-files`. for mod_namespace in &external_mods { if let Some(mod_path) = find_mod_source(workspace_dir, &source_file, mod_namespace)? { - file_queue.extend(SourceFile::new( + file_queue.extend(SourceFile::load( workspace_dir, - mod_path, + &mod_path, &source_file.package_name, false, )?) @@ -135,6 +131,21 @@ fn walk_file( Ok((visitor.mutants, visitor.external_mods)) } +/// For testing: parse and generate mutants from one single file provided as a string. +/// +/// The source code is assumed to be named `src/main.rs` with a fixed package name. +#[cfg(test)] +pub fn mutate_source_str(code: &str, options: &Options) -> Result> { + let source_file = SourceFile::from_str( + Utf8Path::new("src/main.rs"), + code, + "cargo-mutants-testdata-internal", + true, + ); + let (mutants, _) = walk_file(&source_file, &options.parsed_error_exprs()?)?; + Ok(mutants) +} + /// Reference to an external module from a source file. /// /// This is approximately a list of namespace components like `["foo", "bar"]` for @@ -853,4 +864,21 @@ mod test { Err("/leading_slash/../and_dots.rs".to_owned()) ); } + + /// Demonstrate that we can generate mutants from a string, without needing a whole tree. + #[test] + fn mutants_from_test_str() { + let options = Options::default(); + let mutants = mutate_source_str( + indoc! {" + fn always_true() -> bool { true } + "}, + &options, + ) + .expect("walk_file_string"); + assert_eq!( + mutants.iter().map(|m| m.name(false, false)).collect_vec(), + ["src/main.rs: replace always_true -> bool with false"] + ); + } } From 2bc86868af9ef0a421a65bb97d8d8bbed7f523ff Mon Sep 17 00:00:00 2001 From: Martin Pool Date: Wed, 13 Nov 2024 15:47:12 +0000 Subject: [PATCH 5/5] docs: Guidance on self-tests for cargo-mutants --- DESIGN.md | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index ec79b3f1..a48e7025 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -190,7 +190,16 @@ Therefore when running `rustc` we configure all warnings off, with `--cap-lints` ## Testing -Cargo-mutants is primarily tested on its public interface, which is the command line. These tests live in `tests/cli` and generally have the form of: +Testing cargo-mutants is of course important: both so that it works reliably and also so that it's a good example of a well-tested project that should have few missed mutants. + +### integration and unit tests + +The tests have a combination of: + +1. Rust "unit" tests run in-process, declared in `mod test{}` within various files in `src`. +2. Rust "integration" tests in the `tests` directory that actually run `cargo-mutants` as a subprocess and that test its overall behavior. + +The integration tests are in some sense more realistic because they drive the same interface as real users, but they will also typically be slower because they spawn subprocesses. As a result, my general idea is to have at least basic coverage of each feature with an integration test, and then to fill in the other cases with unit tests. 1. Make a copy of a `testdata` tree, so that it's not accidentally modified. 2. Run a `cargo-mutants` command on it. @@ -212,29 +221,25 @@ To manage test time: ### `testdata` trees -The primary means of testing is Rust source trees under `testdata`: you can copy an existing tree and modify it to show the new behavior that you want to test. +Many tests run against trees under `testdata`. -A selection of test trees are available for testing different scenarios. If there is an existing suitable tree, please use it. If you need to test a situation that is not covered yet, please add a new tree. +These have been "disarmed" by renaming `Cargo.toml` to `Cargo_test.toml`, so `cargo` won't normally run them, or see them as part of the main workspace. (See for context.) + +Tests should always run against a copy of these trees using `copy_of_testdata`, to make sure their work has no side effects on the main tree. Please describe the purpose of the testdata tree inside the tree, either in `Cargo.toml` or a `README.md` file. To make a new tree you can copy an existing tree, but make sure to change the package name in its `Cargo.toml`. -All the trees need to be excluded from the overall workspace in the top-level `Cargo.toml`. - ### `--list` tests There is a general test that runs `cargo mutants --list` on each tree and compares the result to an Insta snapshot, for both the text and json output formats. Many features can be tested adequately by only looking at the list of mutants produced, with no need to actually test the mutants. In this case the generic list tests might be enough. -### Unit tests - -Although we primarily want to test the public interface (which is the command line), unit tests can be added in a `mod test {}` within the source tree for any behavior that is inconvenient or overly slow to exercise from the command line. - ### Nextest tests -cargo-mutants tests require `nextest` to be installed. +cargo-mutants tests require `nextest` to be installed so that we can test the integration. ## UI Style