Improvements to the #[should_panic] feature

[keeperofdakeys]

Add more error checking for the #[should_panic] attribute, and print the expected panic string when it does not match.

Fixes #29000

Eg:

test test2 ... ok
test test1 ... FAILED
: Panic did not include expected string 'foo'
test test3 ... FAILED

failures:

---- test1 stdout ----
	thread 'test1' panicked at 'bar', test.rs:7
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- test3 stdout ----
	thread 'test3' panicked at 'bar', test.rs:18

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[GuillaumeGomez]

I restarted the test which failed (for some reasons...).

[est31]

You should also add tests to check

• when the panic message doesn't match, the test fails
• when the panic message matches, the test succeeds

[keeperofdakeys]

You should also add tests to check

when the panic message doesn't match, the test fails
when the panic message matches, the test succeeds

Aren't the tests that are already present covering this?
https://github.com/rust-lang/rust/blob/master/src/libtest/lib.rs#L1526-L1597

[est31]

@keeperofdakeys yes, but this only tests part of the code. The parts that you added aren't tested (extracting the string from the expected attribute). It would be nicer to test all of it.

[keeperofdakeys]

I've updated the test_should_panic_bad_message test so that it also checks the failed message.

[nrc]

Hmm, I suppose this is a back-compat breaking change because people who used the bad forms previously will get an error when there was none previously. Annoyingly, Crater will not pick up such errors. I think therefore that this needs to be a warning, rather than an error for one cycle.

[nrc]

LGTM, otherwise

[keeperofdakeys]

Since it seems you can't insert lints in libsyntax, I've changed the errors into warnings, and added notes about future changes.

[est31]

@keeperofdakeys that's still not fullfilling the complete potential of the test, It still doesn't test whether the thing you specify in expected is actually the string that's being used for the panic message, and not something else.

[keeperofdakeys]

@est31 I don't understand the exact point you're trying to make, could you elaborate?

[brson]

This looks good, except that I'm not sure about the new output:

test test1 ... FAILED
: Panic did not include expected string 'foo'

I don't think there's any other such inline output about what happened in the test for any other sort of test, and I'm reluctant to start the precedent here. Usually this type of information is printed to stderr, captured by the test runner, and printed as part of the final summary list of failures, and I'd prefer if this information could be presented similarly.

[est31]

@keeperofdakeys the unit tests you linked to use internal testing functions, and don't use the #[should_panic] attribute. If there is a bug in parsing of that attribute (the code you add in this PR), they won't detect it.

[keeperofdakeys]

I've updated the output to:

running 3 tests
test test1 ... FAILED
test test2 ... FAILED
test test3 ... ok

failures:

---- test1 stdout ----
    thread 'test1' panicked at 'explicit panic', ./test.rs:6
note: Run with `RUST_BACKTRACE=1` for a backtrace.
note: Panic did not include expected string 'foo'
---- test2 stdout ----
    thread 'test2' panicked at 'bar', ./test.rs:12
note: Panic did not include expected string 'foo'

failures:
    test1
    test2

test result: FAILED. 1 passed; 2 failed; 0 ignored; 0 measured

@est31 Ah, thanks for clarifying. Since writing a unit test for fn should_panic in libsyntax/test.rs seems non-trivial, I'm thinking that a test in src/test/run-fail with a panic! that doesn't match the expected value would be the best.

[nrc]

@bors: r+

[bors]

📌 Commit 72ee3e6 has been approved by nrc

[bors]

⌛ Testing commit 72ee3e6 with merge 0a1b0f4...

[bors]

💔 Test failed - auto-linux-64-opt

[keeperofdakeys]

Whoops, looks like I accidentally made a heisentest. I've split it out into two files now.

[keeperofdakeys]

Strange, travis-ci failed before compilation.

[alexcrichton]

@bors: r+

[bors]

📌 Commit fb5ccf8 has been approved by alexcrichton

[bors]

⌛ Testing commit fb5ccf8 with merge 195c42c...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: alexcrichton
Pushing 195c42c to master...