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

feat: Add a --profile option to choose a cargo profile #398

Merged
merged 9 commits into from
Aug 20, 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
1 change: 1 addition & 0 deletions .cargo/mutants.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@

error_values = ["::anyhow::anyhow!(\"mutated!\")"]
exclude_globs = ["src/console.rs"]
profile = "mutants" # Build without debug symbols
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ indoc = "2.0.0"
itertools = "0.12"
jobserver = "0.1"
mutants = "0.0.3"
num_cpus ="1.16"
num_cpus = "1.16"
patch = "0.7"
path-slash = "0.2"
quote = "1.0.35"
Expand Down
4 changes: 4 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

- Fixed: Avoid generating empty string elements in `ENCODED_RUSTFLAGS` when `--cap-lints` is set. In some situations these could cause a compiler error complaining about the empty argument.

- New: `--profile` option allows selecting a Cargo profile. In particular, it's recommended that you can use `--profile=mutants` and configure a custom profile in your `Cargo.toml` to optimize the build for mutants, by turning off debug symbols.

- New: `--iterate` option skips mutants that were previously caught or unviable.

- New: cargo-mutants starts a GNU jobserver, shared across all children, so that running multiple `--jobs` does not spawn an excessive number of compiler processes. The jobserver is on by default and can be turned off with `--jobserver false`.
Expand Down
28 changes: 24 additions & 4 deletions book/src/performance.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,33 @@ test binary. If you're using doctests only as testable documentation and not to
assert correctness of the code, you can skip them with `cargo mutants --
--all-targets`.

## Optimized builds
## Choosing a cargo profile

On _some but not all_ projects, cargo-mutants can be faster if you use `-C --release`, which will make the build slower but may make the tests faster. Typically this will help on projects with very long CPU-intensive test suites.
[Cargo profiles](https://doc.rust-lang.org/cargo/reference/profiles.html) provide a way to configure compiler settings including several that influence build and runtime performance.

cargo-mutants now shows the breakdown of build versus test time which may help you work out if this will help: if the tests are much slower than the build it's worth trying more more compiler optimizations.
By default, cargo-mutants will use the default profile selected for `cargo test`, which is also called `test`. This includes debug symbols but disables optimization.

You can select a different profile using the `--profile` option or the `profile` configuration key.

You may wish to define a `mutants` profile in `Cargo.toml`, such as:

```toml
[profile.mutants]
inherits = "test"
debug = "none"
```

and then configure this as the default in `.cargo/mutants.toml`:

On projects like this you might also choose just to turn up optimization for all debug builds in [`.cargo/config.toml`](https://doc.rust-lang.org/cargo/reference/config.html).
```toml
profile = "mutants"
```

Turning off debug symbols will make the builds faster, at the expense of possibly giving less useful output when a test fails. In general, since mutants are expected to cause tests to fail, debug symbols may not be worth cost.

If your project's tests take a long time to run then it may be worth experimenting with increasing the `opt` level or other optimization parameters in the profile, to trade off longer builds for faster test runs.

cargo-mutants now shows the breakdown of build versus test time which may help you work out if this will help: if the tests are much slower than the build it's worth trying more more compiler optimizations.

## Ramdisks

Expand Down
141 changes: 108 additions & 33 deletions src/cargo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

//! Run Cargo as a subprocess, including timeouts and propagating signals.

use std::iter::once;
use std::time::{Duration, Instant};

use itertools::Itertools;
Expand All @@ -27,14 +28,17 @@ pub fn run_cargo(
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(options)),
let mut env = vec![
// The tests might use Insta <https://insta.rs>, 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()),
("INSTA_FORCE_PASS".to_owned(), "0".to_owned()),
];
if let Some(encoded_rustflags) = encoded_rustflags(options) {
debug!(?encoded_rustflags);
env.push(("CARGO_ENCODED_RUSTFLAGS".to_owned(), encoded_rustflags));
}
let process_status = Process::run(
&argv,
&env,
Expand Down Expand Up @@ -111,6 +115,16 @@ fn cargo_argv(
cargo_args.push("--tests".to_string());
}
}
if let Some(profile) = &options.profile {
match options.test_tool {
TestTool::Cargo => {
cargo_args.push(format!("--profile={profile}"));
}
TestTool::Nextest => {
cargo_args.push(format!("--cargo-profile={profile}"));
}
}
}
cargo_args.push("--verbose".to_string());
if let Some([package]) = packages {
// Use the unambiguous form for this case; it works better when the same
Expand Down Expand Up @@ -151,34 +165,33 @@ fn cargo_argv(
///
/// See <https://doc.rust-lang.org/cargo/reference/environment-variables.html>
/// <https://doc.rust-lang.org/rustc/lints/levels.html#capping-lints>
fn rustflags(options: &Options) -> String {
let mut rustflags: Vec<String> = if let Some(rustflags) = env::var_os("CARGO_ENCODED_RUSTFLAGS")
{
rustflags
.to_str()
.expect("CARGO_ENCODED_RUSTFLAGS is not valid UTF-8")
.split('\x1f')
.map(|s| s.to_owned())
.collect()
} else if let Some(rustflags) = env::var_os("RUSTFLAGS") {
rustflags
.to_str()
.expect("RUSTFLAGS is not valid UTF-8")
.split(' ')
.map(|s| s.to_owned())
.collect()
fn encoded_rustflags(options: &Options) -> Option<String> {
let cap_lints_arg = "--cap-lints=warn";
let separator = "\x1f";
if !options.cap_lints {
None
} else if let Ok(encoded) = env::var("CARGO_ENCODED_RUSTFLAGS") {
if encoded.is_empty() {
Some(cap_lints_arg.to_owned())
} else {
Some(encoded + separator + cap_lints_arg)
}
} else if let Ok(rustflags) = env::var("RUSTFLAGS") {
if rustflags.is_empty() {
Some(cap_lints_arg.to_owned())
} else {
Some(
rustflags
.split(' ')
.filter(|s| !s.is_empty())
.chain(once("--cap-lints=warn"))
.collect::<Vec<&str>>()
.join(separator),
)
}
} else {
// TODO: We could read the config files, but working out the right target and config seems complicated
// given the information available here.
// TODO: All matching target.<triple>.rustflags and target.<cfg>.rustflags config entries joined together.
// TODO: build.rustflags config value.
Vec::new()
};
if options.cap_lints {
rustflags.push("--cap-lints=warn".to_owned());
Some(cap_lints_arg.to_owned())
}
// debug!("adjusted rustflags: {:?}", rustflags);
rustflags.join("\x1f")
}

#[cfg(test)]
Expand Down Expand Up @@ -352,16 +365,78 @@ mod test {
);
}

#[test]
fn profile_arg_passed_to_cargo() {
let args = Args::try_parse_from(["mutants", "--profile", "mutants"].as_slice()).unwrap();
let options = Options::from_args(&args).unwrap();
let build_dir = Utf8Path::new("/tmp/buildXYZ");
assert_eq!(
cargo_argv(build_dir, None, Phase::Check, &options)[1..],
[
"check",
"--tests",
"--profile=mutants",
"--verbose",
"--workspace",
]
);
}

#[test]
fn nextest_gets_special_cargo_profile_option() {
let args = Args::try_parse_from(
["mutants", "--test-tool=nextest", "--profile", "mutants"].as_slice(),
)
.unwrap();
let options = Options::from_args(&args).unwrap();
let build_dir = Utf8Path::new("/tmp/buildXYZ");
assert_eq!(
cargo_argv(build_dir, None, Phase::Build, &options)[1..],
[
"nextest",
"run",
"--no-run",
"--cargo-profile=mutants",
"--verbose",
"--workspace",
]
);
}

rusty_fork_test! {
#[test]
fn rustflags_with_no_environment_variables() {
fn rustflags_without_cap_lints_and_no_environment_variables() {
env::remove_var("RUSTFLAGS");
env::remove_var("CARGO_ENCODED_RUSTFLAGS");
assert_eq!(
encoded_rustflags(&Options {
..Default::default()
}),
None
);
}
#[test]
fn rustflags_with_cap_lints_and_no_environment_variables() {
env::remove_var("RUSTFLAGS");
env::remove_var("CARGO_ENCODED_RUSTFLAGS");
assert_eq!(
rustflags(&Options {
encoded_rustflags(&Options {
cap_lints: true,
..Default::default()
}),
Some("--cap-lints=warn".into())
);
}

// Don't generate an empty argument if the encoded rustflags is empty.
#[test]
fn rustflags_with_empty_encoded_rustflags() {
env::set_var("CARGO_ENCODED_RUSTFLAGS", "");
assert_eq!(
encoded_rustflags(&Options {
cap_lints: true,
..Default::default()
}).unwrap(),
"--cap-lints=warn"
);
}
Expand All @@ -374,17 +449,17 @@ mod test {
cap_lints: true,
..Default::default()
};
assert_eq!(rustflags(&options), "--something\x1f--else\x1f--cap-lints=warn");
assert_eq!(encoded_rustflags(&options).unwrap(), "--something\x1f--else\x1f--cap-lints=warn");
}

#[test]
fn rustflags_added_to_existing_rustflags() {
env::set_var("RUSTFLAGS", "-Dwarnings");
env::remove_var("CARGO_ENCODED_RUSTFLAGS");
assert_eq!(rustflags(&Options {
assert_eq!(encoded_rustflags(&Options {
cap_lints: true,
..Default::default()
}), "-Dwarnings\x1f--cap-lints=warn");
}).unwrap(), "-Dwarnings\x1f--cap-lints=warn");
}
}
}
2 changes: 2 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ pub struct Config {
pub additional_cargo_test_args: Vec<String>,
/// Minimum test timeout, in seconds, as a floor on the autoset value.
pub minimum_test_timeout: Option<f64>,
/// Cargo profile.
pub profile: Option<String>,
/// Choice of test tool: cargo or nextest.
pub test_tool: Option<TestTool>,
/// Timeout multiplier, relative to the baseline 'cargo test'.
Expand Down
4 changes: 4 additions & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,10 @@ pub struct Args {
#[arg(long, help_heading = "Execution")]
no_shuffle: bool,

/// Build with this cargo profile.
#[arg(long, help_heading = "Build")]
profile: Option<String>,

/// Run only one shard of all generated mutants: specify as e.g. 1/4.
#[arg(long, help_heading = "Execution")]
shard: Option<Shard>,
Expand Down
35 changes: 31 additions & 4 deletions src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ pub struct Options {
/// interesting results.
pub shuffle: bool,

/// Cargo profile.
pub profile: Option<String>,

/// Additional arguments for every cargo invocation.
pub additional_cargo_args: Vec<String>,

Expand Down Expand Up @@ -205,6 +208,10 @@ impl Options {
&config.additional_cargo_test_args,
),
baseline: args.baseline,
build_timeout: args.build_timeout.map(Duration::from_secs_f64),
build_timeout_multiplier: args
.build_timeout_multiplier
.or(config.build_timeout_multiplier),
cap_lints: args.cap_lints.unwrap_or(config.cap_lints),
check_only: args.check,
colors: args.colors,
Expand All @@ -228,16 +235,13 @@ impl Options {
output_in_dir: args.output.clone(),
print_caught: args.caught,
print_unviable: args.unviable,
profile: args.profile.as_ref().or(config.profile.as_ref()).cloned(),
shuffle: !args.no_shuffle,
show_line_col: args.line_col,
show_times: !args.no_times,
show_all_logs: args.all_logs,
test_timeout: args.timeout.map(Duration::from_secs_f64),
test_timeout_multiplier: args.timeout_multiplier.or(config.timeout_multiplier),
build_timeout: args.build_timeout.map(Duration::from_secs_f64),
build_timeout_multiplier: args
.build_timeout_multiplier
.or(config.build_timeout_multiplier),
test_tool: args.test_tool.or(config.test_tool).unwrap_or_default(),
};
options.error_values.iter().for_each(|e| {
Expand Down Expand Up @@ -520,5 +524,28 @@ mod test {
let options = Options::new(&args, &Config::default()).unwrap();
assert_eq!(options.colors.forced_value(), None);
}

#[test]
fn profile_option_from_args() {
let args = Args::parse_from(["mutants", "--profile=mutants"]);
let options = Options::new(&args, &Config::default()).unwrap();
assert_eq!(options.profile.unwrap(), "mutants");
}


#[test]
fn profile_from_config() {
let args = Args::parse_from(["mutants", "-j3"]);
let config = indoc! { r#"
profile = "mutants"
timeout_multiplier = 1.0
build_timeout_multiplier = 2.0
"#};
let mut config_file = NamedTempFile::new().unwrap();
config_file.write_all(config.as_bytes()).unwrap();
let config = Config::read_file(config_file.path()).unwrap();
let options = Options::new(&args, &config).unwrap();
assert_eq!(options.profile.unwrap(), "mutants");
}
}
}
10 changes: 7 additions & 3 deletions tests/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ fn mutants_causing_tests_to_hang_are_stopped_by_manual_timeout() {
#[test]
fn hang_avoided_by_build_timeout_with_cap_lints() {
let tmp_src_dir = copy_of_testdata("hang_when_mutated");
run()
let out = run()
.arg("mutants")
.args([
"--build-timeout-multiplier=4",
Expand All @@ -795,8 +795,12 @@ fn hang_avoided_by_build_timeout_with_cap_lints() {
.current_dir(tmp_src_dir.path())
.env_remove("RUST_BACKTRACE")
.timeout(OUTER_TIMEOUT)
.assert()
.code(3); // exit_code::TIMEOUT
.assert();
println!(
"debug log:\n===\n{}\n===",
read_to_string(tmp_src_dir.path().join("mutants.out/debug.log")).unwrap_or_default()
);
out.code(3); // exit_code::TIMEOUT
let timeout_txt = read_to_string(tmp_src_dir.path().join("mutants.out/timeout.txt"))
.expect("read timeout.txt");
assert!(
Expand Down
Loading