Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --baseline=skip #247

Merged
merged 9 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ env:
CARGO_MUTANTS_MINIMUM_TEST_TIMEOUT: 60

jobs:
build:
test:
strategy:
matrix:
os: [macOS-latest, ubuntu-latest, windows-latest]
Expand Down Expand Up @@ -121,11 +121,11 @@ jobs:

cargo-mutants:
runs-on: ubuntu-latest
needs: [build, pr-mutants, release-binary]
needs: [test, pr-mutants, release-binary]
strategy:
matrix:
shard: [0, 1, 2, 3, 4, 5, 6, 7]
test_tool: [cargo, nextest]
test_tool: [cargo]
steps:
- uses: actions/checkout@v3
- uses: dtolnay/rust-toolchain@master
Expand All @@ -144,9 +144,10 @@ jobs:
run: |
install cargo-mutants $HOME/.cargo/bin/
- name: Mutants
# Skip baselines because this action depends on the tests
run: >
cargo mutants --no-shuffle -vV --shard ${{ matrix.shard }}/8
--test-tool ${{ matrix.test_tool }}
--test-tool ${{ matrix.test_tool }} --baseline=skip
- name: Archive mutants.out
uses: actions/upload-artifact@v3
if: always()
Expand Down
3 changes: 2 additions & 1 deletion .markdownlint.jsonc
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
"default": true,
// Allow lines that are all italics or bold; they're not meant to be headings
"MD036": false,
"MD013": false // Allow long lines
"MD013": false, // Allow long lines
"MD033": false // Allow inline HTML, needed for <div class="warning"> etc
}
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

- New: Mutate `+, -, *, /, %, &, ^, |, <<, >>` binary ops, and their corresponding assignment ops like `+=`.

- New: `--baseline=skip` option to skip running tests in an unmutated tree, when they're already been checked externally.

- Changed: Stop generating mutations of `||` and `&&` to `!=` and `||`, because it seems to raise too many low-value false positives that may be hard to test.

## 24.1.0
Expand Down
1 change: 1 addition & 0 deletions book/src/SUMMARY.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
- [Passing options to Cargo](cargo-args.md)
- [Build directories](build-dirs.md)
- [Using nextest](nextest.md)
- [Baseline tests](baseline.md)
- [Generating mutants](mutants.md)
- [Error values](error-values.md)
- [Improving performance](performance.md)
Expand Down
29 changes: 29 additions & 0 deletions book/src/baseline.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Baseline tests

Normally, cargo-mutants builds and runs your tree in a temporary directory before applying any mutations. This makes sure that your tests are in fact all passing, including in the copy of the tree that cargo-mutants will mutate.

Baseline tests can be skipped by passing the `--baseline=skip` command line option. (There is no config option for this.)

<div class="warning">
If you use <code>--baseline=skip</code>, you must make sure that the tests are actually passing, otherwise the results of cargo-mutants will be meaningless. cargo-mutants will probably report that all or most mutations were caught, but the test failures were not because of the mutations.
</div>

## Performance effects

The performance gain from skipping the baseline is one run of the full test suite, plus one incremental build. When the baseline is run, its build is typically slow because it must do the initial build of the tree, but when it is skipped, the first mutant will have to do a full (rather than incremental) build instead.

This means that, in a run that tests many mutants, the relative performance gain from skipping the baseline will be relatively small. However, it may still be useful to skip baseline tests in some specific situations.

## Timeouts

Normally, cargo-mutants uses the baseline test to establish an appropriate `timeout` for the test suite. If you skip the baseline, you should set `--timeout` manually.

## Use cases for skipping baseline tests

`--baseline=skip` might be useful in these situations:

1. You are running cargo-mutants in a CI or build system that separately runs the tests before cargo-mutants. In this case, you can be confident that the tests are passing, and you can save time by skipping the baseline. In particular, if you are [sharding](shards.md) work in CI, this avoids running the baseline on each shard.

2. You're repeatedly running `cargo-mutants` with different options, without changing the source code, perhaps with different `--file` or `--exclude` options.

3. You're developing `cargo-mutants` itself, and running it repeatedly on a tree that doesn't change.
2 changes: 1 addition & 1 deletion book/src/how-it-works.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ The basic approach is:
- Before applying any mutations, check that `cargo test` succeeds in the
scratch directory: perhaps a test is already broken, or perhaps the tree
doesn't build when copied because it relies on relative paths to find
dependencies, etc.
dependencies, etc. This is called the "baseline" test.
- If running more than one parallel job, make the appropriate number of
additional scratch directories.

Expand Down
2 changes: 1 addition & 1 deletion book/src/limitations.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ cargo-mutants contains two main categories of code, which are mostly independent

2. Code for finding the modules to mutate and their source files, finding the tree to copy, adjusting paths after it is copied, and finally running builds and tests. This is very Cargo-specific, but should not be too hard to generalize.

The main precondition for supporting Bazel is a realistc test case: preferably an open source Rust tree built with Bazel, or at least a contributor with a Bazel-based Rust tree who is willing to help test and debug and to produce some test cases.
The main precondition for supporting Bazel is a realistic test case: preferably an open source Rust tree built with Bazel, or at least a contributor with a Bazel-based Rust tree who is willing to help test and debug and to produce some test cases.

(See <https://github.com/sourcefrog/cargo-mutants/issues/77> for more discussion.)

Expand Down
2 changes: 1 addition & 1 deletion book/src/shards.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ Then, for each mutant in its shard, it does an incremental build and runs all th

Each shard runs the same number of mutants, +/-1. Typically this will mean they each take roughly the same amount of time, although it's possible that some shards are unlucky in drawing mutants that happen to take longer to test.

A rough model for the overall execution time for all of the shards, allowing for this work occuring in parallel, is
A rough model for the overall execution time for all of the shards, allowing for this work occurring in parallel, is

```raw
SHARD_STARTUP + (CLEAN_BUILD + TEST) + (N_MUTANTS/K) * (INCREMENTAL_BUILD + TEST)
Expand Down
2 changes: 1 addition & 1 deletion book/src/timeouts.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ run the test suite in the unmodified tree, and then sets a timeout for mutated
tests at 5x the time to run tests with no mutations, and a minimum of 20
seconds.

The minimum of 20 seconds can be overriden by the
The minimum of 20 seconds can be overridden by the
`CARGO_MUTANTS_MINIMUM_TEST_TIMEOUT` environment variable, measured in seconds.

You can also set an explicit timeout with the `--timeout` option, also measured
Expand Down
97 changes: 56 additions & 41 deletions src/lab.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,48 +55,34 @@ pub fn test_mutants(

let output_mutex = Mutex::new(output_dir);
let mut build_dirs = vec![BuildDir::new(workspace_dir, &options, console)?];
let baseline_outcome = {
let _span = debug_span!("baseline").entered();
test_scenario(
&mut build_dirs[0],
&output_mutex,
&Scenario::Baseline,
&all_packages,
options.test_timeout.unwrap_or(Duration::MAX),
&options,
console,
)?
};
if !baseline_outcome.success() {
error!(
"cargo {} failed in an unmutated tree, so no mutants were tested",
baseline_outcome.last_phase(),
);
return Ok(output_mutex
.into_inner()
.expect("lock output_dir")
.take_lab_outcome());
}

let mutated_test_timeout = if let Some(timeout) = options.test_timeout {
timeout
} else if let Some(baseline_test_duration) = baseline_outcome
.phase_results()
.iter()
.find(|r| r.phase == Phase::Test)
.map(|r| r.duration)
{
let auto_timeout = max(
options.minimum_test_timeout,
baseline_test_duration.mul_f32(5.0),
);
if options.show_times {
console.autoset_timeout(auto_timeout);
let baseline_outcome = match options.baseline {
BaselineStrategy::Run => {
let outcome = test_scenario(
&mut build_dirs[0],
&output_mutex,
&Scenario::Baseline,
&all_packages,
options.test_timeout.unwrap_or(Duration::MAX),
&options,
console,
)?;
if !outcome.success() {
error!(
"cargo {} failed in an unmutated tree, so no mutants were tested",
outcome.last_phase(),
);
// We "successfully" established that the baseline tree doesn't work; arguably this should be represented as an error
// but we'd need a way for that error to convey an exit code...
return Ok(output_mutex
.into_inner()
.expect("lock output_dir")
.take_lab_outcome());
}
Some(outcome)
}
auto_timeout
} else {
Duration::MAX
BaselineStrategy::Skip => None,
};
let test_timeout = test_timeout(&baseline_outcome, &options, console);

let jobs = max(1, min(options.jobs.unwrap_or(1), mutants.len()));
console.build_dirs_start(jobs - 1);
Expand Down Expand Up @@ -131,7 +117,7 @@ pub fn test_mutants(
&output_mutex,
&Scenario::Mutant(mutant),
&[&package],
mutated_test_timeout,
test_timeout,
&options,
console,
)
Expand Down Expand Up @@ -163,6 +149,35 @@ pub fn test_mutants(
Ok(lab_outcome)
}

fn test_timeout(
baseline_outcome: &Option<ScenarioOutcome>,
options: &Options,
console: &Console,
) -> Duration {
if let Some(timeout) = options.test_timeout {
timeout
} else if options.check_only {
Duration::ZERO
} else if options.baseline == BaselineStrategy::Skip {
warn!("An explicit timeout is recommended when using --baseline=skip; using 300 seconds by default");
Duration::from_secs(300)
} else {
let auto_timeout = max(
options.minimum_test_timeout,
baseline_outcome
.as_ref()
.expect("Baseline tests should have run")
.total_phase_duration(Phase::Test)
.mul_f32(5.0),
);
if options.show_times {
// TODO: Just `info!` not a special method.
console.autoset_timeout(auto_timeout);
}
auto_timeout
}
}

/// Test various phases of one scenario in a build dir.
///
/// The [BuildDir] is passed as mutable because it's for the exclusive use of this function for the
Expand Down
16 changes: 15 additions & 1 deletion src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use std::process::exit;

use anyhow::{anyhow, ensure, Context, Result};
use camino::{Utf8Path, Utf8PathBuf};
use clap::{ArgAction, CommandFactory, Parser};
use clap::{ArgAction, CommandFactory, Parser, ValueEnum};
use clap_complete::{generate, Shell};
use tracing::debug;

Expand Down Expand Up @@ -70,6 +70,16 @@ enum Cargo {
Mutants(Args),
}

#[derive(Debug, Default, ValueEnum, Clone, Copy, Eq, PartialEq)]
pub enum BaselineStrategy {
/// Run tests in an unmutated tree before testing mutants.
#[default]
Run,

/// Don't run tests in an unmutated tree: assume that they pass.
Skip,
}

/// Find inadequately-tested code that can be removed without any tests failing.
///
/// See <https://github.com/sourcefrog/cargo-mutants> for more information.
Expand All @@ -80,6 +90,10 @@ struct Args {
#[arg(long)]
all_logs: bool,

/// baseline strategy: check that tests pass in an unmutated tree before testing mutants.
#[arg(long, value_enum, default_value_t = BaselineStrategy::Run)]
baseline: BaselineStrategy,

/// print mutants that were caught by tests.
#[arg(long, short = 'v')]
caught: bool,
Expand Down
19 changes: 19 additions & 0 deletions src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ use crate::*;
/// config file.
#[derive(Default, Debug, Clone)]
pub struct Options {
/// Run tests in an unmutated tree?
pub baseline: BaselineStrategy,

/// Don't run the tests, just see if each mutant builds.
pub check_only: bool,

Expand Down Expand Up @@ -138,6 +141,7 @@ impl Options {
&args.cargo_test_args,
&config.additional_cargo_test_args,
),
baseline: args.baseline,
check_only: args.check,
colors: true, // TODO: An option for this and use CLICOLORS.
emit_json: args.json,
Expand Down Expand Up @@ -228,6 +232,21 @@ mod test {
assert_eq!(options.test_tool, TestTool::Nextest);
}

#[test]
fn options_from_baseline_arg() {
let args = Args::parse_from(["mutants", "--baseline", "skip"]);
let options = Options::new(&args, &Config::default()).unwrap();
assert_eq!(options.baseline, BaselineStrategy::Skip);

let args = Args::parse_from(["mutants", "--baseline", "run"]);
let options = Options::new(&args, &Config::default()).unwrap();
assert_eq!(options.baseline, BaselineStrategy::Run);

let args = Args::parse_from(["mutants"]);
let options = Options::new(&args, &Config::default()).unwrap();
assert_eq!(options.baseline, BaselineStrategy::Run);
}

#[test]
fn test_tool_from_config() {
let config = indoc! { r#"
Expand Down
11 changes: 11 additions & 0 deletions src/outcome.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,17 @@ impl ScenarioOutcome {
&self.phase_results
}

/// Return the total time spent in commands for one phase.
///
/// If the phase was not run, returns zero.
pub fn total_phase_duration(&self, phase: Phase) -> Duration {
self.phase_results
.iter()
.filter(|pr| pr.phase == phase)
.map(|pr| pr.duration)
.sum()
}

/// True if this status indicates the user definitely needs to see the logs, because a task
/// failed that should not have failed.
pub fn should_show_logs(&self) -> bool {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,11 +232,12 @@ src/in_diff.rs: replace += with *= in partial_new_file
src/interrupt.rs: replace install_handler with ()
src/lab.rs: replace test_mutants -> Result<LabOutcome> with Ok(Default::default())
src/lab.rs: replace test_mutants -> Result<LabOutcome> with Err(::anyhow::anyhow!("mutated!"))
src/lab.rs: replace == with != in test_mutants
src/lab.rs: replace - with + in test_mutants
src/lab.rs: replace - with / in test_mutants
src/lab.rs: replace == with != in test_mutants
src/lab.rs: replace == with != in test_mutants
src/lab.rs: replace test_timeout -> Duration with Default::default()
src/lab.rs: replace == with != in test_timeout
src/lab.rs: replace test_scenario -> Result<ScenarioOutcome> with Ok(Default::default())
src/lab.rs: replace test_scenario -> Result<ScenarioOutcome> with Err(::anyhow::anyhow!("mutated!"))
src/lab.rs: replace || with && in test_scenario
Expand Down Expand Up @@ -381,6 +382,8 @@ src/outcome.rs: replace ScenarioOutcome::last_phase -> Phase with Default::defau
src/outcome.rs: replace ScenarioOutcome::last_phase_result -> ProcessStatus with Default::default()
src/outcome.rs: replace ScenarioOutcome::phase_results -> &[PhaseResult] with Vec::leak(Vec::new())
src/outcome.rs: replace ScenarioOutcome::phase_results -> &[PhaseResult] with Vec::leak(vec![Default::default()])
src/outcome.rs: replace ScenarioOutcome::total_phase_duration -> Duration with Default::default()
src/outcome.rs: replace == with != in ScenarioOutcome::total_phase_duration
src/outcome.rs: replace ScenarioOutcome::should_show_logs -> bool with true
src/outcome.rs: replace ScenarioOutcome::should_show_logs -> bool with false
src/outcome.rs: replace && with || in ScenarioOutcome::should_show_logs
Expand Down
26 changes: 26 additions & 0 deletions tests/cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,32 @@ fn small_well_tested_tree_is_clean() {
assert!(log_content.contains("factorial(6) = 0"));
}

#[test]
fn test_small_well_tested_tree_with_baseline_skip() {
let tmp_src_dir = copy_of_testdata("small_well_tested");
run()
.arg("mutants")
.args(["--no-times", "--no-shuffle", "-v", "-V", "--baseline=skip"])
.arg("-d")
.arg(tmp_src_dir.path())
.assert()
.success()
.stdout(predicate::function(|stdout| {
insta::assert_snapshot!(stdout);
true
}))
.stderr(
predicate::str::contains(
"An explicit timeout is recommended when using --baseline=skip",
)
.and(predicate::str::contains("Unmutated baseline in").not()),
);
assert!(!tmp_src_dir
.path()
.join("mutants.out/log/baseline.log")
.exists());
}

#[test]
fn cdylib_tree_is_well_tested() {
let tmp_src_dir = copy_of_testdata("cdylib");
Expand Down
Loading