From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qv1-f45.google.com (mail-qv1-f45.google.com [209.85.219.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 937AD1AF0A7 for ; Sat, 13 Sep 2025 04:13:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.219.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757736793; cv=none; b=W5qWzYOoymm8wTpj6mPamrZkObUH3Xke8crT6E3HJzZUV/1tDSUxkth5CsGOEIJUBE3yzFziHDkVplELPHlWWnq1tuW5sdjhskP+u9fN6r/hRI2fDXCuZ4uzEx2NEfGgFl6FbJsG8cNVxvur2c2+WXPpxGkQZQvvS5xha6O1yGo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757736793; c=relaxed/simple; bh=6Wv2HIN7vUW7BDwatkGfyIrEwGjkldeub3pyvlc0eVc=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=pZRTgfgR2A+LcYYwhkuhznJldD06ISV/6z6TxdzFEMHrHURQUrm0hB7vKwbH4MwpYhyhQGiQ/j3teqBNJeWzz1fAJvkHLCSrTe0jqnTU0ehDlVLWHRm5M5azxqW0dBXWWt6alDXMbGsdxBzgN36Gx2bCBqVb+YACuKE1+VrXIFE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=VKgq0z3i; arc=none smtp.client-ip=209.85.219.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="VKgq0z3i" Received: by mail-qv1-f45.google.com with SMTP id 6a1803df08f44-7726a3f185eso3713626d6.2 for ; Fri, 12 Sep 2025 21:13:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1757736790; x=1758341590; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=2WaF7o0RRlJLnLEo33jbMbEqykYIDx6ByziHakyovt0=; b=VKgq0z3iwwXWN7J+o3FLnj01ipIwk7ZZQa2ydqGiL6eWMNE1j5LiYqhtUsS0i3u5Aa VEi9K8dk0kpeso5Fd0tfJ3VqOHG3K94yiqybc4/OpMlymyi7kySg45Lqb1tV4CDPW67i 6HTrfmm8vti+xjdZIrHWkCfAdZeUyBHB+x+oUnJPN9EcC8h9Ncg9givLBx0gokriOg1L HoKaQuWS6a75MKB4lQnIpUcwcmgbURHOpl5Kf0YJQNE4X9gDPji2M0Pjgfo40NZaWlN/ Jouv2Om5eywaR7+xOc1k0YwXU165RHHEiKeEyzklKj32Y/sGbZEDH1J01yjIgov/tN8v 7Xsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1757736790; x=1758341590; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=2WaF7o0RRlJLnLEo33jbMbEqykYIDx6ByziHakyovt0=; b=etSsZ8WMh2iivEpaGwJvkzTpBtbeMMP3UsItZqmB1pyufGOxp6st+HtW2l5DWilhnV d+gJ+GUyvxnmcCH4uu3TIZOkPP8uRn/59KqzUdUz8WwX7i4+8ah+JPPY2Qk/Kd7unmPo PWfv9zhd0Fn8fIPBqS/LHuPWRiZqh09BdBASe7R7KYMLq2Fm1LEDMhjwapZsQO684cmZ aiHp3hEXjuZ+h4BYWoSykbqnEF+pUMfMMqr4oWQItwpX0E4kQtzOVrNu8zXH7Uqq7wmG Hj9YJlPVCEGEjn+B9RVuYwKiTaRTY+DiI9oUmN3J1GjBsOJJE5QrYLe4ZO8GiLAYFFf3 VL4Q== X-Forwarded-Encrypted: i=1; AJvYcCW8OOLNzUCJZJ4DwTw8T8p2M02caodP59oxqTGl60S0GcsNJzoNPx0YRKd25S+3SujZPNWvxEYWw4bKXlK5HA==@vger.kernel.org X-Gm-Message-State: AOJu0Yw1iHUH8jl4A+sHQZeggwQyUsr4FNUl5rjse4X2y1c0XI+v5DXX OLmUYHhlA327eC3Oz8iM2ccADA+r2EV2050eYRYbNXslGIYHGTpj/OEmumW0pb//b3nhxrfOmfk YIxgoALZgjKqpZefPLuDqINPWfQXdCyfImhE35TC5 X-Gm-Gg: ASbGncvWhj91yuiF+FUixbRLBgD9bmQs8VSro5nonMN8jyH3xLH4mSKMvuXzg7paVnz 3YazignAIAmuQRfQs8+DghJxrv5h3qnw+KmxEiiYVXWQXbq0Bb1av+Jqt39qEQAShSvkrRJR+9A 7dNe0xoLnKrYbNrFEECkvFkyOpunKmG9P+fASVU3GhiqaBF0B90Tzx5fepdkpNxCG8fWzOXKGs6 wP3W/gpwPUD X-Google-Smtp-Source: AGHT+IHKc+BIL8HoXt+AUZpO+zz3DYyjJLhkC0RgZhsdwqSW3JBqhrTUk4DXveLp59CE951IsU1IKqchh0/sEkjjWYQ= X-Received: by 2002:ad4:4f05:0:b0:768:24e:734b with SMTP id 6a1803df08f44-768027c3b96mr58195596d6.45.1757736790152; Fri, 12 Sep 2025 21:13:10 -0700 (PDT) Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20250907201558.104566-2-ent3rm4n@gmail.com> In-Reply-To: <20250907201558.104566-2-ent3rm4n@gmail.com> From: David Gow Date: Sat, 13 Sep 2025 12:12:58 +0800 X-Gm-Features: AS18NWA_OPADCOyEQRHmaGqDz6q8iqh-FKjck1YFOyZcNegh2IRpto0VgcRD95E Message-ID: Subject: Re: [PATCH] rust: kunit: allow `cfg` on `test`s To: Kaibo Ma Cc: Brendan Higgins , Rae Moar , Miguel Ojeda , Alex Gaynor , linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com, Boqun Feng , Gary Guo , =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= , Benno Lossin , Andreas Hindborg , Alice Ryhl , Trevor Gross , Danilo Krummrich , rust-for-linux@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, 8 Sept 2025 at 04:17, Kaibo Ma 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=3D=3D48=3D69hYiDo132= 1pCzgn_n1_jg=3Dez5UYXX91c+g5JVQ@mail.gmail.com/ [1] > Closes: https://github.com/Rust-for-Linux/linux/issues/1185 > Suggested-by: Miguel Ojeda > Signed-off-by: Kaibo Ma > --- 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 =3D ::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 =3D ::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=C3=A9 Exp=C3=B3sito > > use proc_macro::{Delimiter, Group, TokenStream, TokenTree}; > +use std::collections::HashMap; > use std::fmt::Write; > > pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenSt= ream { > @@ -41,20 +42,32 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: Toke= nStream) -> TokenStream { > // Get the functions set as tests. Search for `[test]` -> `fn`. > let mut body_it =3D body.stream().into_iter(); > let mut tests =3D Vec::new(); > + let mut attributes: HashMap =3D HashMap::new(); > while let Some(token) =3D body_it.next() { > match token { > - TokenTree::Group(ident) if ident.to_string() =3D=3D "[test]"= =3D> match body_it.next() { > - Some(TokenTree::Ident(ident)) if ident.to_string() =3D= =3D "fn" =3D> { > - let test_name =3D match body_it.next() { > - Some(TokenTree::Ident(ident)) =3D> ident.to_stri= ng(), > - _ =3D> continue, > - }; > - tests.push(test_name); > + TokenTree::Punct(ref p) if p.as_char() =3D=3D '#' =3D> match= body_it.next() { > + Some(TokenTree::Group(g)) if g.delimiter() =3D=3D Delimi= ter::Bracket =3D> { > + if let Some(TokenTree::Ident(name)) =3D g.stream().i= nto_iter().next() { > + // Collect attributes because we need to find wh= ich 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; > } > - _ =3D> continue, > + _ =3D> (), > }, > + TokenTree::Ident(i) if i.to_string() =3D=3D "fn" && attribut= es.contains_key("test") =3D> { > + if let Some(TokenTree::Ident(test_name)) =3D body_it.nex= t() { > + tests.push((test_name, attributes.remove("cfg").unwr= ap_or_default())) > + } > + } > + > _ =3D> (), > } > + attributes.clear(); > } > > // Add `#[cfg(CONFIG_KUNIT=3D"y")]` before the module declaration. > @@ -100,11 +113,20 @@ pub(crate) fn kunit_tests(attr: TokenStream, ts: To= kenStream) -> TokenStream { > let mut test_cases =3D "".to_owned(); > let mut assert_macros =3D "".to_owned(); > let path =3D crate::helpers::file(); > - for test in &tests { > + let num_tests =3D tests.len(); > + for (test, cfg_attr) in tests { > let kunit_wrapper_fn_name =3D format!("kunit_rust_wrapper_{test}= "); > - // An extra `use` is used here to reduce the length of the messa= ge. > + // Append any `cfg` attributes the user might have written on th= eir 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 =3D format!( > - "unsafe extern \"C\" fn {kunit_wrapper_fn_name}(_test: *mut = ::kernel::bindings::kunit) {{ use ::kernel::kunit::is_test_result_ok; asser= t!(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; {}] =3D= [\n{test_cases} ::kernel::kunit::kunit_case_null(),\n];", > - tests.len() + 1 > + num_tests + 1 > ) > .unwrap(); > > -- > 2.50.1 >