Add cli test framework #88
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
on my laptop, it takes about two minutes to run the test suite the first time, with a clean The test suite uses So, on my laptop, subsequent runs take about 30 seconds to run instead of 2 minutes. |
Contributor
Author
|
(CI test failure. Hmm. I wonder if something is wrong with how I am generated temporary directories?) |
bjorn3
reviewed
May 14, 2020
bjorn3
reviewed
May 14, 2020
bjorn3
reviewed
May 14, 2020
bjorn3
reviewed
May 14, 2020
bjorn3
reviewed
May 14, 2020
bjorn3
reviewed
May 14, 2020
Contributor
Author
|
Oh; I wonder if the problem is that |
Contributor
Author
|
Apparently I might need to use some relatively new functionality... |
Contributor
|
I am seeing the following test fail on macOS 10.15.3 as of f239318 (added a few escaped triple ticks so that it renders in a code block): |
tests/ice.rs
Outdated
|
|
||
| // The most basic check: does the output actually tell us about the | ||
| // "regressing" commit. | ||
| let needle = format!("regression in {}", INJECTION_COMMIT); |
Contributor
There was a problem hiding this comment.
Is it maybe that the "regression in ..." was capitalized so this doesn't match?
"regression" -> "Regression"?
Contributor
There was a problem hiding this comment.
Confirmed
Suggested change
| let needle = format!("regression in {}", INJECTION_COMMIT); | |
| let needle = format!("Regression in {}", INJECTION_COMMIT); |
tests/cli.rs
Outdated
|
|
||
| // The most basic check: does the output actually tell us about the | ||
| // "regressing" commit. | ||
| let needle = format!("regression in {}", test.expected_sha()); |
Contributor
There was a problem hiding this comment.
"regression" -> "Regression"
Contributor
There was a problem hiding this comment.
Confirmed:
Suggested change
| let needle = format!("regression in {}", test.expected_sha()); | |
| let needle = format!("Regression in {}", test.expected_sha()); |
Contributor
chrissimpkins
left a comment
chrissimpkins
left a comment
There was a problem hiding this comment.
The "regression" -> "Regression" change fixes the above issue.
Now there is a new one:
running 10 tests
test least_satisfying::tests::least_satisfying_5 ... ok
test least_satisfying::tests::least_satisfying_3 ... ok
test least_satisfying::tests::least_satisfying_4 ... ok
test least_satisfying::tests::least_satisfying_7 ... ok
test least_satisfying::tests::least_satisfying_2 ... ok
test least_satisfying::tests::least_satisfying_1 ... ok
test least_satisfying::tests::least_satisfying_6 ... ok
test least_satisfying::tests::least_satisfying_8 ... ok
test test_nightly_finder_iterator ... ok
test least_satisfying::tests::qc_prop ... ok
test result: ok. 10 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
Running target/debug/deps/cli-93cf2b2870ddfd32
running 1 test
test cli_test ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
Running target/debug/deps/ice-4f15fd6f69ba3152
running 1 test
test ice_test ... FAILED
failures:
---- ice_test stdout ----
running `cargo-bisect-rustc --preserve --regress=ice --access=github --start 2020-02-20 --end 2020-02-22` in "/tmp/eventually_ice"
Command output stdout for eventually_ice:
std for x86_64-apple-darwin: 16.44 MB / 16.44 MB [=========] 100.00 % 3.33 MB/s
Command output stderr for eventually_ice:
installing nightly-2020-02-20
testing...
RESULT: nightly-2020-02-20, ===> Yes
ERROR: the start of the range (nightly-2020-02-20) must not reproduce the regression
thread 'ice_test' panicked at 'assertion failed: stderr.contains(&needle)', tests/ice.rs:82:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
ice_test
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--test ice'
Contributor
|
I haven't had a chance to fully dig in to your changes but this is fantastic to see btw! :) |
Contributor
|
I added cross-platform GH Actions CI workflows in #90 |
Contributor
|
#90 merged and available for testing with this PR |
pnkfelix
force-pushed
the
add-cli-test-frameworkl
branch
from
f239318 to
23e70ad
Compare
Contributor
Author
|
Sigh: |
Contributor
Author
Seems likely related to this: rust-lang/rustfmt#3794 I will work-around it locally. |
Note: generates tests into fresh temp dir, which is deleted after testing is
done (regardless of success or failure). You can change the
`which_temp::WhichTempDir` type to revise this behavior.
This infrastructure includes two tests: `tests/cli.rs` and `tests/ice.rs`. Each
uses very different strategies for testing cargo-bisect-rustc.
1. `tests/cli.rs` uses a so-called meta-build strategy: the test inspects the
`rustc` version, then generates build files that will inject (or remove,
e.g. when testing `--regress=success`) `#[rustc_error]` from the source
code based on the `rustc` version. This way, we get the effect of an error
that will come or go based solely on the `rustc` version, without any
dependence on the actual behavior of `rustc` itself (beyond its version
string format remaining parsable).
* This strategy should remain usable for the foreseeable future, without
any need for intervention from `cargo-bisect-rustc` developers.
2. `tests/ice.rs` uses a totally different strategy: It embeds an ICE that we
know originated at a certain version of the compiler. The ICE is embedded
in the file `src/ice/included_main.rs`. The injection point associated with
the ICE is encoded in the constant `INJECTION_COMMIT`.
* Over time, since we only keep a certain number of builds associated with
PR merge commits available to download, the embedded ICE, the
`INJECTION_COMMIT` definition, and the search bounds defined in
`INJECTION_LOWER_BOUND` and `INJECTION_UPPER_BOUND` will all have to be
updated as soon as the commit for `INJECTION_COMMIT` is no longer
available for download.
* Thus, this testing strategy requires regular maintenance from the
`cargo-bisect-rustc` developers. (However, it is more flexible than the
meta-build strategy, in that you can embed arbitrary failures from the
recent past using this approach. The meta-build approach can only embed
things that can be expressed via features like `#[rustc_error]`, which
cannot currently express ICE's.
----
Includes suggestions from code review
Co-authored-by: bjorn3 <bjorn3@users.noreply.github.com>
----
Includes some coments explaining the `WhichTempDir` type. (That type maybe
should just be an enum rather than a trait you implement... not sure why I made
it so general...)
----
Includes workaround for rustfmt issue.
Specifically, workaround rust-lang/rustfmt#3794 which
was causing CI's attempt to run `cargo fmt -- --check` to erroneously report:
```
% cargo fmt -- --check
error[E0583]: file not found for module `meta_build`
--> /private/tmp/cbr/tests/cli.rs:11:20
|
11 | pub(crate) mod meta_build;
| ^^^^^^^^^^
|
= help: name the file either meta_build.rs or meta_build/mod.rs inside the directory "/private/tmp/cbr/tests/cli/cli"
error[E0583]: file not found for module `command_invocation`
--> /private/tmp/cbr/tests/ice.rs:34:20
|
34 | pub(crate) mod command_invocation;
| ^^^^^^^^^^^^^^^^^^
|
= help: name the file either command_invocation.rs or command_invocation/mod.rs inside the directory "/private/tmp/cbr/tests/ice/common"
```
----
Includes fix for oversight in my cli test system: it needed to lookup target
binary, not our PATH.
(This functionality is also available via other means, such as
`$CARGO_BIN_EXE_<name>` and https://crates.io/crates/assert_cmd. I opted not to
use the builtin env variable because that is only available in very recent cargo
versions, and I would prefer our test suite to work peven on older versions of
cargo, if that is feasible...)
----
Includes applications of rustfmt suggestions, as well as an expansion of a
comment in a manner compatible with rustfmt.
(Namely, that replaced an inline comment which is erroneously deleted by rustfmt
(see rust-lang/rustfmt#2781 ) with an additional note
in the comment above the definition.)
pnkfelix
force-pushed
the
add-cli-test-frameworkl
branch
from
23e70ad to
da1ee9d
Compare
Contributor
Author
|
Hmm what does this output imply... |
Contributor
Looks reqwest-y :) It should be possible to reproduce this with |
Contributor
Author
I tried on my mac but it does not reproduce there. I'll be getting a dedicated Linux (well maybe Windows too) desktop system in the near future; I'll give it a shot there and see if it reproduces in that context. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Infrastructure for testing the command line tool.
Note: generates tests into fresh temp dir, which is deleted after testing is done (regardless of success or failure). You can change the
which_temp::WhichTempDirtype definition to revise this behavior (it implements a trait and there are two implementations; one which generates into a fresh temp dir, and one that generates into/tmp)This infrastructure includes two tests:
tests/cli.rsandtests/ice.rs. Eachuses very different strategies for testing cargo-bisect-rustc.
tests/cli.rsuses a so-called meta-build strategy: the test inspects therustcversion, then generates build files that will inject (or remove, e.g. when testing--regress=success)#[rustc_error]from the source code based on therustcversion. This way, we get the effect of an error that will come or go based solely on therustcversion, without any dependence on the actual behavior ofrustcitself (beyond its version string format remaining parsable).cargo-bisect-rustcdevelopers.tests/ice.rsuses a totally different strategy: It embeds an ICE that we know originated at a certain version of the compiler. The ICE is embedded in the filesrc/ice/included_main.rs. The injection point associated with the ICE is encoded in the constantINJECTION_COMMIT.Over time, since we only keep a certain number of builds associated with PR merge commits available to download, the embedded ICE, the
INJECTION_COMMITdefinition, and the search bounds defined inINJECTION_LOWER_BOUNDandINJECTION_UPPER_BOUNDwill all have to be updated as soon as the commit forINJECTION_COMMITis no longer available for download.Thus, this testing strategy requires regular maintenance from the
cargo-bisect-rustcdevelopers. (However, it is more flexible than the meta-build strategy, in that you can embed arbitrary failures from the recent past using this approach. The meta-build approach can only embed things that can be expressed via features like#[rustc_error], which cannot currently express ICE's.