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

["Nicolás Antinori" <nico.antinori.7@gmail.com>]

On Fri Nov 6, 2026 at 2:31 AM -03, David Gow wrote:
> 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:
>> 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.)

I'll add a kunit_attributes pointer to kunit then! I can rework this
later to add the public kunit_get_attributre_value functions.

>>>>   #[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?

I think it is a good solution! Making explicit that there is a different
behavior using a different kind of assert, will make the tests easier to
understand (since we are mimicking the userspace API, a Rust dev will
expect a #[should_panic] test to pass if a common assert! fails).

I'll be sending a v3 this week!

Regards,
Nicolás