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

[hovinen]

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?

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

[Mark-Simulacrum]

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.

"Yes", but it's generally fine to change unstable APIs, so this is fine as-is. Based on not needing to update compiletest it'll affect a very small fraction of users most likely, so I'm not worried.

Is there a way to test this change, i.e., to test that no panic occurs if a test returns Result::Err?

Hm, I'm not sure. I would have suggested -Cpanic=abort and --test would work, but it looks like rustc currently hard errors on that (without -Zpanic_abort_tests, which IIRC spawns separate processes). It's probably OK to go without a test here.

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?

Not that I'm aware of.

---

Running most tests in-process to avoid the overhead of spawning new processes.

Can you say more about this? Are you expecting a future patch to differentiate between Result returning tests and those that don't, and only spawn processes for the latter? It seems to me that we'd be unable to make that delta unless we're willing to accept panics in result-returning tests aborting the test harness entirely (maybe yes, but not an obvious tradeoff to me).

[Mark-Simulacrum]

@rustbot author

[hovinen]

Thanks for the quick review!

(Also sorry for the late reply; I was on vacation last week.)

Running most tests in-process to avoid the overhead of spawning new processes.

Can you say more about this? Are you expecting a future patch to differentiate between Result returning tests and those that don't, and only spawn processes for the latter? It seems to me that we'd be unable to make that delta unless we're willing to accept panics in result-returning tests aborting the test harness entirely (maybe yes, but not an obvious tradeoff to me).

In our systems it is most likely that (nearly) all tests will return Result and panics will only occur in exceptional cases. We won't likely use the existing assertion macros like assert_eq! but rather represent test failures by having the test return a Result::Err. This is similar to how GoogleTest works for C++, where the ASSERT_* macros just return from the current function rather than throwing an exception or similar (see https://google.github.io/googletest/primer.html#assertions).

So no, we won't have any need to differentiate between Result-returning and non-Result-returning test functions in deciding whether to spawn separate test processes.

[Mark-Simulacrum]

@rustbot ready

Putting this back in my review queue.

[Mark-Simulacrum]

Please squash in the fixup commits (e.g., formatting, review comments).

Looking over this again I think part of me wants to suggest we go all the way here and make it the responsibility of the test harness (or similar) to catch the panics very close to the test itself, and then have the rest of the code work purely with a Result. The code here feels pretty spread out to me though and I'm not sure there's a great place for that to happen. It doesn't feel like this patch is really improving that situation but it also doesn't feel like it makes it worse, just kind of neutral.

I think I'm inclined to move ahead -- perhaps with a possible test ala #100451 (comment) added around that comment.

[hovinen]

Please squash in the fixup commits (e.g., formatting, review comments).

Done.

Looking over this again I think part of me wants to suggest we go all the way here and make it the responsibility of the test harness (or similar) to catch the panics very close to the test itself, and then have the rest of the code work purely with a Result. The code here feels pretty spread out to me though and I'm not sure there's a great place for that to happen. It doesn't feel like this patch is really improving that situation but it also doesn't feel like it makes it worse, just kind of neutral.

I agree.

I think I'm inclined to move ahead -- perhaps with a possible test ala #100451 (comment) added around that comment.

Sounds good, thanks! As mentioned in the comment above, it seems the test to which you linked does in fact catch that problem -- for static benches, anyway -- but was somehow not triggered by CI. I'd be happy to write a test to cover the dynamic benchmark case if you can point me to how to write dynamic benches.

[Mark-Simulacrum]

@bors r+

[bors]

📌 Commit e19a98c has been approved by Mark-Simulacrum

It is now in the queue for this repository.