From 44d1d86124e7229a59a13899a14e468fee9dc923 Mon Sep 17 00:00:00 2001
From: Zalathar <Zalathar@users.noreply.github.com>
Date: Tue, 8 Apr 2025 13:26:22 +1000
Subject: [PATCH] libtest: Pass the test's panic payload as Option instead of
 Result

Passing a `Result<(), &dyn Any>` to `calc_result` requires awkward code at both
call sites, for no real benefit. It's much easier to just pass the payload as
`Option<&dyn Any>`.

No functional change, except that the owned payload is dropped slightly later.
---
 library/test/src/lib.rs         | 14 ++++++--------
 library/test/src/test_result.rs | 21 ++++++++++++++-------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/library/test/src/lib.rs b/library/test/src/lib.rs
index 7ada3f269a002..acaf026c679bf 100644
--- a/library/test/src/lib.rs
+++ b/library/test/src/lib.rs
@@ -666,10 +666,11 @@ fn run_test_in_process(
 
     io::set_output_capture(None);
 
-    let test_result = match result {
-        Ok(()) => calc_result(&desc, Ok(()), time_opts.as_ref(), exec_time.as_ref()),
-        Err(e) => calc_result(&desc, Err(e.as_ref()), time_opts.as_ref(), exec_time.as_ref()),
-    };
+    // Determine whether the test passed or failed, by comparing its panic
+    // payload (if any) with its `ShouldPanic` value, and by checking for
+    // fatal timeout.
+    let test_result =
+        calc_result(&desc, result.err().as_deref(), time_opts.as_ref(), exec_time.as_ref());
     let stdout = data.lock().unwrap_or_else(|e| e.into_inner()).to_vec();
     let message = CompletedTest::new(id, desc, test_result, exec_time, stdout);
     monitor_ch.send(message).unwrap();
@@ -741,10 +742,7 @@ fn spawn_test_subprocess(
 fn run_test_in_spawned_subprocess(desc: TestDesc, runnable_test: RunnableTest) -> ! {
     let builtin_panic_hook = panic::take_hook();
     let record_result = Arc::new(move |panic_info: Option<&'_ PanicHookInfo<'_>>| {
-        let test_result = match panic_info {
-            Some(info) => calc_result(&desc, Err(info.payload()), None, None),
-            None => calc_result(&desc, Ok(()), None, None),
-        };
+        let test_result = calc_result(&desc, panic_info.map(|info| info.payload()), None, None);
 
         // We don't support serializing TrFailedMsg, so just
         // print the message out to stderr.
diff --git a/library/test/src/test_result.rs b/library/test/src/test_result.rs
index 73dcc2e2a0cca..959cd730fa432 100644
--- a/library/test/src/test_result.rs
+++ b/library/test/src/test_result.rs
@@ -39,15 +39,18 @@ pub enum TestResult {
 
 /// Creates a `TestResult` depending on the raw result of test execution
 /// and associated data.
-pub(crate) fn calc_result<'a>(
+pub(crate) fn calc_result(
     desc: &TestDesc,
-    task_result: Result<(), &'a (dyn Any + 'static + Send)>,
+    panic_payload: Option<&(dyn Any + Send)>,
     time_opts: Option<&time::TestTimeOptions>,
     exec_time: Option<&time::TestExecTime>,
 ) -> TestResult {
-    let result = match (&desc.should_panic, task_result) {
-        (&ShouldPanic::No, Ok(())) | (&ShouldPanic::Yes, Err(_)) => TestResult::TrOk,
-        (&ShouldPanic::YesWithMessage(msg), Err(err)) => {
+    let result = match (desc.should_panic, panic_payload) {
+        // The test did or didn't panic, as expected.
+        (ShouldPanic::No, None) | (ShouldPanic::Yes, Some(_)) => TestResult::TrOk,
+
+        // Check the actual panic message against the expected message.
+        (ShouldPanic::YesWithMessage(msg), Some(err)) => {
             let maybe_panic_str = err
                 .downcast_ref::<String>()
                 .map(|e| &**e)
@@ -71,10 +74,14 @@ pub(crate) fn calc_result<'a>(
                 ))
             }
         }
-        (&ShouldPanic::Yes, Ok(())) | (&ShouldPanic::YesWithMessage(_), Ok(())) => {
+
+        // The test should have panicked, but didn't panic.
+        (ShouldPanic::Yes, None) | (ShouldPanic::YesWithMessage(_), None) => {
             TestResult::TrFailedMsg("test did not panic as expected".to_string())
         }
-        _ => TestResult::TrFailed,
+
+        // The test should not have panicked, but did panic.
+        (ShouldPanic::No, Some(_)) => TestResult::TrFailed,
     };
 
     // If test is already failed (or allowed to fail), do not change the result.