Re: [PATCH v2 1/3] rust: kunit: add #[should_panic] support

[David Gow]

Le 05/11/2026 à 1:58 AM, Nicolás Antinori a écrit :
> On Tue Nov 3, 2026 at 11:46 AM -03, David Gow wrote:
>> Le 30/10/2026 à 00:59, Nicolás Antinori via B4 Relay a écrit :
>>> From: Nicolás Antinori <nico.antinori.7@xxxxxxxxx>
>>>
>>> KUnit tests in Rust are written using user-space like syntax. This patch
>>> adds support for the `#[should_panic]` attribute, enabling the user to
>>> test conditions that are expected to cause a panic and report the test
>>> as successful.
>>>
>>> Signed-off-by: Nicolás Antinori <nico.antinori.7@xxxxxxxxx>
>>> ---
>>
>> Thanks very much.
>>
>> I'm afraid I'd rather go back to the RFC's design of only triggering on 
>> Rust panic!(), rather than on failures in C. It's a much simpler 
>> implementation, and makes more sense for a rust-specific #[should_panic] 
>> to me.
> 
> No problem at all! 
> 
> I'll be sending the v3 with the panic handler then. I decided go this
> way because I thought it would be useful to catch C panics, but those
> would be rather rare cases.
> 
> The only thing I am not yet sure regarding the panic handler
> implementation is how to read the `rust_should_panic` attribute from
> `kunit_case` struct. If I am not mistaken, there's no way to retrieve
> the test case linked to the running KUnit test from Rust. Would it be ok
> to add a public `kunit_get_current_test_case()`? I am not sure if that
> struct is meant to be used outside the runner.

Yeah, this is a pain. In theory, struct kunit_case / struct kunit_suite
are internal to the executor, but we end up having to pass them around a
lot, and a way of getting the current test's attributes is, at a
minimum, something we should support.

I'd rather not expose the struct kunit_case to test authors (though
exposing it to the Rust macros is fine by me: they're sufficiently
KUnit-internal that it's fine). The ultimate best 'public' API would
probably be to have a bunch of 'kunit_get_attribute_value()' style
things, but that's definitely a more involved thing.

How about we compromise on a way to get the attributres for the current
test. The simplest implementation for that is probably just including a
pointer to struct kunit_attributes in struct kunit.

(Though the better long-term solution would be to pass a struct
kunit_case pointer in struct kunit, and generally rework things to not
pass struct kunit_case/kunit_suite around as regularly. There are a lot
of gotcha's there, though, so it's not worth holding this up on that.)

>> Secondly, I'm not convinced we want to alter the behaviour of 
>> assertions. At least KUNIT_ASSERT_* should still be useful in a test 
>> ensuring that the state of the system being tested makes sense before a 
>> deliberate panic!(), and arbitrarily reversing the meaning of assert!() 
>> feels confusing to me.
> 
> I am going to rollback this change. 
> 
> The thing I think is worth keeping is declaring the redefinition of
> `assert!` and `assert_eq!` inside the function's body because depending
> on the order of the code, an `assert!` in a non `#[test]` function
> inside the test module can get replaced by a `kunit_assert!` (this was
> something caught by Sashiko in the v2 review).
> 
>> That being said, I understand if we want assert!() from _outside_ the 
>> test (i.e., an assert!() in the code being tested, not the test itself) 
>> to be considered a panic!().
> 
> IIRC the panic_handler version did this.
> 
>>> +		} else {
>>> +			if (test->last_seen.file)
>>> +				kunit_err(test, "try faulted: last line seen %s:%d\n",
>>> +					  test->last_seen.file, test->last_seen.line);
>>> +			else
>>> +				kunit_err(test, "try faulted\n");
>>> +		}
>>>   	} else if (exit_code == -ETIMEDOUT)
>>>   		kunit_err(test, "try timed out\n");
>>>   	else if (exit_code)
>>> diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
>>> index 25ff272d29b2b3f192a3968481c1d5417df4335b..5e688b743ee2152867e2f99c7f18b86e1b7fbe3d 100644
>>> --- a/rust/kernel/kunit.rs
>>> +++ b/rust/kernel/kunit.rs
>>> @@ -58,7 +58,7 @@ pub fn info(args: fmt::Arguments<'_>) {
>>>   #[doc(hidden)]
>>>   #[macro_export]
>>>   macro_rules! kunit_assert {
>>> -    ($name:literal, $condition:expr $(,)?) => {
>>> +    ($name:literal, $test_should_panic:expr, $condition:expr $(,)?) => {
>>>           'out: {
>>>               // Do nothing if the condition is `true`.
>>>               if $condition {
>>> @@ -93,6 +93,14 @@ macro_rules! kunit_assert {
>>>                   ));
>>>                   break 'out;
>>>               }
>>> +            // If condition fails but the test should panic, then we just abort.
>>> +            else if $test_should_panic {
>>> +                // SAFETY: FFI call. The `kunit_test` pointer is valid because we got it from
>>> +                // `kunit_get_current_test()` and it was not null.
>>> +                unsafe {
>>> +                    $crate::bindings::__kunit_abort(kunit_test);
>>> +                }
>>> +            }
>>
>> I'd rather us keep asserts working as normal: a failed KUnit assertion 
>> shouldn't be considered a panic. If normal Rust assertions (particularly 
>> those outside the test-specific code) compile down to a panic, though, 
>> that's okay.
> 
> I'll revert this. I made the change because in user space those tests
> pass [1] [*].

Having thought about this a bit more, I think there's a compromise:
could we make the override of assert!() and assert_eq!() only appear on
non-should_panic tests. In that case, the assert!() and assert_eq!()
will behave the same as userspace tests (indeed, they'll use the same
code), and we won't need a complicated implementation. And later if we
want the (IMO, more logical) KUnit-style assertions which aren't
negated, we could suggest people use kernel::kunit_assert!() directly
(or with a small wrapper), so it's obvious the behaviour is different
from upstream assert!().

Thoughts?

> 
>>> +    }
>>> +
>>> +    #[test]
>>> +    #[should_panic]
>>> +    fn rust_test_assert_false_success_with_should_panic() {
>>> +        assert!(false)
>>
>> I'd rather this not pass, personally. I think that an assert!() within a 
>> test should work normally (i.e., it's possible to assert that the test 
>> state is normal, and a failing assert will fail the test).
> 
> Same comment as [*].
> 
> Thank you very very much for the detailed review in the series and the
> insights! 
> 
> Also, thank you Miguel for joining the discussion!
> 
> Best regards,
> Nicolás 
> 
> [1] https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=2b4b29a266271497ea9932068ecdb8ff


Thanks again for the patch!

Cheers,
-- David