* [PATCH RFC 1/3] rust: kunit: add #[should_panic] support
2026-09-15 19:33 [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support Nicolás Antinori
@ 2026-09-15 19:33 ` Nicolás Antinori
2026-09-15 19:33 ` [PATCH RFC 2/3] rust: kunit: allow same test name with different #[cfg(...)] Nicolás Antinori
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Nicolás Antinori @ 2026-09-15 19:33 UTC (permalink / raw)
To: Alice Ryhl, Burak Emir, Brendan Higgins, David Gow, Miguel Ojeda
Cc: Nicolás Antinori, Alexandre Courbot, Andreas Hindborg,
Benno Lossin, Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Gary Guo, Jori Koolstra,
Onur Özkan, Rae Moar, Shuah Khan, Tamir Duberstein,
Trevor Gross, Yury Norov, linux-kernel, rust-for-linux,
linux-kernel-mentees
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>
---
include/kunit/test.h | 1 +
include/kunit/try-catch.h | 1 +
lib/kunit/test.c | 14 ++++++++++++
lib/kunit/try-catch.c | 7 ++++++
rust/kernel/kunit.rs | 9 ++++++++
rust/kernel/lib.rs | 46 ++++++++++++++++++++++++++++++++++++---
rust/macros/kunit.rs | 21 ++++++++++++++++--
7 files changed, 94 insertions(+), 5 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index da5312e0dfa5..8b42f431e1c1 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -723,6 +723,7 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
#define KUNIT_SUCCEED(test) _KUNIT_SAVE_LOC(test)
void __noreturn __kunit_abort(struct kunit *test);
+void __noreturn __kunit_abort_expecting_error(struct kunit *test);
void __printf(6, 7) __kunit_do_failed_assertion(struct kunit *test,
const struct kunit_loc *loc,
diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
index d4e1a5b98ed6..a47b1cfbcf93 100644
--- a/include/kunit/try-catch.h
+++ b/include/kunit/try-catch.h
@@ -54,6 +54,7 @@ struct kunit_try_catch {
void kunit_try_catch_run(struct kunit_try_catch *try_catch, void *context);
void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch);
+void __noreturn kunit_try_catch_throw_expecting_error(struct kunit_try_catch *try_catch);
static inline int kunit_try_catch_get_result(struct kunit_try_catch *try_catch)
{
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 09e3dabfac0c..32e5419a2d52 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -323,6 +323,20 @@ void __noreturn __kunit_abort(struct kunit *test)
}
EXPORT_SYMBOL_GPL(__kunit_abort);
+void __noreturn __kunit_abort_expecting_error(struct kunit *test)
+{
+ kunit_try_catch_throw_expecting_error(&test->try_catch); /* Does not return. */
+
+ /*
+ * Throw could not abort from test.
+ *
+ * XXX: we should never reach this line! As kunit_try_catch_throw_expecting_error
+ * is marked __noreturn.
+ */
+ WARN_ONCE(true, "Throw could not abort from test!\n");
+}
+EXPORT_SYMBOL_GPL(__kunit_abort_expecting_error);
+
void __kunit_do_failed_assertion(struct kunit *test,
const struct kunit_loc *loc,
enum kunit_assert_type type,
diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
index d84a879f0a78..123e1f86a5b3 100644
--- a/lib/kunit/try-catch.c
+++ b/lib/kunit/try-catch.c
@@ -22,6 +22,13 @@ void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)
}
EXPORT_SYMBOL_GPL(kunit_try_catch_throw);
+void __noreturn kunit_try_catch_throw_expecting_error(struct kunit_try_catch *try_catch)
+{
+ try_catch->try_result = 0;
+ kthread_exit(0);
+}
+EXPORT_SYMBOL_GPL(kunit_try_catch_throw_expecting_error);
+
static int kunit_generic_run_threadfn_adapter(void *data)
{
struct kunit_try_catch *try_catch = data;
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 91eaff8c186a..65a1040ee2b0 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -9,6 +9,9 @@
use crate::fmt;
use crate::prelude::*;
+#[doc(hidden)]
+pub static KUNIT_SHOULD_PANIC: u32 = 0xDEAD7357;
+
/// Prints a KUnit error-level message.
///
/// Public but hidden since it should only be used from KUnit generated code.
@@ -345,6 +348,12 @@ fn rust_test_kunit_in_kunit_test() {
assert!(in_kunit_test());
}
+ #[test]
+ #[should_panic]
+ fn rust_test_kunit_panic_in_kunit_test() {
+ panic!("This test should panic and pass");
+ }
+
#[test]
#[cfg(not(all()))]
fn rust_test_kunit_always_disabled_test() {
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 4d5c96ddc49c..0f6c3c00ddd5 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -174,14 +174,54 @@ impl ModuleMetadata for LocalModule {
};
}
-#[cfg(not(testlib))]
-#[panic_handler]
-fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
+#[inline]
+fn bug_on_panic(info: &core::panic::PanicInfo<'_>) -> ! {
pr_emerg!("{}\n", info);
// SAFETY: FFI call.
unsafe { bindings::BUG() };
}
+#[cfg(all(not(testlib), not(CONFIG_KUNIT)))]
+#[panic_handler]
+fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
+ bug_on_panic(info);
+}
+
+#[cfg(all(not(testlib), CONFIG_KUNIT))]
+#[panic_handler]
+fn panic(info: &core::panic::PanicInfo<'_>) -> ! {
+ // SAFETY: This function is safe to call even if CONFIG_KUNIT=n. If a null pointer is returned,
+ // the panic is handled same as if CONFIG_KUNIT=n.
+ let kunit_test = unsafe { ::bindings::kunit_get_current_test() };
+ if kunit_test.is_null() {
+ bug_on_panic(info);
+ } else {
+ // SAFETY: We are in the else branch of kunit_test.is_null() condition, meaning that
+ // `::bindings::kunit_get_current_test()` returned a kunit struct successfully.
+ let should_panic_ptr: *const u32 = unsafe { (*kunit_test).priv_ as *const u32 };
+ let should_panic_code: u32 = if should_panic_ptr.is_null() {
+ bug_on_panic(info);
+ } else {
+ // SAFETY: Already tested that the should_panic_ptr pointer is not null, casting it to
+ // its value should be safe since kunit_test is not null and KUnit Rust tests are
+ // initialized by assigning either null or a u32 value to the priv_ field.
+ unsafe { *should_panic_ptr }
+ };
+
+ if should_panic_code == crate::kunit::KUNIT_SHOULD_PANIC {
+ // SAFETY: We are in the else branch of kunit_test.is_null() condition, meaning that
+ // `::bindings::kunit_get_current_test()` returned a kunit struct successfully.
+ unsafe {
+ (*kunit_test).status = ::kernel::bindings::kunit_status_KUNIT_SUCCESS;
+ bindings::__kunit_abort_expecting_error(kunit_test);
+ };
+ } else {
+ pr_emerg!("Invalid KUnit priv_ code 0x{:x}\n", should_panic_code);
+ bug_on_panic(info);
+ }
+ }
+}
+
/// Produces a pointer to an object from a pointer to one of its fields.
///
/// If you encounter a type mismatch due to the [`Opaque`] type, then use [`Opaque::cast_into`] or
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs
index ae20ed6768f1..2c6405cebc0a 100644
--- a/rust/macros/kunit.rs
+++ b/rust/macros/kunit.rs
@@ -106,6 +106,11 @@ pub(crate) fn kunit_tests(test_suite: Ident, mut module: ItemMod) -> Result<Toke
.cloned()
.collect();
+ let should_panic = f
+ .attrs
+ .iter()
+ .any(|attr| attr.path().is_ident("should_panic"));
+
// Before the test, override usual `assert!` and `assert_eq!` macros with ones that call
// KUnit instead.
let test_str = test.to_string();
@@ -135,6 +140,19 @@ macro_rules! assert_eq {
&CString::new(test_str.as_str()).expect("identifier cannot contain NUL"),
test.span(),
);
+ let assertion = if should_panic {
+ quote!(
+ (*_test).priv_ = &raw const crate::kunit::KUNIT_SHOULD_PANIC as *mut ffi::c_void;
+ let _ = #test();
+ (*_test).status = ::kernel::bindings::kunit_status_KUNIT_FAILURE;
+ )
+ } else {
+ quote!(
+ (*_test).priv_ = core::ptr::null_mut();
+ use ::kernel::kunit::is_test_result_ok;
+ assert!(is_test_result_ok(#test()));
+ )
+ };
processed_items.push(parse_quote! {
unsafe extern "C" fn #kunit_wrapper_fn_name(_test: *mut ::kernel::bindings::kunit) {
(*_test).status = ::kernel::bindings::kunit_status_KUNIT_SKIPPED;
@@ -145,8 +163,7 @@ macro_rules! assert_eq {
#(#cfg_attrs)*
{
(*_test).status = ::kernel::bindings::kunit_status_KUNIT_SUCCESS;
- use ::kernel::kunit::is_test_result_ok;
- assert!(is_test_result_ok(#test()));
+ #assertion
}
}
});
--
2.47.3
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH RFC 2/3] rust: kunit: allow same test name with different #[cfg(...)]
2026-09-15 19:33 [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support Nicolás Antinori
2026-09-15 19:33 ` [PATCH RFC 1/3] rust: kunit: add #[should_panic] support Nicolás Antinori
@ 2026-09-15 19:33 ` Nicolás Antinori
2026-09-15 19:33 ` [PATCH RFC 3/3] rust: bitmap: kunit: uncomment owned_bitmap_out_of_bounds panic case Nicolás Antinori
2026-09-22 7:56 ` [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support David Gow
3 siblings, 0 replies; 10+ messages in thread
From: Nicolás Antinori @ 2026-09-15 19:33 UTC (permalink / raw)
To: Alice Ryhl, Burak Emir, Brendan Higgins, David Gow, Miguel Ojeda
Cc: Nicolás Antinori, Alexandre Courbot, Andreas Hindborg,
Benno Lossin, Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Gary Guo, Jori Koolstra,
Onur Özkan, Rae Moar, Shuah Khan, Tamir Duberstein,
Trevor Gross, Yury Norov, linux-kernel, rust-for-linux,
linux-kernel-mentees
Sometimes it is necessary to test the same code paths under different
configurations. The `#[cfg(...)]` macro can be used to check if specific
configurations are enabled and run tests accordingly.
Currently, defining multiple tests with the same name under different
`#[cfg(...)]` attributes results in a compilation error. This patch removes
that restriction, allowing identical test names across different
configurations. Additionally, it appends the active configuration to the
test name, ensuring the runner clearly indicates which test executed and
which was skipped.
Signed-off-by: Nicolás Antinori <nico.antinori.7@gmail.com>
---
rust/kernel/kunit.rs | 15 ++++++++++++
rust/macros/kunit.rs | 57 +++++++++++++++++++++++++++++++++++++++++---
2 files changed, 69 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 65a1040ee2b0..613c8d2aea78 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -348,6 +348,21 @@ fn rust_test_kunit_in_kunit_test() {
assert!(in_kunit_test());
}
+ // Both tests with cfg have the same name on purpose because we are implicitly testing that
+ // tests with the same name but different configs do not throw a compilation error
+ #[test]
+ #[cfg(CONFIG_RUST_KUNIT_SELFTEST = "y")]
+ fn rust_test_kunit_parse_cfg_in_kunit_test() {
+ assert!(in_kunit_test());
+ }
+
+ #[test]
+ #[cfg(CONFIG_RUST_KUNIT_SELFTEST = "n")]
+ fn rust_test_kunit_parse_cfg_in_kunit_test() {
+ // This test should never run because of the `cfg`.
+ assert!(false)
+ }
+
#[test]
#[should_panic]
fn rust_test_kunit_panic_in_kunit_test() {
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs
index 2c6405cebc0a..605d925cd8f0 100644
--- a/rust/macros/kunit.rs
+++ b/rust/macros/kunit.rs
@@ -6,14 +6,19 @@
use std::ffi::CString;
-use proc_macro2::TokenStream;
+use proc_macro2::{
+ TokenStream,
+ TokenTree, //
+};
use quote::{
format_ident,
quote,
ToTokens, //
};
use syn::{
+ parse::ParseStream,
parse_quote,
+ Attribute,
Error,
Ident,
Item,
@@ -22,6 +27,46 @@
Result, //
};
+fn get_cfg_string(attr: &Attribute) -> Result<String> {
+ let mut result = String::from("_cfg");
+ attr.parse_args_with(|input: ParseStream<'_>| {
+ while !input.is_empty() {
+ build_cfg_string(input.parse()?, &mut result)?;
+ }
+ Ok(result)
+ })
+}
+
+fn build_cfg_string(tt: TokenTree, result: &mut String) -> Result<()> {
+ match tt {
+ TokenTree::Ident(ident) => {
+ result.push('_');
+ result.push_str(&ident.to_string().to_lowercase());
+ }
+ TokenTree::Punct(ref punct) => match punct.as_char() {
+ '=' => {
+ result.push_str("_equals");
+ }
+ _ => {
+ return Err(Error::new_spanned(
+ punct,
+ "only \"=\" is allowed to check configurations",
+ ))
+ }
+ },
+ TokenTree::Literal(lit) => {
+ result.push('_');
+ result.push_str(&lit.to_string().trim_matches('"').to_string());
+ }
+ TokenTree::Group(group) => {
+ for group_tt in group.stream() {
+ build_cfg_string(group_tt, result)?;
+ }
+ }
+ }
+ Ok(())
+}
+
pub(crate) fn kunit_tests(test_suite: Ident, mut module: ItemMod) -> Result<TokenStream> {
if test_suite.to_string().len() > 255 {
return Err(Error::new_spanned(
@@ -106,6 +151,12 @@ pub(crate) fn kunit_tests(test_suite: Ident, mut module: ItemMod) -> Result<Toke
.cloned()
.collect();
+ let cfg_attrs_str = cfg_attrs
+ .iter()
+ .map(get_cfg_string)
+ .collect::<Result<Vec<String>>>()?
+ .join("__");
+
let should_panic = f
.attrs
.iter()
@@ -113,7 +164,7 @@ pub(crate) fn kunit_tests(test_suite: Ident, mut module: ItemMod) -> Result<Toke
// Before the test, override usual `assert!` and `assert_eq!` macros with ones that call
// KUnit instead.
- let test_str = test.to_string();
+ let test_str = format!("{test}{cfg_attrs_str}");
let path = CString::new(crate::helpers::file()).expect("file path cannot contain NUL");
processed_items.push(parse_quote! {
#[allow(unused)]
@@ -135,7 +186,7 @@ macro_rules! assert_eq {
// Add back the test item.
processed_items.push(Item::Fn(f));
- let kunit_wrapper_fn_name = format_ident!("kunit_rust_wrapper_{test}");
+ let kunit_wrapper_fn_name = format_ident!("kunit_rust_wrapper_{test}{cfg_attrs_str}");
let test_cstr = LitCStr::new(
&CString::new(test_str.as_str()).expect("identifier cannot contain NUL"),
test.span(),
--
2.47.3
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH RFC 3/3] rust: bitmap: kunit: uncomment owned_bitmap_out_of_bounds panic case
2026-09-15 19:33 [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support Nicolás Antinori
2026-09-15 19:33 ` [PATCH RFC 1/3] rust: kunit: add #[should_panic] support Nicolás Antinori
2026-09-15 19:33 ` [PATCH RFC 2/3] rust: kunit: allow same test name with different #[cfg(...)] Nicolás Antinori
@ 2026-09-15 19:33 ` Nicolás Antinori
2026-09-22 7:56 ` [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support David Gow
3 siblings, 0 replies; 10+ messages in thread
From: Nicolás Antinori @ 2026-09-15 19:33 UTC (permalink / raw)
To: Alice Ryhl, Burak Emir, Brendan Higgins, David Gow, Miguel Ojeda
Cc: Nicolás Antinori, Alexandre Courbot, Andreas Hindborg,
Benno Lossin, Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Gary Guo, Jori Koolstra,
Onur Özkan, Rae Moar, Shuah Khan, Tamir Duberstein,
Trevor Gross, Yury Norov, linux-kernel, rust-for-linux,
linux-kernel-mentees
Rust KUnit tests now support `#[should_panic]` attribute. Uncomment the
test so it can be run when CONFIG_RUST_BITMAP_HARDENED=y.
Signed-off-by: Nicolás Antinori <nico.antinori.7@gmail.com>
---
rust/kernel/bitmap.rs | 40 ++++++++++++++++++----------------------
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/rust/kernel/bitmap.rs b/rust/kernel/bitmap.rs
index b27e0ec80d64..a9935b2be8c5 100644
--- a/rust/kernel/bitmap.rs
+++ b/rust/kernel/bitmap.rs
@@ -571,33 +571,29 @@ fn bitmap_set_clear_find() -> Result<(), AllocError> {
Ok(())
}
+ #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
#[test]
fn owned_bitmap_out_of_bounds() -> Result<(), AllocError> {
- // TODO: Kunit #[test]s do not support `cfg` yet,
- // so we add it here in the body.
- #[cfg(not(CONFIG_RUST_BITMAP_HARDENED))]
- {
- let mut b = BitmapVec::new(128, GFP_KERNEL)?;
- b.set_bit(2048);
- b.set_bit_atomic(2048);
- b.clear_bit(2048);
- b.clear_bit_atomic(2048);
- assert_eq!(None, b.next_bit(2048));
- assert_eq!(None, b.next_zero_bit(2048));
- assert_eq!(None, b.last_bit());
- }
+ let mut b = BitmapVec::new(128, GFP_KERNEL)?;
+ b.set_bit(2048);
+ b.set_bit_atomic(2048);
+ b.clear_bit(2048);
+ b.clear_bit_atomic(2048);
+ assert_eq!(None, b.next_bit(2048));
+ assert_eq!(None, b.next_zero_bit(2048));
+ assert_eq!(None, b.last_bit());
Ok(())
}
- // TODO: uncomment once kunit supports [should_panic] and `cfg`.
- // #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
- // #[test]
- // #[should_panic]
- // fn owned_bitmap_out_of_bounds() -> Result<(), AllocError> {
- // let mut b = BitmapVec::new(128, GFP_KERNEL)?;
- //
- // b.set_bit(2048);
- // }
+ #[cfg(CONFIG_RUST_BITMAP_HARDENED)]
+ #[test]
+ #[should_panic]
+ fn owned_bitmap_out_of_bounds() -> Result<(), AllocError> {
+ let mut b = BitmapVec::new(128, GFP_KERNEL)?;
+
+ b.set_bit(2048);
+ Ok(())
+ }
#[test]
fn bitmap_copy_and_extend() -> Result<(), AllocError> {
--
2.47.3
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support
2026-09-15 19:33 [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support Nicolás Antinori
` (2 preceding siblings ...)
2026-09-15 19:33 ` [PATCH RFC 3/3] rust: bitmap: kunit: uncomment owned_bitmap_out_of_bounds panic case Nicolás Antinori
@ 2026-09-22 7:56 ` David Gow
2026-09-22 13:35 ` Gary Guo
2026-09-23 15:04 ` Nicolás Antinori
3 siblings, 2 replies; 10+ messages in thread
From: David Gow @ 2026-09-22 7:56 UTC (permalink / raw)
To: Nicolás Antinori, Alice Ryhl, Burak Emir, Brendan Higgins,
Miguel Ojeda
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Gary Guo, Jori Koolstra,
Onur Özkan, Rae Moar, Shuah Khan, Tamir Duberstein,
Trevor Gross, Yury Norov, linux-kernel, rust-for-linux,
linux-kernel-mentees
Le 16/09/2026 à 03:33, Nicolás Antinori 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].
>
Thanks very much for this series! It works fine here, but I think there
are a few other options for how this could be implemented, and it's
probably worth our at least considering them.
In particular, we've already got code for suppressing warnings, and I'm
not sure whether it makes sense to unify all of the different attempts
to intercept panics / bugs / warnings of various kinds.
That being said, Rust has unwinding and panic handlers as a core part of
the language, and the C side of the kernel doesn't. Combine that with
the fact that #[should_panic] is already standardised in Rust, and the
argument for a separate implementation is not totally silly either.
Do you think that #[should_panic] should only trigger on a rust
panic!(), or on any kernel panic? I'm leaning towards the former, but if
the latter then we'd need to implement it in C and provide a C interface
to it.
> 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, and I don't know if it makes sense to
> support it)
>
> The way it is supported is by having a separate `#[panic_handler]` when
> `CONFIG_KUNIT` is enabled. When a test is marked with `#[should_panic]`,
> a static value (KUNIT_SHOULD_PANIC = 0xDEAD7357) is assigned to the
> kunit's `priv` field, since it is meant for saving arbitrary user data
> [3]. At the moment, I did not find any place where Rust tests use that
> field, so it should be safe to write it.
I don't think the `priv` field is the optimal place to put this. I don't
think it's strictly a _problem_, particularly since Rust tests aren't
using it, but nominally `priv` is for test use, and I'd rather not use
it here (there may be future tests which want to use priv for something
else, particularly as a quick way of passing test state between C and Rust).
For most of these sorts of things, I'd recommend using a KUnit 'named
resource', but alas, there aren't any Rust binding for these. That being
said, we've used named resources in C because they're setup at runtime
(which is how we've handled this in the past). That's useful if we want
to note that a particular line in the test wants to panic, but if we're
only concerned with whether a test as a whole panics, then this could be
static.
In that case, how about adding a new `rust_should_panic` field to
`struct kunit_attributes`. If you only care about whether a panic
occurs, this could just be a boolean, but it also could be a place to
store, for example, a string to support #[should_panic = "message"] if
you wish.
As an attribute, you could also then add it to lib/kunit/attributes.c
(probably with PRINT_NEVER, as I don't think we need it included in KTAP
output), which would, for example, allow us to filter tests by whether
or not they expect to panic.
>
> When the test panics, the `#[panic_handler]` function is called, obtains
> the kunit current test and checks if the `priv` field is not null and
> contains the value `KUNIT_SHOULD_PANIC`.
>
> If those conditions are true, it marks the test as successful (since it
> panicked as expected) and calls `__kunit_abort_expecting_error`, a new
> function that exits the testing thread but fills `try_catch->try_result`
> with a 0 so the test runner does not mistake it as a failed test.
>
> If those conditions are not true:
> - If `priv` is null, the test panics as it would have before having
> this feature, priv = null means that the test was not expected to
> panic.
> - If `priv` is not null but its value is not `KUNIT_SHOULD_PANIC`, the
> test panics with an error message informing that the code found in
> `priv` was invalid.
>
> If the test does not panic, the `#[panic_handler]` is not triggered. The
> test is marked as failed (since it was expected to panic).
>
> Regarding this feature:
> - Do this approach make sense?
Yes, I think this approach makes sense. While I think a less
rust-specific way of trapping panics could be useful (à la the
suppressed warning system), I am erring on the side of implementing it
this way given it (a) doesn't involve
> - Is it ok to mark the `#[should_panic]` tests with a static constant?
> Is another mechanism better to check in the `#[panic_handler]` that
> the test was supposed to panic?
I think that we do want to base this off the struct kunit, though a
special constant in 'priv' is not optimal. I'd go with either a named
kunit resource (alas, which don't have Rust bindings) if we'd want to
support extending this to specify a specific line / block panicking; or
a new field in struct kunit_attributes.
> 2. Allow same test name with different #[cfg(...)]:
>
> When testing `#[should_panic]` in `bitmap.rs` (check the last patch of
> the series) I found that the test that was supposed to panic had the
> same name as another one, but they were run on different configurations.
> This caused the following compilation error:
>
> ERROR:root:error[E0428]: the name `kunit_rust_wrapper_owned_bitmap_out_of_bounds` is defined multiple times
> --> ../rust/kernel/bitmap.rs:503:1
> |
> 503 | #[macros::kunit_tests(rust_kernel_bitmap)]
> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `kunit_rust_wrapper_owned_bitmap_out_of_bounds` redefined here
> |
> = note: `kunit_rust_wrapper_owned_bitmap_out_of_bounds` must be defined only once in the value namespace of this module
> = note: this error originates in the attribute macro `macros::kunit_tests` (in Nightly builds, run with -Z macro-backtrace for more info)
>
>
> To fix this problem, I appended to the `kunit_rust_wrapper_*`
> identifiers an 'stringified' version of the test's #[cfg(...)]
> arguments. The purpose of this is that, if we have a test with the same
> name and configuration, it would fail.
>
> The configuration string was also appended to the tests names. This was
> done to have a better test run report:
>
> ...
> [SKIPPED] owned_bitmap_out_of_bounds_cfg_not_config_rust_bitmap_hardened
> [PASSED] owned_bitmap_out_of_bounds_cfg_config_rust_bitmap_hardened
> ...
>
> Otherwise we would have something like the following:
> ...
> [SKIPPED] owned_bitmap_out_of_bounds
> [PASSED] owned_bitmap_out_of_bounds
> ...
>
> Regarding this:
> - Does it makes sense to allow same test names with different cfgs?
Yes-ish. I think it definitely makes sense for the same test to be
redefined with different cfgs, but I'd rather only one of those tests
then actually be compiled in (see below).
> - Is it ok to 'stringify' the configuration so it can be distinguished
> in the report? Would you prefer something like `_case_1` `_case_2` ..
> instead?
I don't _like_ this: my preference would be for us to keep the same
name, and just not emit a test_case for anything which should be
compiled out with cfg. Unfortunately, implementing that is a bit harder
than would be ideal: we need a way of evaluating the cfg() arguments in
a proc macro, I think. (Ultimately, because otherwise there's no way of
statically determining the length of the TEST_CASES array?)
Unless you've got a good idea how to fix this, though, I'm happy to put
up with adding the configs to the name for now. Though if there's a nice
way to make the names shorter
(rust_test_kunit_parse_cfg_in_kunit_test_cfg_config_rust_kunit_selftest_equals_n
is definitely too long a test name, for instance), that'd be best.
> This is the first RFC patch I send to the LKML, if there's something not
> right with it please let me know.
>
> Kind Regards,
> Nicolás
>
> [1] https://github.com/Rust-for-Linux/linux/blob/fd73f4a6659897191fa0d40695fe370925dd3780/rust/kernel/bitmap.rs#L592-L600
> [2] https://doc.rust-lang.org/rust-by-example/testing/unit_testing.html#testing-panics
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/kunit/test.h?id=f6e7b42bf05b2427fb8a7a1d1c387a86638bb413#n314
>
> Nicolás Antinori (3):
> rust: kunit: add #[should_panic] support
> rust: kunit: allow same test name with different #[cfg(...)]
> rust: bitmap: kunit: uncomment owned_bitmap_out_of_bounds panic case
>
> include/kunit/test.h | 1 +
> include/kunit/try-catch.h | 1 +
> lib/kunit/test.c | 14 +++++++
> lib/kunit/try-catch.c | 7 ++++
> rust/kernel/bitmap.rs | 40 +++++++++-----------
> rust/kernel/kunit.rs | 24 ++++++++++++
> rust/kernel/lib.rs | 46 +++++++++++++++++++++--
> rust/macros/kunit.rs | 78 ++++++++++++++++++++++++++++++++++++---
> 8 files changed, 181 insertions(+), 30 deletions(-)
>
> --
> 2.47.3
>
Cheers,
-- David
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support
2026-09-22 7:56 ` [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support David Gow
@ 2026-09-22 13:35 ` Gary Guo
2026-09-22 15:27 ` David Gow
2026-09-23 15:04 ` Nicolás Antinori
1 sibling, 1 reply; 10+ messages in thread
From: Gary Guo @ 2026-09-22 13:35 UTC (permalink / raw)
To: David Gow, Nicolás Antinori, Alice Ryhl, Burak Emir,
Brendan Higgins, Miguel Ojeda
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Gary Guo, Jori Koolstra,
Onur Özkan, Rae Moar, Shuah Khan, Tamir Duberstein,
Trevor Gross, Yury Norov, linux-kernel, rust-for-linux,
linux-kernel-mentees
On Tue Sep 22, 2026 at 8:56 AM BST, David Gow wrote:
> Le 16/09/2026 à 03:33, Nicolás Antinori a écrit :
>> - Is it ok to 'stringify' the configuration so it can be distinguished
>> in the report? Would you prefer something like `_case_1` `_case_2` ..
>> instead?
I also don't like the stringifcation of cfgs.
>
> I don't _like_ this: my preference would be for us to keep the same
> name, and just not emit a test_case for anything which should be
> compiled out with cfg. Unfortunately, implementing that is a bit harder
> than would be ideal: we need a way of evaluating the cfg() arguments in
> a proc macro, I think. (Ultimately, because otherwise there's no way of
> statically determining the length of the TEST_CASES array?)
This is possible with a trick. In pin-init we have a similar need, so what I do
is for
#[macro]
struct Foo {
#[cfg(a)]
bar: u32,
}
to be expanded to
#[cfg(a)]
#[macro]
struct Foo {
bar: u32
}
#[cfg(not(a))]
#[macro]
struct Foo {
}
However, for kunit I don't think that's needed. Deduplicating the names
should be sufficient?
Best,
Gary
>
> Unless you've got a good idea how to fix this, though, I'm happy to put
> up with adding the configs to the name for now. Though if there's a nice
> way to make the names shorter
> (rust_test_kunit_parse_cfg_in_kunit_test_cfg_config_rust_kunit_selftest_equals_n
> is definitely too long a test name, for instance), that'd be best.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support
2026-09-22 13:35 ` Gary Guo
@ 2026-09-22 15:27 ` David Gow
2026-09-22 15:37 ` Gary Guo
0 siblings, 1 reply; 10+ messages in thread
From: David Gow @ 2026-09-22 15:27 UTC (permalink / raw)
To: Gary Guo, Nicolás Antinori, Alice Ryhl, Burak Emir,
Brendan Higgins, Miguel Ojeda
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Jori Koolstra, Onur Özkan,
Rae Moar, Shuah Khan, Tamir Duberstein, Trevor Gross, Yury Norov,
linux-kernel, rust-for-linux, linux-kernel-mentees
Le 22/09/2026 à 21:35, Gary Guo a écrit :
> On Tue Sep 22, 2026 at 8:56 AM BST, David Gow wrote:
>> Le 16/09/2026 à 03:33, Nicolás Antinori a écrit :
>>> - Is it ok to 'stringify' the configuration so it can be distinguished
>>> in the report? Would you prefer something like `_case_1` `_case_2` ..
>>> instead?
>
> I also don't like the stringifcation of cfgs.
>
>>
>> I don't _like_ this: my preference would be for us to keep the same
>> name, and just not emit a test_case for anything which should be
>> compiled out with cfg. Unfortunately, implementing that is a bit harder
>> than would be ideal: we need a way of evaluating the cfg() arguments in
>> a proc macro, I think. (Ultimately, because otherwise there's no way of
>> statically determining the length of the TEST_CASES array?)
>
> This is possible with a trick. In pin-init we have a similar need, so what I do
> is for
>
> #[macro]
> struct Foo {
> #[cfg(a)]
> bar: u32,
> }
>
> to be expanded to
>
> #[cfg(a)]
> #[macro]
> struct Foo {
> bar: u32
> }
>
> #[cfg(not(a))]
> #[macro]
> struct Foo {
> }
>
> However, for kunit I don't think that's needed. Deduplicating the names
> should be sufficient?
>
The problem with (at least my naive implementation of) duplication is
that -- while it works great for switching between implementations -- it
doesn't handle the case where _no_ implementation is active.
(The current implementation just compiles to a skipped test if the
#[cfg(...)] isn't active, which sidesteps the problem until we have
multiple implementations...)
Even the expansion above could be problematic, as we really are trying
to add entries to a static array, so I don't know what we could put in
the not(a) case (particularly since there'd be potentially lots of them).
Maybe the trick is to generate the array size by using a big series of
something like:
static mut TEST_CASES: [...,
#[cfg(a)] 1
#[cfg(not(a))]0
+
#[cfg(b)] 1
#[cfg(not(b))]0
+
…] = {
#[cfg(a)] case1,
#[cfg(b)] case1,
…
}
}
Then, as long as there's only one or zero active configurations, the
array size should match, and any duplicates will be caught by having
multiple definitions of case1.
I assume the compiler would be able to reduce that down to a
compile-time integer, even if it is extremely ugly...
-- David
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support
2026-09-22 15:27 ` David Gow
@ 2026-09-22 15:37 ` Gary Guo
2026-09-22 15:53 ` David Gow
0 siblings, 1 reply; 10+ messages in thread
From: Gary Guo @ 2026-09-22 15:37 UTC (permalink / raw)
To: David Gow, Gary Guo, Nicolás Antinori, Alice Ryhl,
Burak Emir, Brendan Higgins, Miguel Ojeda
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Jori Koolstra, Onur Özkan,
Rae Moar, Shuah Khan, Tamir Duberstein, Trevor Gross, Yury Norov,
linux-kernel, rust-for-linux, linux-kernel-mentees
On Tue Sep 22, 2026 at 4:27 PM BST, David Gow wrote:
> Le 22/09/2026 à 21:35, Gary Guo a écrit :
>> On Tue Sep 22, 2026 at 8:56 AM BST, David Gow wrote:
>>> Le 16/09/2026 à 03:33, Nicolás Antinori a écrit :
>>>> - Is it ok to 'stringify' the configuration so it can be distinguished
>>>> in the report? Would you prefer something like `_case_1` `_case_2` ..
>>>> instead?
>>
>> I also don't like the stringifcation of cfgs.
>>
>>>
>>> I don't _like_ this: my preference would be for us to keep the same
>>> name, and just not emit a test_case for anything which should be
>>> compiled out with cfg. Unfortunately, implementing that is a bit harder
>>> than would be ideal: we need a way of evaluating the cfg() arguments in
>>> a proc macro, I think. (Ultimately, because otherwise there's no way of
>>> statically determining the length of the TEST_CASES array?)
>>
>> This is possible with a trick. In pin-init we have a similar need, so what I do
>> is for
>>
>> #[macro]
>> struct Foo {
>> #[cfg(a)]
>> bar: u32,
>> }
>>
>> to be expanded to
>>
>> #[cfg(a)]
>> #[macro]
>> struct Foo {
>> bar: u32
>> }
>>
>> #[cfg(not(a))]
>> #[macro]
>> struct Foo {
>> }
>>
>> However, for kunit I don't think that's needed. Deduplicating the names
>> should be sufficient?
>>
> The problem with (at least my naive implementation of) duplication is
> that -- while it works great for switching between implementations -- it
> doesn't handle the case where _no_ implementation is active.
>
> (The current implementation just compiles to a skipped test if the
> #[cfg(...)] isn't active, which sidesteps the problem until we have
> multiple implementations...)
>
> Even the expansion above could be problematic, as we really are trying
> to add entries to a static array, so I don't know what we could put in
> the not(a) case (particularly since there'd be potentially lots of them).
>
> Maybe the trick is to generate the array size by using a big series of
> something like:
> static mut TEST_CASES: [...,
> #[cfg(a)] 1
> #[cfg(not(a))]0
> +
> #[cfg(b)] 1
> #[cfg(not(b))]0
> +
> …] = {
> #[cfg(a)] case1,
> #[cfg(b)] case1,
> …
> }
> }
>
For array sizes, you have the option of building a slice first.
Some thing like:
const TEST_CASES_UNIT: &[()] = [
#[cfg(a)] (),
#[cfg(b)] (),
];
static mut TEST_CASES: [...; TEST_CASES_SLICE.len()] = [...];
You could also just build everything as a const slice of `&'static
[kunit_cases]`, if there is no need to make it `mut`. But I suppose it needs to
be `static mut` for some reason?
Best,
Gary
> Then, as long as there's only one or zero active configurations, the
> array size should match, and any duplicates will be caught by having
> multiple definitions of case1.
>
> I assume the compiler would be able to reduce that down to a
> compile-time integer, even if it is extremely ugly...
>
> -- David
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support
2026-09-22 15:37 ` Gary Guo
@ 2026-09-22 15:53 ` David Gow
0 siblings, 0 replies; 10+ messages in thread
From: David Gow @ 2026-09-22 15:53 UTC (permalink / raw)
To: Gary Guo, Nicolás Antinori, Alice Ryhl, Burak Emir,
Brendan Higgins, Miguel Ojeda
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Jori Koolstra, Onur Özkan,
Rae Moar, Shuah Khan, Tamir Duberstein, Trevor Gross, Yury Norov,
linux-kernel, rust-for-linux, linux-kernel-mentees
Le 22/09/2026 à 23:37, Gary Guo a écrit :
> On Tue Sep 22, 2026 at 4:27 PM BST, David Gow wrote:
>> Le 22/09/2026 à 21:35, Gary Guo a écrit :
>>> On Tue Sep 22, 2026 at 8:56 AM BST, David Gow wrote:
>>>> Le 16/09/2026 à 03:33, Nicolás Antinori a écrit :
>>>>> - Is it ok to 'stringify' the configuration so it can be distinguished
>>>>> in the report? Would you prefer something like `_case_1` `_case_2` ..
>>>>> instead?
>>>
>>> I also don't like the stringifcation of cfgs.
>>>
>>>>
>>>> I don't _like_ this: my preference would be for us to keep the same
>>>> name, and just not emit a test_case for anything which should be
>>>> compiled out with cfg. Unfortunately, implementing that is a bit harder
>>>> than would be ideal: we need a way of evaluating the cfg() arguments in
>>>> a proc macro, I think. (Ultimately, because otherwise there's no way of
>>>> statically determining the length of the TEST_CASES array?)
>>>
>>> This is possible with a trick. In pin-init we have a similar need, so what I do
>>> is for
>>>
>>> #[macro]
>>> struct Foo {
>>> #[cfg(a)]
>>> bar: u32,
>>> }
>>>
>>> to be expanded to
>>>
>>> #[cfg(a)]
>>> #[macro]
>>> struct Foo {
>>> bar: u32
>>> }
>>>
>>> #[cfg(not(a))]
>>> #[macro]
>>> struct Foo {
>>> }
>>>
>>> However, for kunit I don't think that's needed. Deduplicating the names
>>> should be sufficient?
>>>
>> The problem with (at least my naive implementation of) duplication is
>> that -- while it works great for switching between implementations -- it
>> doesn't handle the case where _no_ implementation is active.
>>
>> (The current implementation just compiles to a skipped test if the
>> #[cfg(...)] isn't active, which sidesteps the problem until we have
>> multiple implementations...)
>>
>> Even the expansion above could be problematic, as we really are trying
>> to add entries to a static array, so I don't know what we could put in
>> the not(a) case (particularly since there'd be potentially lots of them).
>>
>> Maybe the trick is to generate the array size by using a big series of
>> something like:
>> static mut TEST_CASES: [...,
>> #[cfg(a)] 1
>> #[cfg(not(a))]0
>> +
>> #[cfg(b)] 1
>> #[cfg(not(b))]0
>> +
>> …] = {
>> #[cfg(a)] case1,
>> #[cfg(b)] case1,
>> …
>> }
>> }
>>
>
> For array sizes, you have the option of building a slice first.
>
> Some thing like:
>
> const TEST_CASES_UNIT: &[()] = [
> #[cfg(a)] (),
> #[cfg(b)] (),
> ];
>
> static mut TEST_CASES: [...; TEST_CASES_SLICE.len()] = [...];
>
Neat: I hadn't thought of that, and it seems to work great.
> You could also just build everything as a const slice of `&'static
> [kunit_cases]`, if there is no need to make it `mut`. But I suppose it needs to
> be `static mut` for some reason?
>
Yeah, the kunit_cases are modified at runtime to store the result, so
this really does need to be `static mut`. And while these writes are all
done behind the scenes from C, so they should _appear_ constant from
Rust (modulo a couple of writes to status we can get rid of once we fix
the cfg() stuff here), we still need to ensure they can't end up in
read-only memory.
Cheers,
-- David
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support
2026-09-22 7:56 ` [PATCH RFC 0/3] rust: kunit: #[should_panic] and same test name with different #[cfg(...)] support David Gow
2026-09-22 13:35 ` Gary Guo
@ 2026-09-23 15:04 ` Nicolás Antinori
1 sibling, 0 replies; 10+ messages in thread
From: Nicolás Antinori @ 2026-09-23 15:04 UTC (permalink / raw)
To: David Gow, Alice Ryhl, Burak Emir, Brendan Higgins, Miguel Ojeda,
Gary Guo
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Brigham Campbell,
Daniel Almeida, Danilo Krummrich, Gary Guo, Jori Koolstra,
Onur Özkan, Rae Moar, Shuah Khan, Tamir Duberstein,
Trevor Gross, Yury Norov, linux-kernel, rust-for-linux,
linux-kernel-mentees
Thank you for the feedback!
On Tue Sep 22, 2026 at 4:56 AM -03, David Gow wrote:
> Le 16/09/2026 à 03:33, Nicolás Antinori 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].
>>
>
> Thanks very much for this series! It works fine here, but I think there
> are a few other options for how this could be implemented, and it's
> probably worth our at least considering them.
>
> In particular, we've already got code for suppressing warnings, and I'm
> not sure whether it makes sense to unify all of the different attempts
> to intercept panics / bugs / warnings of various kinds.
>
> That being said, Rust has unwinding and panic handlers as a core part of
> the language, and the C side of the kernel doesn't. Combine that with
> the fact that #[should_panic] is already standardised in Rust, and the
> argument for a separate implementation is not totally silly either.
>
> Do you think that #[should_panic] should only trigger on a rust
> panic!(), or on any kernel panic? I'm leaning towards the former, but if
> the latter then we'd need to implement it in C and provide a C interface
> to it.
When I sent the series I leaned towards the former too. But thinking
about it I believe there are situations where a kernel panic can be
originated from C code called by Rust, for example, this test case:
#[test]
#[should_panic]
fn rust_test_kunit_panic_in_kunit_test_bug() {
unsafe { bindings::BUG() };
}
This kernel panic is not caught by the Rust's panic handler. In the
current version of my code, I catch that in
lib/kunit/test.c::kunit_run_case_catch_errors function. With that
modification there's no need of a Rust side panic handler (as it catches
Rust's panics too, since the panic hanlder executes a bindings::BUG()).
>
>> 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, and I don't know if it makes sense to
>> support it)
>>
>> The way it is supported is by having a separate `#[panic_handler]` when
>> `CONFIG_KUNIT` is enabled. When a test is marked with `#[should_panic]`,
>> a static value (KUNIT_SHOULD_PANIC = 0xDEAD7357) is assigned to the
>> kunit's `priv` field, since it is meant for saving arbitrary user data
>> [3]. At the moment, I did not find any place where Rust tests use that
>> field, so it should be safe to write it.
>
> I don't think the `priv` field is the optimal place to put this. I don't
> think it's strictly a _problem_, particularly since Rust tests aren't
> using it, but nominally `priv` is for test use, and I'd rather not use
> it here (there may be future tests which want to use priv for something
> else, particularly as a quick way of passing test state between C and Rust).
>
> For most of these sorts of things, I'd recommend using a KUnit 'named
> resource', but alas, there aren't any Rust binding for these. That being
> said, we've used named resources in C because they're setup at runtime
> (which is how we've handled this in the past). That's useful if we want
> to note that a particular line in the test wants to panic, but if we're
> only concerned with whether a test as a whole panics, then this could be
> static.
I did not know that you could test particular lines for panics! That
said, I believe the #[should_panic] attribute is meant to check if the
test panics as a whole.
If I had to test a particular line for panic that I'd write a new test,
but that's just how I'd do it :P.
>
> In that case, how about adding a new `rust_should_panic` field to
> `struct kunit_attributes`. If you only care about whether a panic
> occurs, this could just be a boolean, but it also could be a place to
> store, for example, a string to support #[should_panic = "message"] if
> you wish.
I could not find a way to retrieve the kunit_case struct from Rust. I
believe this is needed for implementing the check because the actual
panic message can only be retrieved from the PanicInfo [1] struct.
I am sure this can be implemented (the first things that comes to mind
is having a C api that retrieves the current kunit_case struct, but I am
not sure if the kunit_case meant to be leaked outside the runner) but
I'd do it in another iteration if we find that it is useful.
>
> As an attribute, you could also then add it to lib/kunit/attributes.c
> (probably with PRINT_NEVER, as I don't think we need it included in KTAP
> output), which would, for example, allow us to filter tests by whether
> or not they expect to panic.
Excellent! I'll do that!
>> ...
>> 2. Allow same test name with different #[cfg(...)]:
>>
>> When testing `#[should_panic]` in `bitmap.rs` (check the last patch of
>> the series) I found that the test that was supposed to panic had the
>> same name as another one, but they were run on different configurations.
>> This caused the following compilation error:
>>
>> ERROR:root:error[E0428]: the name `kunit_rust_wrapper_owned_bitmap_out_of_bounds` is defined multiple times
>> --> ../rust/kernel/bitmap.rs:503:1
>> |
>> 503 | #[macros::kunit_tests(rust_kernel_bitmap)]
>> | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `kunit_rust_wrapper_owned_bitmap_out_of_bounds` redefined here
>> |
>> = note: `kunit_rust_wrapper_owned_bitmap_out_of_bounds` must be defined only once in the value namespace of this module
>> = note: this error originates in the attribute macro `macros::kunit_tests` (in Nightly builds, run with -Z macro-backtrace for more info)
>>
>>
>> To fix this problem, I appended to the `kunit_rust_wrapper_*`
>> identifiers an 'stringified' version of the test's #[cfg(...)]
>> arguments. The purpose of this is that, if we have a test with the same
>> name and configuration, it would fail.
>>
>> The configuration string was also appended to the tests names. This was
>> done to have a better test run report:
>>
>> ...
>> [SKIPPED] owned_bitmap_out_of_bounds_cfg_not_config_rust_bitmap_hardened
>> [PASSED] owned_bitmap_out_of_bounds_cfg_config_rust_bitmap_hardened
>> ...
>>
>> Otherwise we would have something like the following:
>> ...
>> [SKIPPED] owned_bitmap_out_of_bounds
>> [PASSED] owned_bitmap_out_of_bounds
>> ...
>>
>> Regarding this:
>> - Does it makes sense to allow same test names with different cfgs?
>
> Yes-ish. I think it definitely makes sense for the same test to be
> redefined with different cfgs, but I'd rather only one of those tests
> then actually be compiled in (see below).
>
>> - Is it ok to 'stringify' the configuration so it can be distinguished
>> in the report? Would you prefer something like `_case_1` `_case_2` ..
>> instead?
>
> I don't _like_ this: my preference would be for us to keep the same
> name, and just not emit a test_case for anything which should be
> compiled out with cfg. Unfortunately, implementing that is a bit harder
> than would be ideal: we need a way of evaluating the cfg() arguments in
> a proc macro, I think. (Ultimately, because otherwise there's no way of
> statically determining the length of the TEST_CASES array?)
>
> Unless you've got a good idea how to fix this, though, I'm happy to put
> up with adding the configs to the name for now. Though if there's a nice
> way to make the names shorter
> (rust_test_kunit_parse_cfg_in_kunit_test_cfg_config_rust_kunit_selftest_equals_n
> is definitely too long a test name, for instance), that'd be best.
I did not like it either but I could not find a way of evaluating the
correspondig cfgs and not including the ones that were not active in the
TEST_CASES array. I implemented the Gary's solution [2] (very neat
trick!) and it worked really well!
Again, thank you both for the feedback. I'll be sending a patch soon.
Best regards,
Nicolás
[1] https://github.com/Rust-for-Linux/linux/blob/93f51579e7df248780214094418f205253383cc5/rust/kernel/lib.rs#L179
[2] https://lore.kernel.org/rust-for-linux/DLLYGLVEA0R3.3D1733XFFTFPV@garyguo.net/
^ permalink raw reply [flat|nested] 10+ messages in thread