mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Bradford Hovinen 2022-07-28 14:45:43 +00:00
parent 2d1aa57d1e
commit e19a98cff7
7 changed files with 135 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
library/test/src/bench.rs
View File

@ -49,12 +49,12 @@ pub fn iter<T, F>(&mut self, mut inner: F)
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,14 +211,14 @@ 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];
@ -226,6 +226,7 @@ pub fn benchmark<F>(
TestResult::TrBench(bs)
}
Err(_) => TestResult::TrFailed,
Ok(Err(_)) => TestResult::TrFailed,
};
let stdout = data.lock().unwrap().to_vec();
@ -233,10 +234,10 @@ pub fn benchmark<F>(
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(|_| ())
}

61
library/test/src/lib.rs
View File

@ -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
@ -77,6 +78,7 @@ pub mod test {
#[cfg(test)]
mod tests;
use core::any::Any;
use event::{CompletedTest, TestEvent};
use helpers::concurrency::get_concurrency;
use helpers::exit_code::get_exit_code;
@ -176,17 +178,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>(
@ -479,7 +484,7 @@ fn run_test_inner(
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;
@ -568,11 +573,11 @@ fn run_test_inner(
/// 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(
@ -580,7 +585,7 @@ fn run_test_in_process(
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>,
) {
@ -592,7 +597,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)
@ -609,6 +614,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,
@ -664,7 +682,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 {
@ -690,7 +711,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")
}

90
library/test/src/tests.rs
View File

@ -67,7 +67,7 @@ fn one_ignored_one_unignored_test() -> Vec<TestDescAndFn> {
no_run: false,
test_type: TestType::Unknown,
},
testfn: DynTestFn(Box::new(move || {})),
testfn: DynTestFn(Box::new(move || Ok(()))),
},
TestDescAndFn {
desc: TestDesc {
@ -79,14 +79,14 @@ fn one_ignored_one_unignored_test() -> Vec<TestDescAndFn> {
no_run: false,
test_type: TestType::Unknown,
},
testfn: DynTestFn(Box::new(move || {})),
testfn: DynTestFn(Box::new(move || Ok(()))),
},
]
}
#[test]
pub fn do_not_run_ignored_tests() {
fn f() {
fn f() -> Result<(), String> {
panic!();
}
let desc = TestDescAndFn {
@ -109,7 +109,9 @@ fn f() {
#[test]
pub fn ignored_tests_result_in_ignored() {
fn f() {}
fn f() -> Result<(), String> {
Ok(())
}
let desc = TestDescAndFn {
desc: TestDesc {
name: StaticTestName("whatever"),
@ -132,7 +134,7 @@ fn f() {}
#[test]
#[cfg(not(target_os = "emscripten"))]
fn test_should_panic() {
fn f() {
fn f() -> Result<(), String> {
panic!();
}
let desc = TestDescAndFn {
@ -157,7 +159,7 @@ fn f() {
#[test]
#[cfg(not(target_os = "emscripten"))]
fn test_should_panic_good_message() {
fn f() {
fn f() -> Result<(), String> {
panic!("an error message");
}
let desc = TestDescAndFn {
@ -183,7 +185,7 @@ fn f() {
#[cfg(not(target_os = "emscripten"))]
fn test_should_panic_bad_message() {
use crate::tests::TrFailedMsg;
fn f() {
fn f() -> Result<(), String> {
panic!("an error message");
}
let expected = "foobar";
@ -214,7 +216,7 @@ fn f() {
fn test_should_panic_non_string_message_type() {
use crate::tests::TrFailedMsg;
use std::any::TypeId;
fn f() {
fn f() -> Result<(), String> {
std::panic::panic_any(1i32);
}
let expected = "foobar";
@ -249,7 +251,9 @@ fn test_should_panic_but_succeeds() {
let should_panic_variants = [ShouldPanic::Yes, ShouldPanic::YesWithMessage("error message")];
for &should_panic in should_panic_variants.iter() {
fn f() {}
fn f() -> Result<(), String> {
Ok(())
}
let desc = TestDescAndFn {
desc: TestDesc {
name: StaticTestName("whatever"),
@ -283,7 +287,9 @@ fn f() {}
}
fn report_time_test_template(report_time: bool) -> Option<TestExecTime> {
fn f() {}
fn f() -> Result<(), String> {
Ok(())
}
let desc = TestDescAndFn {
desc: TestDesc {
name: StaticTestName("whatever"),
@ -318,7 +324,9 @@ fn test_should_report_time() {
}
fn time_test_failure_template(test_type: TestType) -> TestResult {
fn f() {}
fn f() -> Result<(), String> {
Ok(())
}
let desc = TestDescAndFn {
desc: TestDesc {
name: StaticTestName("whatever"),
@ -480,7 +488,7 @@ pub fn exclude_should_panic_option() {
no_run: false,
test_type: TestType::Unknown,
},
testfn: DynTestFn(Box::new(move || {})),
testfn: DynTestFn(Box::new(move || Ok(()))),
});
let filtered = filter_tests(&opts, tests);
@ -504,7 +512,7 @@ fn tests() -> Vec<TestDescAndFn> {
no_run: false,
test_type: TestType::Unknown,
},
testfn: DynTestFn(Box::new(move || {})),
testfn: DynTestFn(Box::new(move || Ok(()))),
})
.collect()
}
@ -580,7 +588,9 @@ fn sample_tests() -> Vec<TestDescAndFn> {
"test::run_include_ignored_option".to_string(),
"test::sort_tests".to_string(),
];
fn testfn() {}
fn testfn() -> Result<(), String> {
Ok(())
}
let mut tests = Vec::new();
for name in &names {
let test = TestDescAndFn {
@ -717,21 +727,26 @@ pub fn test_metricmap_compare() {
#[test]
pub fn test_bench_once_no_iter() {
fn f(_: &mut Bencher) {}
bench::run_once(f);
fn f(_: &mut Bencher) -> Result<(), String> {
Ok(())
}
bench::run_once(f).unwrap();
}
#[test]
pub fn test_bench_once_iter() {
fn f(b: &mut Bencher) {
b.iter(|| {})
fn f(b: &mut Bencher) -> Result<(), String> {
b.iter(|| {});
Ok(())
}
bench::run_once(f);
bench::run_once(f).unwrap();
}
#[test]
pub fn test_bench_no_iter() {
fn f(_: &mut Bencher) {}
fn f(_: &mut Bencher) -> Result<(), String> {
Ok(())
}
let (tx, rx) = channel();
@ -751,8 +766,9 @@ fn f(_: &mut Bencher) {}
#[test]
pub fn test_bench_iter() {
fn f(b: &mut Bencher) {
b.iter(|| {})
fn f(b: &mut Bencher) -> Result<(), String> {
b.iter(|| {});
Ok(())
}
let (tx, rx) = channel();
@ -821,3 +837,33 @@ fn should_sort_failures_before_printing_them() {
let bpos = s.find("b").unwrap();
assert!(apos < bpos);
}
#[test]
#[cfg(not(target_os = "emscripten"))]
fn test_dyn_bench_returning_err_fails_when_run_as_test() {
fn f(_: &mut Bencher) -> Result<(), String> {
Result::Err("An error".into())
}
let desc = TestDescAndFn {
desc: TestDesc {
name: StaticTestName("whatever"),
ignore: false,
ignore_message: None,
should_panic: ShouldPanic::No,
compile_fail: false,
no_run: false,
test_type: TestType::Unknown,
},
testfn: DynBenchFn(Box::new(f)),
};
let (tx, rx) = channel();
let notify = move |event: TestEvent| {
if let TestEvent::TeResult(result) = event {
tx.send(result).unwrap();
}
Ok(())
};
run_tests(&TestOpts { run_tests: true, ..TestOpts::new() }, vec![desc], notify).unwrap();
let result = rx.recv().unwrap().result;
assert_eq!(result, TrFailed);
}

15
library/test/src/types.rs
View File

@ -75,14 +75,15 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
}
// A function that runs a test. If the function returns successfully,
// the test succeeds; if the function panics then the test fails. We
// may need to come up with a more clever definition of test in order
// to support isolation of tests into threads.
// the test succeeds; if the function panics or returns Result::Err
// then the test fails. We may need to come up with a more clever
// definition of test in order to support isolation of tests into
// threads.
pub enum TestFn {
StaticTestFn(fn()),
StaticBenchFn(fn(&mut Bencher)),
DynTestFn(Box<dyn FnOnce() + Send>),
DynBenchFn(Box<dyn Fn(&mut Bencher) + Send>),
StaticTestFn(fn() -> Result<(), String>),
StaticBenchFn(fn(&mut Bencher) -> Result<(), String>),
DynTestFn(Box<dyn FnOnce() -> Result<(), String> + Send>),
DynBenchFn(Box<dyn Fn(&mut Bencher) -> Result<(), String> + Send>),
}
impl TestFn {

1
src/librustdoc/doctest.rs
View File

@ -1134,6 +1134,7 @@ fn add_test(&mut self, test: String, config: LangString, line: usize) {
panic::resume_unwind(Box::new(()));
}
Ok(())
})),
});
}

2
src/test/ui/rfc-1937-termination-trait/termination-trait-test-wrong-type.stderr
View File

@ -13,7 +13,7 @@ LL | | }
note: required by a bound in `assert_test_result`
--> $SRC_DIR/test/src/lib.rs:LL:COL
|
LL | pub fn assert_test_result<T: Termination>(result: T) {
LL | pub fn assert_test_result<T: Termination>(result: T) -> Result<(), String> {
| ^^^^^^^^^^^ required by this bound in `assert_test_result`
= note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

5
src/tools/compiletest/src/main.rs
View File

@ -805,7 +805,10 @@ fn make_test_closure(
let config = config.clone();
let testpaths = testpaths.clone();
let revision = revision.cloned();
test::DynTestFn(Box::new(move || runtest::run(config, &testpaths, revision.as_deref())))
test::DynTestFn(Box::new(move || {
runtest::run(config, &testpaths, revision.as_deref());
Ok(())
}))
}
/// Returns `true` if the given target is an Android target for the