From be993beb0b0294c6ce4519bf007f73063187809e Mon Sep 17 00:00:00 2001 From: Thomas Etter Date: Mon, 18 Nov 2019 00:51:18 +0100 Subject: [PATCH 1/3] print a more useful error message on should_panic mismatch --- src/libtest/test_result.rs | 22 +++++++++++++++++----- src/libtest/tests.rs | 35 +++++++++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/src/libtest/test_result.rs b/src/libtest/test_result.rs index 80ca9dea18f5a..5dbbd71554e98 100644 --- a/src/libtest/test_result.rs +++ b/src/libtest/test_result.rs @@ -37,10 +37,12 @@ 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::() .map(|e| &**e) - .or_else(|| err.downcast_ref::<&'static str>().map(|e| *e)) + .or_else(|| err.downcast_ref::<&'static str>().map(|e| *e)); + + if maybe_panic_str .map(|e| e.contains(msg)) .unwrap_or(false) { @@ -49,9 +51,19 @@ pub fn calc_result<'a>( if desc.allow_fail { TestResult::TrAllowedFail } else { - TestResult::TrFailedMsg( - format!("panic did not include expected string '{}'", msg) - ) + 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 { + TestResult::TrFailedMsg( + format!(r#"expected panic with string value, + found non-string value: `{:?}` + expected substring: `{:?}`"#, (**err).type_id(), &*msg) + ) + } } } } diff --git a/src/libtest/tests.rs b/src/libtest/tests.rs index 5f55b647f5e78..fc82bb4f47a4b 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; @@ -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::()); + 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) From 48a86e0b2cdfaba5a2a684911178d717067e4d28 Mon Sep 17 00:00:00 2001 From: Thomas Etter Date: Mon, 18 Nov 2019 00:52:10 +0100 Subject: [PATCH 2/3] replace some asserts with assert_eq for better error readability --- src/libtest/tests.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libtest/tests.rs b/src/libtest/tests.rs index fc82bb4f47a4b..0bea2b80ecf5e 100644 --- a/src/libtest/tests.rs +++ b/src/libtest/tests.rs @@ -85,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] @@ -104,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) @@ -127,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) @@ -150,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) @@ -227,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 { @@ -601,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()); } } From 16bf4f5e1b50773c6b4ec7b7524876440db69d1b Mon Sep 17 00:00:00 2001 From: Thomas Etter Date: Tue, 19 Nov 2019 21:35:07 +0100 Subject: [PATCH 3/3] Simplify if else as suggested in PR feedback --- src/libtest/test_result.rs | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/src/libtest/test_result.rs b/src/libtest/test_result.rs index 5dbbd71554e98..bfabe1722dbed 100644 --- a/src/libtest/test_result.rs +++ b/src/libtest/test_result.rs @@ -42,29 +42,25 @@ pub fn calc_result<'a>( .map(|e| &**e) .or_else(|| err.downcast_ref::<&'static str>().map(|e| *e)); - if maybe_panic_str - .map(|e| e.contains(msg)) - .unwrap_or(false) - { + 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 + } 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 { - TestResult::TrFailedMsg( - format!(r#"expected panic with string value, + expected substring: `{:?}`"#, + panic_str, msg + )) + } else { + TestResult::TrFailedMsg(format!( + r#"expected panic with string value, found non-string value: `{:?}` - expected substring: `{:?}`"#, (**err).type_id(), &*msg) - ) - } - } + expected substring: `{:?}`"#, + (**err).type_id(), + msg + )) } } (&ShouldPanic::Yes, Ok(())) => {