Skip to content

Add default regression terms based on --regress option. #339

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

Merged
merged 1 commit into from
Jun 17, 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
6 changes: 4 additions & 2 deletions guide/src/examples/diagnostics.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,14 @@ fi
echo "$OUTPUT"
# This indicates a regression when the text "non-ASCII" is in the output.
#
# If the regression is when the text is *not* in the output, remove the `!` prefix.
# If the regression is when the text is *not* in the output, remove the `!` prefix
# (and customize the `--term-old` and `--term-new` CLI options if you want).
! echo "$OUTPUT" | grep "non-ASCII"
```

Then run something like:

```sh
cargo bisect-rustc --start=1.67.0 --end=1.68.0 --script ./test.sh
cargo bisect-rustc --start=1.67.0 --end=1.68.0 --script ./test.sh \
--term-old="No warning" --term-new="Found non-ASCII warning"
```
3 changes: 2 additions & 1 deletion guide/src/examples/doc-change.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ fi
And run with:

```sh
cargo bisect-rustc --start 1.68.0 --end 1.69.0 -c rust-docs --script ./test.sh
cargo bisect-rustc --start 1.68.0 --end 1.69.0 -c rust-docs --script ./test.sh \
--term-old="Did not find" --term-new="Found"
```

> **Note**: This may not work on all targets since `cargo-bisect-rustc` doesn't properly handle rustup manifests, which alias some targets to other targets.
Expand Down
3 changes: 2 additions & 1 deletion guide/src/examples/rustdoc.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ grep "some example text" $CARGO_TARGET_DIR/doc/mycrate/fn.foo.html
This can be used with the `--script` option:

```sh
cargo-bisect-rustc --start=2023-01-22 --end=2023-03-18 --script=./test.sh
cargo-bisect-rustc --start=2023-01-22 --end=2023-03-18 --script=./test.sh \
--term-old="Found example text" --term-new="Failed, or did not find text"
```
18 changes: 4 additions & 14 deletions guide/src/tutorial.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ cargo bisect-rustc --script=./test.sh \

## Custom bisection messages

[Available from v0.6.9]
*Available from v0.6.9*

You can add custom messages when bisecting a regression. Taking inspiration from git-bisect, with `term-new` and `term-old` you can set custom messages to indicate if a regression matches the condition set by the bisection.

Expand All @@ -151,18 +151,8 @@ cargo bisect-rustc \
--term-new "Yes, this build reproduces the regression, compile error"
```

Note that `--term-{old,new}` are aware of the `--regress` parameter. If the bisection is looking for a build to reproduce a regression (i.e. `--regress {error,ice}`), `--term-old` indicates a point in time where the regression does not reproduce and `--term-new` indicates that it does.
In other words, `--term-old` is displayed for older compilers that **do not** exhibit the regression. `--term-new` is for newer compilers which do exhibit the regression.

On the other hand, if `--regress {non-error,non-ice,success}` you are looking into bisecting when a condition of error stopped being reproducible (e.g. some faulty code does not produce an error anymore). In this case `cargo-bisect` flips the meaning of these two parameters.
What counts as a "regression" is defined by the [`--regress`](usage.html#regression-check) CLI option. By default, a regression is a compile-error (which is equivalent to `--term-new`). If you flip the definition of a "regression" with `--regress=success`, then a regression is a successful compile (which is *also* equivalent to `--term-new`).

Example:
```sh
cargo bisect-rustc \
--start=2018-08-14 \
--end=2018-10-11 \
--regress=success \
--term-old "This build does not compile" \
--term-new "This build compiles"
```

See [`--regress`](usage.html#regression-check) for more details.
There are default terms based on the current `--regress` setting. Customizing the terms is most useful when using [scripting](#testing-with-a-script). For example, in the [Documentation changes](examples/doc-change.md) example, the customized terms can more clearly express the results of the script of whether or not it found what it was looking for in the documentation.
26 changes: 4 additions & 22 deletions src/least_satisfying.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
use std::collections::BTreeMap;
use std::fmt;

use crate::RegressOn;

pub fn least_satisfying<T, P>(slice: &[T], mut predicate: P) -> usize
where
T: fmt::Display + fmt::Debug,
Expand Down Expand Up @@ -175,27 +173,11 @@ pub enum Satisfies {
}

impl Satisfies {
pub fn msg_with_context(&self, regress: &RegressOn, term_old: &str, term_new: &str) -> String {
let msg_yes = if regress == &RegressOn::Error || regress == &RegressOn::Ice {
// YES: compiles, does not reproduce the regression
term_new
} else {
// NO: compile error, reproduces the regression
term_old
};

let msg_no = if regress == &RegressOn::Error || regress == &RegressOn::Ice {
// YES: compile error
term_old
} else {
// NO: compiles
term_new
};

pub fn msg_with_context<'a>(&self, term_old: &'a str, term_new: &'a str) -> &'a str {
match self {
Self::Yes => msg_yes.to_string(),
Self::No => msg_no.to_string(),
Self::Unknown => "Unable to figure out if the condition matched".to_string(),
Self::Yes => term_new,
Self::No => term_old,
Self::Unknown => "Unable to figure out if the condition matched",
}
}
}
Expand Down
48 changes: 41 additions & 7 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,15 +183,13 @@ a date (YYYY-MM-DD), git tag name (e.g. 1.58.0) or git commit SHA."

#[arg(
long,
default_value = "Test condition NOT matched",
help = "Text shown when a test fails to match the condition requested"
help = "Text shown when a test does match the condition requested"
)]
term_new: Option<String>,

#[arg(
long,
default_value = "Test condition matched",
help = "Text shown when a test does match the condition requested"
help = "Text shown when a test fails to match the condition requested"
)]
term_old: Option<String>,
}
Expand Down Expand Up @@ -880,8 +878,44 @@ impl Config {
dl_spec: &DownloadParams,
) -> Result<Satisfies, InstallError> {
let regress = self.args.regress;
let term_old = self.args.term_old.clone().unwrap_or_default();
let term_new = self.args.term_new.clone().unwrap_or_default();
let term_old = self.args.term_old.as_deref().unwrap_or_else(|| {
if self.args.script.is_some() {
match regress {
RegressOn::Error => "Script returned success",
RegressOn::Success => "Script returned error",
RegressOn::Ice => "Script did not ICE",
RegressOn::NonIce => "Script found ICE",
RegressOn::NonError => "Script returned error (no ICE)",
}
} else {
match regress {
RegressOn::Error => "Successfully compiled",
RegressOn::Success => "Compile error",
RegressOn::Ice => "Did not ICE",
RegressOn::NonIce => "Found ICE",
RegressOn::NonError => "Compile error (no ICE)",
}
}
});
let term_new = self.args.term_new.as_deref().unwrap_or_else(|| {
if self.args.script.is_some() {
match regress {
RegressOn::Error => "Script returned error",
RegressOn::Success => "Script returned success",
RegressOn::Ice => "Script found ICE",
RegressOn::NonIce => "Script did not ICE",
RegressOn::NonError => "Script returned success or ICE",
}
} else {
match regress {
RegressOn::Error => "Compile error",
RegressOn::Success => "Successfully compiled",
RegressOn::Ice => "Found ICE",
RegressOn::NonIce => "Did not ICE",
RegressOn::NonError => "Successfully compiled or ICE",
}
}
});
match t.install(&self.client, dl_spec) {
Ok(()) => {
let outcome = t.test(self);
Expand All @@ -893,7 +927,7 @@ impl Config {
eprintln!(
"RESULT: {}, ===> {}",
t,
r.msg_with_context(&regress, &term_old, &term_new)
r.msg_with_context(term_old, term_new)
);
remove_toolchain(self, t, dl_spec);
eprintln!();
Expand Down
6 changes: 2 additions & 4 deletions tests/cmd/h.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,8 @@ Options:
-t, --timeout <TIMEOUT> Assume failure after specified number of seconds (for bisecting
hangs)
--target <TARGET> Cross-compilation target platform
--term-new <TERM_NEW> Text shown when a test fails to match the condition requested
[default: "Test condition NOT matched"]
--term-old <TERM_OLD> Text shown when a test does match the condition requested [default:
"Test condition matched"]
--term-new <TERM_NEW> Text shown when a test does match the condition requested
--term-old <TERM_OLD> Text shown when a test fails to match the condition requested
--test-dir <TEST_DIR> Root directory for tests [default: .]
-v, --verbose...
-V, --version Print version
Expand Down
8 changes: 2 additions & 6 deletions tests/cmd/help.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,10 @@ Options:
Cross-compilation target platform

--term-new <TERM_NEW>
Text shown when a test fails to match the condition requested

[default: "Test condition NOT matched"]
Text shown when a test does match the condition requested

--term-old <TERM_OLD>
Text shown when a test does match the condition requested

[default: "Test condition matched"]
Text shown when a test fails to match the condition requested

--test-dir <TEST_DIR>
Root directory for tests
Expand Down