Add option to run all tests

[Emerentius]

This adds the "--include-ignored" flag to libtest, which allows running ignored and unignored tests in one go.

Closes#50363

[rust-highfive]

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

[Emerentius]

I don't know if "--all" is such a good name when we also have flags for "--tests" and "--benches", but I'm pulling blank on alternatives. I'm thinking it should be ok because the most common usage would be cargo test -- --all.

[Mark-Simulacrum]

A few minor nits, but otherwise this looks good.

[Mark-Simulacrum]

I wonder if it makes sense to error here if we've been passed both; what do you think?

[Emerentius]

Hmm, maybe. It's often better for humans but worse for automated tools. It's also the more conservative approach that we can always relax later so I'm going for it.

[Mark-Simulacrum]

assert_eq and needless inner parentheses

[Mark-Simulacrum]

No need for this panic, we can probably just unwrap?

[Emerentius]

Yeah. Also, I think this is an error message for a buggy test, not a buggy parse_opts.

[Mark-Simulacrum]

@rust-lang/libs - as an insta-stable change asking for FCP (I don't think I can start it).

Cc @rust-lang/dev-tools, not sure if this is under your purview...

[SimonSapin]

libtest is semi-private / permanently unstable (except possibly for Bencher), so I don’t think there is a library discussion to be had here.

[Manishearth]

@djrenren on devtools is working on custom test frameworks and probably should have a say on this

[djrenren]

This looks really helpful. Two nits:

• I'd recommend --force that way this could be used with test, bench, or filtered subset. It also conveys that we'd be overriding something.

• Since this is a change to the public-facing interface (which I believe we consider stable?) shouldn't we run this through nightly?

[Emerentius]

I associate --force quite strongly, though not exclusively, with operations that are dangerous, inadvisable or rarely needed like rm -f or git {push | pull | clean | checkout } --force

shouldn't we run this through nightly?

What does that mean in this context?

[djrenren]

Presumably ignored tests are ignored for a reason. You're explicitly going against the intentions of the author so to me it seems worth of a --force, but maybe I don't have the correct mental model for ignored tests.

What does that mean in this context?

Ah @Mark-Simulacrum mentioned making this an insta-stable change rather than starting as nightly-only then stabilizing later.

[Emerentius]

If the author didn't want it to be run ever it would be deleted, commented out or not marked #[test]. There is of course no prescribed reason for which #[ignore] may be used, but realistic cases would be for example very expensive computations or things that access external resources (say, access certain files that may not be setup). In fact, expensive computations is what I've used them for. I've done a quick search through the rust repo. There aren't many occasions, but I've found that the book uses expensive tests as an example and there are two such tests for float printing. A test in cargo's test suite is ignored because it does a web request.
I'm thinking of #[ignore] more like an "off by default"

There's an unstable options flag that supposedly only allows the nightly compiler to use certain flags, but it seems like it really just enforces passing -Zunstable-options prior to using the unstable option. --format json is currently the only unstable option.

[SimonSapin]

It isn’t great that the subtly different commands cargo test --all and cargo test -- --all would do very different things. I’d prefer another name, but I don’t really have a suggestion.

[Emerentius]

I was thinking something like --ignored (include | skip | only) [default: skip] would be nice, but that's not backwards compatible. Maybe --run-all ?
It may not be obvious with that name that this can still be combined with filters.

[nrc]

Seems like a nice thing to have from my perspective. I agree --all is not great. Maybe --test-ignored or something similarly explicit?

fixed

[Emerentius]

--include-ignored ?
Should be quite clear, but it's long and imo too close to --ignored

[Emerentius]

What's the procedure for moving forward? I can't think think of any better flag names than the ones I've proposed so far.
Will there be an FCP to give others a chance to give input?

[Mark-Simulacrum]

I'm going to reassign to r? @nrc since I think the dev tools team is where the decision needs to come from at this point

[bors]

⌛ Testing commit f6f3228 with merge e6ec19172a3119598e81f83d9f9a962f78c92c06...

[bors]

💔 Test failed - status-appveyor

[kennytm]

This has broken a dependency compiletest_rs which clippy's test relies on

[00:55:31] error[E0308]: mismatched types
[00:55:31] --> C:\Users\appveyor\.cargo\registry\src\github.com-1ecc6299db9ec823\compiletest_rs-0.3.13\src\lib.rs:100:22
[00:55:31] |
[00:55:31] 100 | run_ignored: config.run_ignored,
[00:55:31] | ^^^^^^^^^^^^^^^^^^ expected enum `test::RunIgnored`, found bool
[00:55:31] |
[00:55:31] = note: expected type `test::RunIgnored`
[00:55:31] found type `bool`
[00:55:31] [00:55:32] error: aborting due to previous error

[Emerentius]

@kennytm So that's an external crate that calls into rustc internals? I'm a bit confused on what I should do now.
Get a patch ready for compiletest_rs so this can be changed in sync? Some intermediate breakage seems necessary.

[Mark-Simulacrum]

You'll probably want to just retry the build in a few days at which point this should land since we don't require clippy to pass normally, just before and during release week. Once this lands compiletest on crates.io can be updated and finally clippy update can land (or just Cargo.lock, perhaps).

[kennytm]

@bors retry

[bors]

⌛ Testing commit f6f3228 with merge 9f53c87...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 9f53c87 to master...

[rust-highfive]

📣 Toolstate changed by #53527!

Tested on commit 9f53c87.
Direct link to PR: #53527

💔 clippy-driver on windows: test-pass → test-fail (cc @Manishearth@llogiq@mcarton@oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → test-fail (cc @Manishearth@llogiq@mcarton@oli-obk, @rust-lang/infra).

[eddyb]

In the interest of saving time, and breaking as little as possible, I'll add a From<bool> impl for this new enum in #54116, and update compiletest-rs to use From::from.

EDIT: oh we don't own compiletest-rs... I'll revert this PR instead. It really shouldn't have landed so nearly to cutting the RC. Apologies for the inconvenience.

[eddyb]

So I think we should try to do this without breaking compiletest_rs. I'm looking into it right now.
EDIT: okay, so it seems that the usual thing is to just break it and release a patch version - e.g. Manishearth/compiletest-rs#99 - that means we should wait for beta (Rust 2018 RC1) to branch off and then merge this PR again.

[Emerentius]

@eddyb Have the waters calmed enough that this can be merged again? Should I open a new PR?

[eddyb]

@Emerentius Yes, sorry to delay this and keep you waiting! RC1 went out about a week ago.