Re: [PATCH v2 1/3] rust: kunit: add #[should_panic] support
From: David Gow
Date: Tue Nov 03 2026 - 10:52:29 EST
Le 03/11/2026 à 05:25, Miguel Ojeda a écrit :
On Mon, Nov 2, 2026 at 9:53 PM Nicolás Antinori
<nico.antinori.7@xxxxxxxxx> wrote:
I first thought of disabling `#[should_panic]` tests by directly
checking the conditions in the macro code. I ended up creating a new
config because I thought it would be cleaner codewise, if a new
constraint arises in the future, we just have to add it to the
`depends on` clause instead of modifying the macro code.
It depends on what we want: if it is meant to be configurable, then it
should be a visible Kconfig symbol; if not, then it should remain an
implementation detail (which could perhaps be a non-visible Kconfig
symbol).
The reason KUNIT_FAULT_TEST was user-configurable is that some test harnesses / CI systems would automatically mark a test failed if they saw a panic/WARN stacktrace in the kernel output.
Yes! they are pretty similar, for `KUNIT_FAULT_TEST`:
Yeah, in that case, I think a new visible Kconfig symbol may be
justifiable -- please see below.
The only one that is different in `RUST_KUNIT_TESTS` is:
#[test]
#[should_panic]
fn rust_test_assert_false_success_with_should_panic() {
assert!(false)
}
I guess it is reasonable to expect this test to pass, since `assert!`
doesn't return and actually panics in normal Rust.
Having said that, I would expect our `should_panic` tests to be
generally about non-`assert*!` panics.
Yeah, I really don't like the way this effectively reverses what assert!() does. I'm sort-of okay with it in indirectly-called code (I want to check that this function panics if I pass an invalid input, and don't care whether it calls assert!() or panic!()), but it's really confusing from within the test function.
Plus there's a whole bunch of special-cased code to make kunit_assert!() behave this way in this patch, and if we could get rid of that, the whole thing would be much cleaner.
(Of course, that introduces an inconsistency depending on where the assert!() is, which isn't particularly nice, though I'd be prepared to tolerate it to make everything else behave as expected.)
I've always thought of CONFIG_KUNIT_FAULT_TEST as being a more general switch for tests which can cause faults, but the name and description to imply it's more of a switch for just those tests. I'm happy to keep CONFIG_KUNIT_FAULT_TEST as the generic implementation here, but if we really want another one, they both probably should chain off a single CONFIG_HAVE_KUNIT_FAULT_HANDLER or similar.On top of the reasons I gave above, I tried to follow the same as
`CONFIG_RUST_BUG_KUNIT_TEST` or `CONFIG_KUNIT_FAULT_TEST` are doing, but
in this case in particular, the config is not for enabling a test suite
but for enabling a feature that can affect multiple suites (in this
patch series the ones behind `CONFIG_RUST_KUNIT_TESTS` and
`CONFIG_RUST_BITMAP_KUNIT_TESTS`).
Yeah, it is a "mode" -- that is why originally I thought we would try
to just do the right thing automatically, i.e. enable them when
possible, but then when I saw the description of
`CONFIG_KUNIT_FAULT_TEST`:
Enables fault handling tests for the KUnit framework. These tests may
trigger a kernel BUG(), and the associated stack trace, even when they
pass. If this conflicts with your test infrastrcture (or is confusing
or annoying), they can be disabled by setting this to N.
and I thought it may be fair to have the Kconfig symbol, i.e. it could
also be annoying to see the traces or similar, and thus it may be that
someone needs to disable this.
Having said that, if nobody has asked, we could keep it simple for now
as well -- that may be fine too.
David: what do you think? Is it something that was discussed back then
for that one in C?
That being said, I'd prefer we go back to the panic_handler implementation for should_panic, as I think we're asking for trouble with Rust tests which deliberately panic in C code. Ideally, Rust tests will only trigger Rust panics deliberately. The panic_handler-based imeplementation works fine on UML.
Cheers,
-- David