The `kunit_test` proc macro only checks for the `test` attribute immediately preceding a `fn`. If the function is disabled via a `cfg`, the generated code would result in a compile error referencing a non-existent function [1].
This collects attributes and specifically cherry-picks `cfg` attributes to be duplicated inside KUnit wrapper functions such that a test function disabled via `cfg` compiles and is ignored correctly.
Link: https://lore.kernel.org/rust-for-linux/CANiq72==48=69hYiDo1321pCzgn_n1_jg=ez... [1] Closes: https://github.com/Rust-for-Linux/linux/issues/1185 Suggested-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Kaibo Ma ent3rm4n@gmail.com --- rust/kernel/kunit.rs | 7 +++++++ rust/macros/kunit.rs | 46 ++++++++++++++++++++++++++++++++------------ 2 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 41efd8759..32640dfc9 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -357,4 +357,11 @@ fn rust_test_kunit_example_test() { fn rust_test_kunit_in_kunit_test() { assert!(in_kunit_test()); } + + #[test] + #[cfg(not(all()))] + fn rust_test_kunit_always_disabled_test() { + // This test should never run because of the `cfg`. + assert!(false); + } } diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs index 81d18149a..850a321e5 100644 --- a/rust/macros/kunit.rs +++ b/rust/macros/kunit.rs @@ -5,6 +5,7 @@ //! Copyright (c) 2023 José Expósito jose.exposito89@gmail.com
use proc_macro::{Delimiter, Group, TokenStream, TokenTree}; +use std::collections::HashMap; use std::fmt::Write;
pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { @@ -41,20 +42,32 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { // Get the functions set as tests. Search for `[test]` -> `fn`. let mut body_it = body.stream().into_iter(); let mut tests = Vec::new(); + let mut attributes: HashMap<String, TokenStream> = HashMap::new(); while let Some(token) = body_it.next() { match token { - TokenTree::Group(ident) if ident.to_string() == "[test]" => match body_it.next() { - Some(TokenTree::Ident(ident)) if ident.to_string() == "fn" => { - let test_name = match body_it.next() { - Some(TokenTree::Ident(ident)) => ident.to_string(), - _ => continue, - }; - tests.push(test_name); + TokenTree::Punct(ref p) if p.as_char() == '#' => match body_it.next() { + Some(TokenTree::Group(g)) if g.delimiter() == Delimiter::Bracket => { + if let Some(TokenTree::Ident(name)) = g.stream().into_iter().next() { + // Collect attributes because we need to find which are tests. We also + // need to copy `cfg` attributes so tests can be conditionally enabled. + attributes + .entry(name.to_string()) + .or_default() + .extend([token, TokenTree::Group(g)]); + } + continue; } - _ => continue, + _ => (), }, + TokenTree::Ident(i) if i.to_string() == "fn" && attributes.contains_key("test") => { + if let Some(TokenTree::Ident(test_name)) = body_it.next() { + tests.push((test_name, attributes.remove("cfg").unwrap_or_default())) + } + } + _ => (), } + attributes.clear(); }
// Add `#[cfg(CONFIG_KUNIT="y")]` before the module declaration. @@ -100,11 +113,20 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { let mut test_cases = "".to_owned(); let mut assert_macros = "".to_owned(); let path = crate::helpers::file(); - for test in &tests { + let num_tests = tests.len(); + for (test, cfg_attr) in tests { let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{test}"); - // An extra `use` is used here to reduce the length of the message. + // Append any `cfg` attributes the user might have written on their tests so we don't + // attempt to call them when they are `cfg`'d out. An extra `use` is used here to reduce + // the length of the assert message. let kunit_wrapper = format!( - "unsafe extern "C" fn {kunit_wrapper_fn_name}(_test: *mut ::kernel::bindings::kunit) {{ use ::kernel::kunit::is_test_result_ok; assert!(is_test_result_ok({test}())); }}", + r#"unsafe extern "C" fn {kunit_wrapper_fn_name}(_test: *mut ::kernel::bindings::kunit) + {{ + {cfg_attr} {{ + use ::kernel::kunit::is_test_result_ok; + assert!(is_test_result_ok({test}())); + }} + }}"#, ); writeln!(kunit_macros, "{kunit_wrapper}").unwrap(); writeln!( @@ -139,7 +161,7 @@ macro_rules! assert_eq {{ writeln!( kunit_macros, "static mut TEST_CASES: [::kernel::bindings::kunit_case; {}] = [\n{test_cases} ::kernel::kunit::kunit_case_null(),\n];", - tests.len() + 1 + num_tests + 1 ) .unwrap();
On Mon, 8 Sept 2025 at 04:17, Kaibo Ma ent3rm4n@gmail.com wrote:
The `kunit_test` proc macro only checks for the `test` attribute immediately preceding a `fn`. If the function is disabled via a `cfg`, the generated code would result in a compile error referencing a non-existent function [1].
This collects attributes and specifically cherry-picks `cfg` attributes to be duplicated inside KUnit wrapper functions such that a test function disabled via `cfg` compiles and is ignored correctly.
Link: https://lore.kernel.org/rust-for-linux/CANiq72==48=69hYiDo1321pCzgn_n1_jg=ez... [1] Closes: https://github.com/Rust-for-Linux/linux/issues/1185 Suggested-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Kaibo Ma ent3rm4n@gmail.com
Thanks very much: I think this is a great improvement over not having 'cfg' work at all.
I do think, though, that we need to handle disabled tests differently, as it causes two issues: 1. Currently, disabled tests are still included in the suite which contains them, and always pass. It'd be nice if they either were missing from the suite, or were marked 'skipped' by default. 2. It's not possible to have several implementations of the same test (or, depending on how you look at it, several tests with the same name) with different #[cfg] attributes, even if all but one are disabled.
My ideal solution to both of these issues would be to only include tests (i.e., the wrapper function and the kunit_case struct) if they're enabled. That's possibly more difficult than it looks, though: my initial attempt at implementing this ran aground trying to calculate num_tests.
Even if that's not feasible, we should at least make disabled tests be marked 'skipped'. A quick way of doing this would be to mark the test as skipped in the wrapper function: (*_test).status = ::kernel::bindings::kunit_status_KUNIT_SKIPPED; And then re-mark it as success (KUnit tests expect the initial state to be success) within the {cfg_attr} block: (*_test).status = ::kernel::bindings::kunit_status_KUNIT_SUCCESS;
That doesn't solve issue #2, but I suspect that's rare enough that we can leave it until someone actually has a good reason to have two test implementations.
Otherwise, this seems fine to me.
Thanks, -- David
rust/kernel/kunit.rs | 7 +++++++ rust/macros/kunit.rs | 46 ++++++++++++++++++++++++++++++++------------ 2 files changed, 41 insertions(+), 12 deletions(-)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 41efd8759..32640dfc9 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -357,4 +357,11 @@ fn rust_test_kunit_example_test() { fn rust_test_kunit_in_kunit_test() { assert!(in_kunit_test()); }
- #[test]
- #[cfg(not(all()))]
- fn rust_test_kunit_always_disabled_test() {
// This test should never run because of the `cfg`.assert!(false);- }
} diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs index 81d18149a..850a321e5 100644 --- a/rust/macros/kunit.rs +++ b/rust/macros/kunit.rs @@ -5,6 +5,7 @@ //! Copyright (c) 2023 José Expósito jose.exposito89@gmail.com
use proc_macro::{Delimiter, Group, TokenStream, TokenTree}; +use std::collections::HashMap; use std::fmt::Write;
pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { @@ -41,20 +42,32 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { // Get the functions set as tests. Search for `[test]` -> `fn`. let mut body_it = body.stream().into_iter(); let mut tests = Vec::new();
- let mut attributes: HashMap<String, TokenStream> = HashMap::new(); while let Some(token) = body_it.next() { match token {
TokenTree::Group(ident) if ident.to_string() == "[test]" => match body_it.next() {Some(TokenTree::Ident(ident)) if ident.to_string() == "fn" => {let test_name = match body_it.next() {Some(TokenTree::Ident(ident)) => ident.to_string(),_ => continue,};tests.push(test_name);
TokenTree::Punct(ref p) if p.as_char() == '#' => match body_it.next() {Some(TokenTree::Group(g)) if g.delimiter() == Delimiter::Bracket => {if let Some(TokenTree::Ident(name)) = g.stream().into_iter().next() {// Collect attributes because we need to find which are tests. We also// need to copy `cfg` attributes so tests can be conditionally enabled.attributes.entry(name.to_string()).or_default().extend([token, TokenTree::Group(g)]);}continue; }
_ => continue,
_ => (), },TokenTree::Ident(i) if i.to_string() == "fn" && attributes.contains_key("test") => {if let Some(TokenTree::Ident(test_name)) = body_it.next() {tests.push((test_name, attributes.remove("cfg").unwrap_or_default()))}}_ => (), }attributes.clear();}
// Add `#[cfg(CONFIG_KUNIT="y")]` before the module declaration.
@@ -100,11 +113,20 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream { let mut test_cases = "".to_owned(); let mut assert_macros = "".to_owned(); let path = crate::helpers::file();
- for test in &tests {
- let num_tests = tests.len();
- for (test, cfg_attr) in tests { let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{test}");
// An extra `use` is used here to reduce the length of the message.
// Append any `cfg` attributes the user might have written on their tests so we don't// attempt to call them when they are `cfg`'d out. An extra `use` is used here to reduce// the length of the assert message. let kunit_wrapper = format!(
"unsafe extern \"C\" fn {kunit_wrapper_fn_name}(_test: *mut ::kernel::bindings::kunit) {{ use ::kernel::kunit::is_test_result_ok; assert!(is_test_result_ok({test}())); }}",
r#"unsafe extern "C" fn {kunit_wrapper_fn_name}(_test: *mut ::kernel::bindings::kunit){{{cfg_attr} {{use ::kernel::kunit::is_test_result_ok;assert!(is_test_result_ok({test}()));}}}}"#, ); writeln!(kunit_macros, "{kunit_wrapper}").unwrap(); writeln!(@@ -139,7 +161,7 @@ macro_rules! assert_eq {{ writeln!( kunit_macros, "static mut TEST_CASES: [::kernel::bindings::kunit_case; {}] = [\n{test_cases} ::kernel::kunit::kunit_case_null(),\n];",
tests.len() + 1
num_tests + 1-- 2.50.1
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Kaibo Ma