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

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

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@gmail.com>
>> 
>> 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@gmail.com>
>> ---
>
> 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.

> 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] [*].

>> +    }
>> +
>> +    #[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