Add unstable option to ignore should_panic tests

[memoryruins]

Add an unstable option --exclude-should-panic to libtest to workaround rust-lang/miri#636

?r @oli-obk
cc @RalfJung

[rust-highfive]

r? @sfackler

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

[memoryruins]

r? @oli-obk

[RalfJung]

To just satisfy Miri's needs, all we need to do is use the existing ShouldPanic enum, and add a #[cfg(miri)] block that ignores test marked as such. Why all the complexity?

[memoryruins]

It was the first experiment tried, but I agree about the complexity, which is why I posted an alternative in #58689 (comment)

It would only use the ShouldPanic enum and wouldn’t need a miri specific cfg.

[RalfJung]

I've seen that, but that still adds a new option exclude_should_panic. That seems overkill if it's just for Miri, and it seems unclear to me how cargo miri test would set that option. What I would have done is

#[cfg(miri)]
{ // Miri does not support panics, ignore should_panic tests
filtered.retain(|test| test.desc.should_panic == ShouldPanic::No);
}

If, OTOH, you think that option is also useful outside of Miri, then this seems fine.

[memoryruins]

I understand your reservations, as it seems like the option would be niche outside of this. One possible advantage for miri is that since the option does not require further changes to libtest, it could be useful for testing when the time comes to support panics and unwinding.

The flag idea was proposed by oli-obk, so I rolled with it (unless if I misunderstood).

I think a less miri-specific variant would be to add a flag to ignore should_panic tests. Then we can just enable that by default. But this has to happen inside the test harness, too.

rust-lang/miri#636 (comment)

The new flag can be passed to cargo similar to include-ignored

cargo test -- --Z unstable-options --exclude-should-panic

or in the case of cargo miri,

cargo miri test -- -- --Z unstable-options --exclude-should-panic

which could be passed by default by cargo miri test if desired.

[RalfJung]

which could be passed by default by cargo miri test if desired.

I suppose that works. I'll leave it up to you to decide whether it makes sense to carry that extra flag just for Miri, or whether it has more general use.

I think we'll have no trouble testing the panic/unwinding story without this, when the time comes for Miri to support that.

[memoryruins]

I think we'll have no trouble testing the panic/unwinding story without this

That’s good to hear. Thank you for the review, Ralf :)

I'll leave it up to you to decide whether it makes sense to carry that extra flag just for Miri

I would like to first hear oli’s thoughts on the current changes. It’s possible I’ve made more than they expected too.

[oli-obk]

I did indeed have the current changes in mind, but I agree with @RalfJung, if there's a simpler option, let's use that.

That seems overkill if it's just for Miri, and it seems unclear to me how cargo miri test would set that option. What I would have done is

that won't work, libtest is never compiled with cfg(miri), right? If it is, then by all means, just use a cfg to not run any should_panic tests.

[RalfJung]

that won't work, libtest is never compiled with cfg(miri), right?

It is. cfg(miri) is set when cargo miri setup builds libstd+libtest.

However, I think it is NOT set inside rustbuild when we run the Miri test suite there...

[memoryruins]

Ah right, using a cli flag would cover the case of miri users that are using a specific nightly with cargo miri test as described here without ever running cargo miri setup. ^^

[RalfJung]

Running cargo miri test implicitly automatically runs cargo miri setup.

[oli-obk]

Also, without full MIR, you can't run tests anyway ;) So you do need cargo miri setup.

However, I think it is NOT set inside rustbuild when we run the Miri test suite there...

we already have the test-miri flag in config.toml, so we can just make bootstrap set cfg(miri) if test-miri is on.

[RalfJung]

we already have the test-miri flag in config.toml, so we can just make bootstrap set cfg(miri) if test-miri is on.

We could, but do we want to? This means that with that flag set, all should_panic tests will get skipped, even when not run in Miri. That seems wrong.

[oli-obk]

hm. true. Ok, so... we roll with this PR and undo it once miri can do unwinding?

[memoryruins]

I looked into it more and saw that compiletest and the extracted compiletest-rs crate depend on libtest's TestOpts struct, which would mean that adding this flag could require fixing toolstate afterwards (and then doing so again when removing).

Before going further, I would like to try out ralf's suggestion locally and see if it sorts the original issue.
If it sorts it, I would like to open another PR using #[cfg(miri)] in libtest.

(unless if the cli flag is preferred after all for the rustbuild/bootstrap case)

[RalfJung]

Basically this would be a hack to make Miri somewhat more useful, but we'd have to be careful that we don't ever set cfg(miri) in a situation where the code might also be run "for real". I am not sure if the libs team would accept such hacks in the compiler repo.

Having a --ignore-should-panic makes somewhat more sense as there are "real" (non-Miri) platforms where abort=panic is the only possible implementation. @japaric have you ever wanted to run a test suite on (an emulator for) an embedded device, and had trouble because of panic=abort and should_panic tests?

So I think I have come around to the conclusion that such a flag actually might make some sense. But then ignored tests should be treated just like those marked #[ignore], i.e., they should be printed as "Test <name>... ignored".

compiletest and the extracted compiletest-rs crate depend on libtest's TestOpts struct, which would mean that adding this flag could require fixing toolstate afterwards (and then doing so again when removing).

AFAIK the extracted crate carries a copy of this, so this should be fine from a toolstate perspective.

[japaric]

have you ever wanted to run a test suite on (an emulator for) an embedded device, and had trouble because of panic=abort and should_panic tests?

All the embedded targets that are panic=abort and I'm familiar with are no_std targets and we can't run standard unit tests (#[test]) for those because test depends on std (cargo test -> "error: std not found for target").

[RalfJung]

@rust-lang/infra recently discussed issues with panics on Android and disabling should_panic tests. Maybe they'd be interested in an option like this?

[emilyalbini]

It would be huge for the infra team if this is merged!

[oli-obk]

@bors r+

This is the design I envisioned, seems to be the least surprising or least error prone solution we came up with. Additionally being useful to more than just miri makes the slight additional complexity acceptable

[bors]

📌 Commit 43e7434 has been approved by oli-obk

[emilyalbini]

@bors p=1

Needed to fix a spurious failure.

[kennytm]

Seemed to cause failure in #58820 on WASM.

[01:19:52] �[0m�[0m�[1m�[32m Running�[0m build/x86_64-unknown-linux-gnu/stage2-std/wasm32-unknown-unknown/release/deps/coretests-a4433084340ef73b.wasm
[01:19:53] RuntimeError: unreachable
[01:19:53] at __rust_start_panic (wasm-function[1095]:45)
[01:19:53] at rust_panic (wasm-function[1082]:39)
[01:19:53] at _ZN3std9panicking20rust_panic_with_hook17hd0681d90b51f9001E (wasm-function[1077]:346)
[01:19:53] at _ZN3std9panicking18continue_panic_fmt17heb2c3399be9c1647E (wasm-function[1076]:151)
[01:19:53] at _ZN3std9panicking15begin_panic_fmt17heae8ec5c6936ff86E (wasm-function[955]:108)
[01:19:53] at _ZN4core3ops8function6FnOnce9call_once17h8654601badcab08eE (wasm-function[418]:500)
[01:19:53] at _ZN4test28__rust_begin_short_backtrace17h5acac12ba406c08cE (wasm-function[753]:3)
[01:19:53] at _ZN50_$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$8call_box17h3970079b2a2e8325E (wasm-function[752]:6)
[01:19:53] at _ZN3std9panicking3try7do_call17h2a54e10a8ef5b9a4E (wasm-function[679]:14)
[01:19:53] at __rust_maybe_catch_panic (wasm-function[1094]:5)
[01:19:53] at _ZN4test8run_test14run_test_inner17ha9d0c3c920b17a93E (wasm-function[833]:801)
[01:19:53] at _ZN4test8run_test17h9570bc596a31e164E (wasm-function[830]:2119)
[01:19:53] at _ZN4test9run_tests17hac4301576bb3c3acE (wasm-function[825]:3408)
[01:19:53] at _ZN4test17run_tests_console17hddadae4b0dffc12fE (wasm-function[819]:993)
[01:19:53] at _ZN4test9test_main17h343b975aae7c6b90E (wasm-function[816]:433)
[01:19:53] at _ZN4test16test_main_static17hfd242c8df31b861aE (wasm-function[820]:1746)
[01:19:53] at _ZN9coretests4main17h59dcd2e523abb771E (wasm-function[642]:10)
[01:19:53] at _ZN3std2rt10lang_start28_$u7b$$u7b$closure$u7d$$u7d$17h409059acbb92528aE (wasm-function[10]:25)
[01:19:53] at _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17hcfd224ff90044727E (wasm-function[1060]:8)
[01:19:53] at _ZN3std9panicking3try7do_call17h526f0906eecc5923E (wasm-function[1073]:20)
[01:19:53] �[0m�[0m�[1m�[31merror:�[0m test failed, to rerun pass '--test coretests'
[01:19:53] [01:19:53] [01:19:53] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "wasm32-unknown-unknown" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--"
[01:19:53] expected success, got: exit code: 101

[bors]

⌛ Testing commit 43e7434 with merge 7b4f8f9...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 7b4f8f9 to master...

[rust-highfive]

📣 Toolstate changed by #58689!

Tested on commit 7b4f8f9.
Direct link to PR: #58689

💔 miri on windows: test-pass → test-fail (cc @oli-obk@RalfJung@eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → test-fail (cc @oli-obk@RalfJung@eddyb, @rust-lang/infra).

[RalfJung]

Seems like I was wrong thinking that compiletest-rs carries copies of these structs. Oh well.