Rollup merge of #66503 - thomasetter:panic-error-msg, r=joshtriplett

More useful test error messages on should_panic(expected=...) mismatch

Fixes  #66304
r? @gilescope

Shows both the actual as well as the expected panic value when a test with `should_panic(expected=...)` fails.
This makes `should_panic` more consistent with `assert_eq`.

I am not sure whether printing the `Any::type_id()` is useful, is there something better that we could print for non-string panic values?

2 files changed, 59 insertions, 20 deletions

diff --git a/src/libtest/test_result.rs b/src/libtest/test_result.rs
index 80ca9dea18f..bfabe1722db 100644
--- a/src/libtest/test_result.rs
+++ b/src/libtest/test_result.rs
@@ -37,22 +37,30 @@ pub fn calc_result<'a>(
     let result = match (&desc.should_panic, task_result) {
         (&ShouldPanic::No, Ok(())) | (&ShouldPanic::Yes, Err(_)) => TestResult::TrOk,
         (&ShouldPanic::YesWithMessage(msg), Err(ref err)) => {
-            if err
+            let maybe_panic_str = err
                 .downcast_ref::<String>()
                 .map(|e| &**e)
-                .or_else(|| err.downcast_ref::<&'static str>().map(|e| *e))
-                .map(|e| e.contains(msg))
-                .unwrap_or(false)
-            {
+                .or_else(|| err.downcast_ref::<&'static str>().map(|e| *e));
+
+            if maybe_panic_str.map(|e| e.contains(msg)).unwrap_or(false) {
                 TestResult::TrOk
+            } else if desc.allow_fail {
+                TestResult::TrAllowedFail
+            } else if let Some(panic_str) = maybe_panic_str {
+                TestResult::TrFailedMsg(format!(
+                    r#"panic did not contain expected string
+      panic message: `{:?}`,
+ expected substring: `{:?}`"#,
+                    panic_str, msg
+                ))
             } else {
-                if desc.allow_fail {
-                    TestResult::TrAllowedFail
-                } else {
-                    TestResult::TrFailedMsg(
-                        format!("panic did not include expected string '{}'", msg)
-                    )
-                }
+                TestResult::TrFailedMsg(format!(
+                    r#"expected panic with string value,
+ found non-string value: `{:?}`
+     expected substring: `{:?}`"#,
+                    (**err).type_id(),
+                    msg
+                ))
             }
         }
         (&ShouldPanic::Yes, Ok(())) => {
diff --git a/src/libtest/tests.rs b/src/libtest/tests.rs
index 5f55b647f5e..0bea2b80ecf 100644
--- a/src/libtest/tests.rs
+++ b/src/libtest/tests.rs
@@ -15,6 +15,7 @@ use crate::{
         // TestType, TrFailedMsg, TrIgnored, TrOk,
     },
 };
+use std::any::TypeId;
 use std::sync::mpsc::channel;
 use std::time::Duration;
 
@@ -84,7 +85,7 @@ pub fn do_not_run_ignored_tests() {
     let (tx, rx) = channel();
     run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
     let result = rx.recv().unwrap().result;
-    assert!(result != TrOk);
+    assert_ne!(result, TrOk);
 }
 
 #[test]
@@ -103,7 +104,7 @@ pub fn ignored_tests_result_in_ignored() {
     let (tx, rx) = channel();
     run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
     let result = rx.recv().unwrap().result;
-    assert!(result == TrIgnored);
+    assert_eq!(result, TrIgnored);
 }
 
 // FIXME: Re-enable emscripten once it can catch panics again (introduced by #65251)
@@ -126,7 +127,7 @@ fn test_should_panic() {
     let (tx, rx) = channel();
     run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
     let result = rx.recv().unwrap().result;
-    assert!(result == TrOk);
+    assert_eq!(result, TrOk);
 }
 
 // FIXME: Re-enable emscripten once it can catch panics again (introduced by #65251)
@@ -149,7 +150,7 @@ fn test_should_panic_good_message() {
     let (tx, rx) = channel();
     run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
     let result = rx.recv().unwrap().result;
-    assert!(result == TrOk);
+    assert_eq!(result, TrOk);
 }
 
 // FIXME: Re-enable emscripten once it can catch panics again (introduced by #65251)
@@ -161,7 +162,9 @@ fn test_should_panic_bad_message() {
         panic!("an error message");
     }
     let expected = "foobar";
-    let failed_msg = "panic did not include expected string";
+    let failed_msg = r#"panic did not contain expected string
+      panic message: `"an error message"`,
+ expected substring: `"foobar"`"#;
     let desc = TestDescAndFn {
         desc: TestDesc {
             name: StaticTestName("whatever"),
@@ -175,7 +178,35 @@ fn test_should_panic_bad_message() {
     let (tx, rx) = channel();
     run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
     let result = rx.recv().unwrap().result;
-    assert!(result == TrFailedMsg(format!("{} '{}'", failed_msg, expected)));
+    assert_eq!(result, TrFailedMsg(failed_msg.to_string()));
+}
+
+// FIXME: Re-enable emscripten once it can catch panics again (introduced by #65251)
+#[test]
+#[cfg(not(target_os = "emscripten"))]
+fn test_should_panic_non_string_message_type() {
+    use crate::tests::TrFailedMsg;
+    fn f() {
+        panic!(1i32);
+    }
+    let expected = "foobar";
+    let failed_msg = format!(r#"expected panic with string value,
+ found non-string value: `{:?}`
+     expected substring: `"foobar"`"#, TypeId::of::<i32>());
+    let desc = TestDescAndFn {
+        desc: TestDesc {
+            name: StaticTestName("whatever"),
+            ignore: false,
+            should_panic: ShouldPanic::YesWithMessage(expected),
+            allow_fail: false,
+            test_type: TestType::Unknown,
+        },
+        testfn: DynTestFn(Box::new(f)),
+    };
+    let (tx, rx) = channel();
+    run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
+    let result = rx.recv().unwrap().result;
+    assert_eq!(result, TrFailedMsg(failed_msg));
 }
 
 // FIXME: Re-enable emscripten once it can catch panics again (introduced by #65251)
@@ -196,7 +227,7 @@ fn test_should_panic_but_succeeds() {
     let (tx, rx) = channel();
     run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
     let result = rx.recv().unwrap().result;
-    assert!(result == TrFailedMsg("test did not panic as expected".to_string()));
+    assert_eq!(result, TrFailedMsg("test did not panic as expected".to_string()));
 }
 
 fn report_time_test_template(report_time: bool) -> Option<TestExecTime> {
@@ -570,7 +601,7 @@ pub fn sort_tests() {
     ];
 
     for (a, b) in expected.iter().zip(filtered) {
-        assert!(*a == b.desc.name.to_string());
+        assert_eq!(*a, b.desc.name.to_string());
     }
 }