Re: [PATCH v2 0/3] rust: kunit: support #[should_panic] and same test name with different #[cfg(...)]

[David Gow <david@davidgow.net>]

Le 05/11/2026 à 9:46 PM, Nicolás Antinori a écrit :
> On Tue Nov 3, 2026 at 11:45 AM -03, David Gow wrote:
>> Thanks very much, Nicolás.
>>
>> I've had a closer look at the new version of this, and have a few 
>> comments. Ultimately, I think a mix of this version and the original RFC 
>> is probably best, and in particular, using a panic_handler instead of 
>> modifying kunit_try_catch, and removing the special-case assert!() 
>> handling would be ideal.
>>
>> Le 30/10/2026 à 00:59, Nicolás Antinori via B4 Relay a écrit :
>>> This patch series intends to implement two features for KUnit tests
>>> written in Rust. The work is based on a TODO comment made in the
>>> `bitmap.rs` module [1].
>>>
>>> Changelog:
>>>
>>> v2:
>>> - Added more tests in `rust/kernel/kunit.rs` testing Sashiko findings
>>>    and changes.
>>> - Solved Sashiko: "[Low] kunit: spurious "try faulted" error logs for
>>>    successful #[should_panic] tests": Solved by adding
>>>    `rust_should_panic` in `kunit_try_catch` C struct to check if the test
>>>    is expected to panic and act accordingly. Dismissed what I first talked
>>>    about in [3].
>>
>> I think if we only handle Rust panics, this should go away, right?
> 
> Yes, it should. Since we won't `BUG()` anymore in case a test should
> panic, the test thread won't exit with `-EINTR`. Because of this we
> won't need to shield the feature behind a Kconfig symbol either, since
> we won't have error logs in successful tests.
> 
> Here's the raw output from the panic handler version:
> 
> KTAP version 1
> 1..1
>     KTAP version 1
>     # Subtest: rust_kernel_kunit
>     # speed: normal
>     1..1
>     # rust_test_kunit_panic_in_kunit_test.speed: normal
>     ok 1 rust_test_kunit_panic_in_kunit_test
> ok 1 rust_kernel_kunit
> 
> for test:
> 
> #[test]
> #[should_panic]
> fn rust_test_kunit_panic_in_kunit_test() {
>     panic!("This test should panic and pass");
> }
> 

Looks great!

>>> - Now the tests that have an inactive #[cfg(..)] do not appear un the
>>>    KUnit execution summary as ignored, they just do not run.
>>> Link to v1: https://lore.kernel.org/all/20260930-rust_kunit_v4-v1-0-e3b0d45311dd@gmail.com/
>>>
>>> - RFC patch: https://lore.kernel.org/rust-for-linux/cover.1789500084.git.nico.antinori.7@gmail.com/
>>>
>>> 1. Supporting `#[should_panic]` [2]:
>>>
>>> KUnit tests in Rust follow the user-space syntax, but at the moment
>>> `#[should_panic]` is not supported. The first patch of this series adds
>>> support for the attribute (only in its basic form, `#[should_panic =
>>> "message"]` is not supported yet).
>>>
>>> The way it is supported is by adding a new boolean property called
>>> `rust_should_panic` to `kunit_attributes` struct in kunit/test.h and in
>>> `kunit_attributes` in kunit/try-catch.h and check for the
>>> `try_catch->try_result` code and the named flag to detect if there was a
>>> panic in `kunit_try_catch_run` function.
>>
>> I think rust_should_panic as an attribute is good, but it should just be 
>> checked from a panic_handler. If it supports more than just rust panics, 
>> it probably shouldn't be called *rust_*should_panic (and again, should 
>> probably be based on the warning suppression instead).
> 
> If we consider failed `assert!` from outside a #[test] function panics,
> then I'll leave it like this, otherwise I am thinking of
> `rust_should_fail`.
I had a closer look at this: the assert!() macro (and equivalents, like
assert_eq!(), etc.) are defined to call panic!():
 https://rust.docs.kernel.org/core/macro.assert.html

So I'd definitely keep the rust_should_panic name. (You could make an
argument for `should_rust_panic` instead, but I don't particularly like it.)

I don't think `rust_should_fail` would be a good name: the whole reason
this affects assertions is that they become panics, and we're
implementating rust's "should_panic", so should keep that nomenclature
so long as we're aiming for compatibility with that.

Cheers,
-- David