On Sun, 4 Jan 2026 18:38:53 -0500 Tamir Duberstein <[email protected]> wrote:
> On Thu, Dec 11, 2025 at 2:30 PM Gary Guo <[email protected]> wrote: > > > > From: Gary Guo <[email protected]> > > > > This allows significant cleanups. > > > > Signed-off-by: Gary Guo <[email protected]> > > --- > > rust/macros/kunit.rs | 274 +++++++++++++++++++------------------------ > > rust/macros/lib.rs | 6 +- > > 2 files changed, 123 insertions(+), 157 deletions(-) > > > > diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs > > index 7427c17ee5f5c..516219f5b1356 100644 > > --- a/rust/macros/kunit.rs > > +++ b/rust/macros/kunit.rs > > ... > > + // Make the entire module gated behind `CONFIG_KUNIT`. > > + module > > + .attrs > > + .insert(0, parse_quote!(#[cfg(CONFIG_KUNIT="y")])); > > Does this need to be the first attribute? I think it can just be > pushed to the end. This is the current behaviour, and I think it should be the first. Other attributes shouldn't need to be processed if kunit is not enabled. > > > > > > - // Add `#[cfg(CONFIG_KUNIT="y")]` before the module declaration. > > - let config_kunit = > > "#[cfg(CONFIG_KUNIT=\"y\")]".to_owned().parse().unwrap(); > > - tokens.insert( > > - 0, > > - TokenTree::Group(Group::new(Delimiter::None, config_kunit)), > > - ); > > + let mut processed_items = Vec::new(); > > + let mut test_cases = Vec::new(); > > > > // Generate the test KUnit test suite and a test case for each > > `#[test]`. > > + // > > // The code generated for the following test module: > > // > > // ``` > > @@ -110,98 +79,93 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: > > TokenStream) -> TokenStream { > > // > > // ::kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, > > TEST_CASES); > > // ``` > > - let mut kunit_macros = "".to_owned(); > > - let mut test_cases = "".to_owned(); > > - let mut assert_macros = "".to_owned(); > > - let path = crate::helpers::file(); > > - let num_tests = tests.len(); > > - for (test, cfg_attr) in tests { > > - let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{test}"); > > - // 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!( > > - r#"unsafe extern "C" fn {kunit_wrapper_fn_name}(_test: *mut > > ::kernel::bindings::kunit) > > - {{ > > - (*_test).status = > > ::kernel::bindings::kunit_status_KUNIT_SKIPPED; > > - {cfg_attr} {{ > > - (*_test).status = > > ::kernel::bindings::kunit_status_KUNIT_SUCCESS; > > - use ::kernel::kunit::is_test_result_ok; > > - assert!(is_test_result_ok({test}())); > > + // > > + // Non-function items (e.g. imports) are preserved. > > + for item in module_items { > > + let Item::Fn(mut f) = item else { > > + processed_items.push(item); > > + continue; > > + }; > > + > > + // TODO: Replace below with `extract_if` when MSRV is bumped above > > 1.85. > > + // Remove `#[test]` attributes applied on the function and count > > if any. > > What does "count if any" mean here? Sorry, I think the last half of the sentence was missing. I meant "count if any is removed". > > > + if !f.attrs.iter().any(|attr| attr.path().is_ident("test")) { > > + processed_items.push(Item::Fn(f)); > > + continue; > > + } > > + f.attrs.retain(|attr| !attr.path().is_ident("test")); > > Can this code be something like this: > > let before = f.attrs.len(); > f.attrs.retain(|attr| !attr.path().is_ident("test")); > let after = f.attrs.len(); > > if after == before { > processed_items.push(Item::Fn(f)); > continue; > } Indeed looks better. > > > + > > + let test = f.sig.ident.clone(); > > + > > + // Retrieve `#[cfg]` applied on the function which needs to be > > present on derived items too. > > + let cfg_attrs: Vec<_> = f > > + .attrs > > + .iter() > > + .filter(|attr| attr.path().is_ident("cfg")) > > + .cloned() > > + .collect(); > > + > > + // Before the test, override usual `assert!` and `assert_eq!` > > macros with ones that call > > + // KUnit instead. > > + let test_str = test.to_string(); > > + let path = crate::helpers::file(); > > + processed_items.push(parse_quote! { > > + #[allow(unused)] > > + macro_rules! assert { > > + ($cond:expr $(,)?) => {{ > > + kernel::kunit_assert!(#test_str, #path, 0, $cond); > > + }} > > + } > > + }); > > + processed_items.push(parse_quote! { > > + #[allow(unused)] > > + macro_rules! assert_eq { > > + ($left:expr, $right:expr $(,)?) => {{ > > + kernel::kunit_assert_eq!(#test_str, #path, 0, $left, > > $right); > > }} > > - }}"#, > > + } > > + }); > > Am I reading this right that the macros will be repeatedly redefined > before each test? Could we put them inside each test body instead? Indeed. The `test_str` is different for each test so this needs to be redefined for each test. I experimented about better approaches but none of them are too satisfactory. I think the existing approach is not unreasonable. Putting them into test body sounds more reasonable (I suppose the existing impl doesn't want to parse functions), but I'll leave it to the future as this commit is focusing on converting with minimal behavioural change. Best, Gary
