Add support for `#[should_panic]` macro #151

skogseth · 2025-11-03T18:46:08Z

This is done by recursively parsing attributes following the #[libtest2::test] macro, and then parsing the first #[should_panic] macro encountered. If a macro is encountered then the test will be run within a catch_unwind, which is done by wrapping it in the assert_panic macro (function style).

Potentially supporting multiple #[should_panic] macros is left for the future.

Fixes #15

coveralls · 2025-11-03T18:48:26Z

Pull Request Test Coverage Report for Build 19069497492

Details

16 of 35 (45.71%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 30.902%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/libtest2/src/panic.rs	0	19	0.0%

Totals
Change from base Build 18990226994:	0.3%
Covered Lines:	521
Relevant Lines:	1686

💛 - Coveralls

crates/libtest2/src/macros.rs

epage · 2025-11-03T21:16:56Z

crates/libtest2/src/macros.rs

                $(
-                    match $crate::_private::parse_ignore!($($attr)*) {
-                        ::std::option::Option::None => context.ignore()?,
-                        ::std::option::Option::Some(reason) => context.ignore_for(reason)?,
-                    }
-                )*
+                    $crate::_private::parse_ignore!(context, $ignore)?;
+                )?


Should we move the conditional nature of $ignore into _parse_ignore, simplifying this?

epage · 2025-11-03T21:18:17Z

crates/libtest2/src/macros.rs

+    // Discard unknown attributes
+    (continue: name=$name:ident body=[$($item:tt)*] attrs=[#[$($unknown_attr:tt)+] $(#[$($attr:tt)+])*] $(ignore=$ignore:tt)?) => {


Is there a reason we aren't doing compile errors for them anymore?

Side note: we need to add trybuild to check the error reporting of our macros

crates/libtest2/src/macros.rs

epage · 2025-11-03T21:21:03Z

crates/libtest2/src/macros.rs

+#[macro_export]
+macro_rules! assert_panic {


We should add documentation and tests for this

epage · 2025-11-03T21:21:41Z

crates/libtest2/src/macros.rs

+#[macro_export]
+macro_rules! assert_panic {


How much of this can live in a function? This is a lot of code to put in a macro which can negatively impact compile times

We can put everything in a function: assert_panic(f: impl FnOnce(), panic_message: Option<&str>). That's a pretty decent API, but it has the slight disadvantage of the Option being there at runtime, even though it should always be known at compile time whether or not there is a panic message. Normally that seems fine to me, but since this is "test code" most people won't have optimizations on, so I'm less happy about that.

We can also do two functions: assert_panic(f: impl FnOnce()) & assert_panic_with_message(f: impl FnOnce(), panic_message: &str). It's also possible to then hide those behind an assert_panic macro similar to the one implemented here, which delegates to the correct function call.

If assert_panic is an implementation detail of the macro, then I don't mind there being one variant. The runtime overhead of Option should be minimal.

If it's not, then I would go with assert_panic and assert_panic_message (don't need with because we're asserting on the message, not panicking itself) / assert_panic_contains (express what message operation we do).

One thing in favor of a macro is adding a display message like other assert macros ... but that would be confusing with the panic message being optional.

epage · 2025-11-03T21:22:14Z

crates/libtest2/src/macros.rs

+        match ::std::panic::catch_unwind(::std::panic::AssertUnwindSafe(|| $f)) {
+            // The test should have panicked, but didn't.
+            ::std::result::Result::Ok(_) => {
+                // TODO: Rust includes the source file location here, consider doing the same?


We should resolve TODOs or track them in issues before merging, rather than having a second backlog

epage · 2025-11-03T21:24:45Z

crates/libtest2/src/macros.rs

+    ($f:expr $(,)?) => {
+        match ::std::panic::catch_unwind(::std::panic::AssertUnwindSafe(|| $f)) {
+            // The test should have panicked, but didn't.
+            ::std::result::Result::Ok(_) => {
+                // TODO: Rust includes the source file location here, consider doing the same?
+                ::std::result::Result::Err($crate::RunError::fail("test did not panic as expected"))
+            }
+
+            // The test panicked, as expected.
+            ::std::result::Result::Err(_) => Ok(()),
+        }


Would there be a good way to unify these code paths, like turning expected into an Option<&str>?

Yes, definitely, absolutely. If it's okay to merge these paths then I think swapping to an assert_panic function (as mentioned in another comment) would be best.

epage · 2025-11-03T21:26:57Z

crates/libtest2/tests/testsuite/should_panic.rs

+#[should_panic = "very long and specific message"]
+fn passes_because_subsequent_panics_are_ignored(_context: &libtest2::TestContext) {


I wonder if we should error instead. We could always defer this question to another issue.

epage · 2025-11-03T21:27:43Z

crates/libtest2/tests/testsuite/should_panic.rs

+---- panics_with_the_wrong_message ----
+panic did not contain expected string
+      panic message: "with the wrong message"
+ expected substring: "in a controlled manner"
+
+
+failures:
+    accidentally_panics
+    panics_with_the_wrong_message
+
+test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 filtered out; finished in [..]s


Does libtest show the backtrace for the panic? Are we able to do that?

epage · 2025-11-03T21:28:08Z

crates/libtest2/src/macros.rs

Could you update the documentation in src/lib.rs with any deviations from libtest?

The `test_parse` macro is changed to recursively parse attributes, filtering out the `#[ignore]` macro. This should also make it fairly trivial to add additional macros, such as `#[should_ignore]`. There is a fair amount of complexity added to the macro with this change, but on the other hand, it also succeeds in generating only the necessary code. You win some, you lose some.

skogseth force-pushed the should_panic branch from 0ff7c92 to 1cf8554 Compare November 3, 2025 18:47

epage reviewed Nov 3, 2025

View reviewed changes

crates/libtest2/src/macros.rs Show resolved Hide resolved

epage reviewed Nov 3, 2025

View reviewed changes

crates/libtest2/src/macros.rs Outdated Show resolved Hide resolved

epage reviewed Nov 3, 2025

View reviewed changes

crates/libtest2/src/macros.rs

Copy link

Collaborator

epage Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the documentation in src/lib.rs with any deviations from libtest?

skogseth force-pushed the should_panic branch from 1cf8554 to b69f17b Compare November 4, 2025 08:46

skogseth added 3 commits November 4, 2025 14:01

feat: Add assert_panic function

eacbd66

feat: Add support for '#[should_panic]' macro

ee1e4e3

fix: Add tests for '#[should_panic]' macro

dea0e5f

skogseth force-pushed the should_panic branch from b69f17b to dea0e5f Compare November 4, 2025 13:01

		// Discard unknown attributes
		(continue: name=$name:ident body=[$($item:tt)] attrs=[#[$($unknown_attr:tt)+] $(#[$($attr:tt)+])] $(ignore=$ignore:tt)?) => {

		#[should_panic = "very long and specific message"]
		fn passes_because_subsequent_panics_are_ignored(_context: &libtest2::TestContext) {

		#[macro_export]
		macro_rules! assert_panic {

		#[macro_export]
		macro_rules! assert_panic {

Add support for #[should_panic] macro #151

Are you sure you want to change the base?

Add support for #[should_panic] macro #151

Conversation

skogseth commented Nov 3, 2025

Uh oh!

coveralls commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 19069497492

Details

💛 - Coveralls

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add support for `#[should_panic]` macro #151

Add support for `#[should_panic]` macro #151

coveralls commented Nov 3, 2025 •

edited

Loading