Better error messages for #[should_panic] tests with expected strings

[thomasetter]

When running this test (playground):

#[test]
#[should_panic(expected = "foo\r\n")]
fn panic_test() {
    panic!("foo\n");
}

the output is

running 1 test
test panic_test ... FAILED

failures:

---- panic_test stdout ----
thread 'panic_test' panicked at 'foo
', src/lib.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
note: panic did not include expected string 'foo

'

failures:
    panic_test

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

which makes it hard to spot the difference in whitespace characters.

Would it be possible to change this to e.g.:

running 1 test
test panic_test ... FAILED

failures:

---- panic_test stdout ----
thread 'panic_test' panicked at 'foo
', src/lib.rs:4:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
note: panic did not include expected string 'foo

'
   Panic message: "foo\n"
 Expected string: "foo\r\n"

failures:
    panic_test

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

This would improve the usability when running into these kinds of test failures and also make it more consistent with assert_eq which outputs both the left and the right value too.

[gilescope]

Agree with having them vertically aligned. I think we do want line endings to create a new line though for multi-line readability.

Maybe we should say something like this:

First difference at char 15: expected: '\r' found: '\n'

   Panic message: "foo
"
Expected message: "foo
"

(The first difference at char line we might also want to add to assert_eq also?)

[thomasetter]

The First difference at char message sounds good, but I am not sure how well it applies to the should_panic(expected=...) comparison, as it is not an equality, but a substring comparison.
Maybe we could adjust the wording of the error message to e.g.:

 Expected substring:

to make that easier to spot.

I think the escaping is essential, take for instance this test which has no newlines, but contains shell control characters to move the cursor:

#[test]
#[should_panic(expected = "foo\x1b[1Dbar")]
fn shell_escape_panic_test() {
    panic!("fobar");
}

On many shells, this will produce this output:

running 1 test
test shell_escape_panic_test ... FAILED

failures:

---- shell_escape_panic_test stdout ----
thread 'shell_escape_panic_test' panicked at 'fobar', src/lib.rs:4:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
note: panic did not include expected string 'fobar'

failures:
    shell_escape_panic_test

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

which makes it very hard for a user to figure out what's going on (let's assume the panic happens in a more obscure location).

Maybe a more important reason is that escaping would be consistent with the behavior of assert_eq, which for this test:

#[test]
fn assert_eq_shell_escape_test() {
    assert_eq!("foo\x1b[1Dbar", "fobar");
}

outputs:

running 1 test
test assert_eq_shell_escape_test ... FAILED

failures:

---- assert_eq_shell_escape_test stdout ----
thread 'assert_eq_shell_escape_test' panicked at 'assertion failed: `(left == right)`
  left: `"foo\u{1b}[1Dbar"`,
 right: `"fobar"`', src/lib.rs:3:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.


failures:
    assert_eq_shell_escape_test

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

The escaping avoids surprises which could occur depending on the user`s environment.

[gilescope]

Ok I'm sold. Do you want to fork rust-lang/rust and submit this as a PR? (If you don't want to I can do the legwork).

[thomasetter]

Ok I'm sold. Do you want to fork rust-lang/rust and submit this as a PR? (If you don't want to I can do the legwork).

Thanks for the offer, I'll try to write a PR and will add you as a reviewer once I have a first version ready.