Rollup merge of rust-lang#100451 - hovinen:no-panic-on-result-err-in-test, r=Mark-Simulacrum

Do not panic when a test function returns Result::Err.

Rust's test library allows test functions to return a `Result`, so that the test is deemed to have failed if the function returns a `Result::Err` variant. Currently, this works by having `Result` implement the `Termination` trait and asserting in assert_test_result that `Termination::report()` indicates successful completion. This turns a `Result::Err` into a panic, which is caught and unwound in the test library.

This approach is problematic in certain environments where one wishes to save on both binary size and compute resources when running tests by:

 * Compiling all code with `--panic=abort` to avoid having to generate unwinding tables, and
 * Running most tests in-process to avoid the overhead of spawning new processes.

This change removes the intermediate panic step and passes a `Result::Err` directly through to the test runner.

To do this, it modifies `assert_test_result` to return a `Result<(), String>` where the `Err` variant holds what was previously the panic message. It changes the types in the `TestFn` enum to return `Result<(), String>`.

This tries to minimise the changes to benchmark tests, so it calls `unwrap()` on the `Result` returned by `assert_test_result`, effectively keeping the same behaviour as before.

Some questions for reviewers:

 * Does the change to the return types in the enum `TestFn` constitute a breaking change for the library API? Namely, the enum definition is public but the test library indicates that "Currently, not much of this is meant for users" and most of the library API appears to be marked unstable.
 * Is there a way to test this change, i.e., to test that no panic occurs if a test returns `Result::Err`?
 * Is there a shorter, more idiomatic way to fold `Result<Result<T,E>,E>` into a `Result<T,E>` than the `fold_err` function I added?

Author: Dylan-DPC

library/test/src/bench.rs

@@ -49,12 +49,12 @@ impl Bencher {
         self.summary = Some(iter(&mut inner));
     }
 
-    pub fn bench<F>(&mut self, mut f: F) -> Option<stats::Summary>
+    pub fn bench<F>(&mut self, mut f: F) -> Result<Option<stats::Summary>, String>
     where
-        F: FnMut(&mut Bencher),
+        F: FnMut(&mut Bencher) -> Result<(), String>,
     {
-        f(self);
-        self.summary
+        let result = f(self);
+        result.map(|_| self.summary)
     }
 }
 
@@ -195,7 +195,7 @@ pub fn benchmark<F>(
     nocapture: bool,
     f: F,
 ) where
-    F: FnMut(&mut Bencher),
+    F: FnMut(&mut Bencher) -> Result<(), String>,
 {
     let mut bs = Bencher { mode: BenchMode::Auto, summary: None, bytes: 0 };
 
@@ -211,32 +211,33 @@ pub fn benchmark<F>(
 
     let test_result = match result {
         //bs.bench(f) {
-        Ok(Some(ns_iter_summ)) => {
+        Ok(Ok(Some(ns_iter_summ))) => {
             let ns_iter = cmp::max(ns_iter_summ.median as u64, 1);
             let mb_s = bs.bytes * 1000 / ns_iter;
 
             let bs = BenchSamples { ns_iter_summ, mb_s: mb_s as usize };
             TestResult::TrBench(bs)
         }
-        Ok(None) => {
+        Ok(Ok(None)) => {
             // iter not called, so no data.
             // FIXME: error in this case?
             let samples: &mut [f64] = &mut [0.0_f64; 1];
             let bs = BenchSamples { ns_iter_summ: stats::Summary::new(samples), mb_s: 0 };
             TestResult::TrBench(bs)
         }
         Err(_) => TestResult::TrFailed,
+        Ok(Err(_)) => TestResult::TrFailed,
     };
 
     let stdout = data.lock().unwrap().to_vec();
     let message = CompletedTest::new(id, desc, test_result, None, stdout);
     monitor_ch.send(message).unwrap();
 }
 
-pub fn run_once<F>(f: F)
+pub fn run_once<F>(f: F) -> Result<(), String>
 where
-    F: FnMut(&mut Bencher),
+    F: FnMut(&mut Bencher) -> Result<(), String>,
 {
     let mut bs = Bencher { mode: BenchMode::Single, summary: None, bytes: 0 };
-    bs.bench(f);
+    bs.bench(f).map(|_| ())
 }

library/test/src/lib.rs

@@ -6,7 +6,8 @@
 //! benchmarks themselves) should be done via the `#[test]` and
 //! `#[bench]` attributes.
 //!
-//! See the [Testing Chapter](../book/ch11-00-testing.html) of the book for more details.
+//! See the [Testing Chapter](../book/ch11-00-testing.html) of the book for more
+//! details.
 
 // Currently, not much of this is meant for users. It is intended to
 // support the simplest interface possible for representing and
@@ -76,6 +77,7 @@ mod types;
 #[cfg(test)]
 mod tests;
 
+use core::any::Any;
 use event::{CompletedTest, TestEvent};
 use helpers::concurrency::get_concurrency;
 use helpers::exit_code::get_exit_code;
@@ -175,17 +177,20 @@ fn make_owned_test(test: &&TestDescAndFn) -> TestDescAndFn {
     }
 }
 
-/// Invoked when unit tests terminate. Should panic if the unit
-/// Tests is considered a failure. By default, invokes `report()`
-/// and checks for a `0` result.
-pub fn assert_test_result<T: Termination>(result: T) {
+/// Invoked when unit tests terminate. Returns `Result::Err` if the test is
+/// considered a failure. By default, invokes `report() and checks for a `0`
+/// result.
+pub fn assert_test_result<T: Termination>(result: T) -> Result<(), String> {
     let code = result.report().to_i32();
-    assert_eq!(
-        code, 0,
-        "the test returned a termination value with a non-zero status code ({}) \
-         which indicates a failure",
-        code
-    );
+    if code == 0 {
+        Ok(())
+    } else {
+        Err(format!(
+            "the test returned a termination value with a non-zero status code \
+             ({}) which indicates a failure",
+            code
+        ))
+    }
 }
 
 pub fn run_tests<F>(
@@ -478,7 +483,7 @@ pub fn run_test(
         id: TestId,
         desc: TestDesc,
         monitor_ch: Sender<CompletedTest>,
-        testfn: Box<dyn FnOnce() + Send>,
+        testfn: Box<dyn FnOnce() -> Result<(), String> + Send>,
         opts: TestRunOpts,
     ) -> Option<thread::JoinHandle<()>> {
         let concurrency = opts.concurrency;
@@ -567,19 +572,19 @@ pub fn run_test(
 
 /// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`.
 #[inline(never)]
-fn __rust_begin_short_backtrace<F: FnOnce()>(f: F) {
-    f();
+fn __rust_begin_short_backtrace<T, F: FnOnce() -> T>(f: F) -> T {
+    let result = f();
 
     // prevent this frame from being tail-call optimised away
-    black_box(());
+    black_box(result)
 }
 
 fn run_test_in_process(
     id: TestId,
     desc: TestDesc,
     nocapture: bool,
     report_time: bool,
-    testfn: Box<dyn FnOnce() + Send>,
+    testfn: Box<dyn FnOnce() -> Result<(), String> + Send>,
     monitor_ch: Sender<CompletedTest>,
     time_opts: Option<time::TestTimeOptions>,
 ) {
@@ -591,7 +596,7 @@ fn run_test_in_process(
     }
 
     let start = report_time.then(Instant::now);
-    let result = catch_unwind(AssertUnwindSafe(testfn));
+    let result = fold_err(catch_unwind(AssertUnwindSafe(testfn)));
     let exec_time = start.map(|start| {
         let duration = start.elapsed();
         TestExecTime(duration)
@@ -608,6 +613,19 @@ fn run_test_in_process(
     monitor_ch.send(message).unwrap();
 }
 
+fn fold_err<T, E>(
+    result: Result<Result<T, E>, Box<dyn Any + Send>>,
+) -> Result<T, Box<dyn Any + Send>>
+where
+    E: Send + 'static,
+{
+    match result {
+        Ok(Err(e)) => Err(Box::new(e)),
+        Ok(Ok(v)) => Ok(v),
+        Err(e) => Err(e),
+    }
+}
+
 fn spawn_test_subprocess(
     id: TestId,
     desc: TestDesc,
@@ -663,7 +681,10 @@ fn spawn_test_subprocess(
     monitor_ch.send(message).unwrap();
 }
 
-fn run_test_in_spawned_subprocess(desc: TestDesc, testfn: Box<dyn FnOnce() + Send>) -> ! {
+fn run_test_in_spawned_subprocess(
+    desc: TestDesc,
+    testfn: Box<dyn FnOnce() -> Result<(), String> + Send>,
+) -> ! {
     let builtin_panic_hook = panic::take_hook();
     let record_result = Arc::new(move |panic_info: Option<&'_ PanicInfo<'_>>| {
         let test_result = match panic_info {
@@ -689,7 +710,9 @@ fn run_test_in_spawned_subprocess(desc: TestDesc, testfn: Box<dyn FnOnce() + Sen
     });
     let record_result2 = record_result.clone();
     panic::set_hook(Box::new(move |info| record_result2(Some(&info))));
-    testfn();
+    if let Err(message) = testfn() {
+        panic!("{}", message);
+    }
     record_result(None);
     unreachable!("panic=abort callback should have exited the process")
 }