diff mbox series

[v4,2/3] rust: macros: add macro to easily run KUnit tests

Message ID 20241101064505.3820737-3-davidgow@google.com (mailing list archive)
State New
Headers show
Series rust: kunit: Support KUnit tests with a user-space like syntax | expand

Commit Message

David Gow Nov. 1, 2024, 6:45 a.m. UTC
From: José Expósito <jose.exposito89@gmail.com>

Add a new procedural macro (`#[kunit_tests(kunit_test_suit_name)]`) to
run KUnit tests using a user-space like syntax.

The macro, that should be used on modules, transforms every `#[test]`
in a `kunit_case!` and adds a `kunit_unsafe_test_suite!` registering
all of them.

The only difference with user-space tests is that instead of using
`#[cfg(test)]`, `#[kunit_tests(kunit_test_suit_name)]` is used.

Note that `#[cfg(CONFIG_KUNIT)]` is added so the test module is not
compiled when `CONFIG_KUNIT` is set to `n`.

Reviewed-by: David Gow <davidgow@google.com>
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: David Gow <davidgow@google.com>
---

Changes since v3:
https://lore.kernel.org/linux-kselftest/20241030045719.3085147-6-davidgow@google.com/
- The #[kunit_tests()] macro now preserves span information, so
  errors can be better reported. (Thanks, Boqun!)
- The example test has been replaced to no longer use assert_eq!() with
  a constant bool argument (which triggered a clippy warning now we
  have the span info). It now checks 1 + 1 == 2, to match the C example.
  - (The in_kunit_test() test in the next patch uses assert!() to check
    a bool, so having something different here seemed to make a better
    example.)

Changes since v2:
https://lore.kernel.org/linux-kselftest/20241029092422.2884505-3-davidgow@google.com/
- Include missing rust/macros/kunit.rs file from v2. (Thanks Boqun!)
- The proc macro now emits an error if the suite name is too long.

Changes since v1:
https://lore.kernel.org/lkml/20230720-rustbind-v1-2-c80db349e3b5@google.com/
- Rebased on top of rust-next
- Make use of the new const functions, rather than the kunit_case!()
  macro.

---
 MAINTAINERS          |   1 +
 rust/kernel/kunit.rs |  10 +++
 rust/macros/kunit.rs | 168 +++++++++++++++++++++++++++++++++++++++++++
 rust/macros/lib.rs   |  29 ++++++++
 4 files changed, 208 insertions(+)
 create mode 100644 rust/macros/kunit.rs

Comments

Boqun Feng Nov. 1, 2024, 9:38 p.m. UTC | #1
On Fri, Nov 01, 2024 at 02:45:01PM +0800, David Gow wrote:
[...]
> @@ -273,3 +275,11 @@ macro_rules! kunit_unsafe_test_suite {
>          };
>      };
>  }
> +
> +#[kunit_tests(rust_kernel_kunit)]
> +mod tests {
> +    #[test]
> +    fn rust_test_kunit_example_test() {
> +        assert_eq!(1 + 1, 2);

Clippy reported:

ERROR:root:error: identical args used in this `assert_eq!` macro call
   --> ../rust/kernel/kunit.rs:348:20
    |
348 |         assert_eq!(1 + 1, 2);
    |                    ^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#eq_op
    = note: `-D clippy::eq-op` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::eq_op)]`

but this lint doesn't make sense to me, I would say we just drop this
lint?

Regards,
Boqun

> +    }
> +}
[...]
Miguel Ojeda Nov. 1, 2024, 10:12 p.m. UTC | #2
On Fri, Nov 1, 2024 at 10:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> but this lint doesn't make sense to me, I would say we just drop this
> lint?

I am not sure if it is intended to fire there (i.e. to resolve
constants), but seems OK since it is not something one would want
normally to write except in exceptional cases.

So we could drop it globally, but if it is just this case that we
expect, then I think it is better to just `#[expect(...)]` it (or
perhaps use a different example) and then, if we see in the future
that we have quite a few "trivial" comparisons like this, then we can
drop it globally.

Cheers,
Miguel
Boqun Feng Nov. 2, 2024, 12:08 a.m. UTC | #3
On Fri, Nov 01, 2024 at 11:12:06PM +0100, Miguel Ojeda wrote:
> On Fri, Nov 1, 2024 at 10:38 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > but this lint doesn't make sense to me, I would say we just drop this
> > lint?
> 
> I am not sure if it is intended to fire there (i.e. to resolve
> constants), but seems OK since it is not something one would want
> normally to write except in exceptional cases.
> 

Hmm.. so I think clippy won't warn for a normal Rust #[test] function:

	https://github.com/rust-lang/rust-clippy/pull/7811

but since in this patch #[test] is handled by us instead of by "rustc
--test" (I assume this is how Rust std handle #[cfg(test)] and
#[test]?), clippy doesn't know.

> So we could drop it globally, but if it is just this case that we
> expect, then I think it is better to just `#[expect(...)]` it (or
> perhaps use a different example) and then, if we see in the future
> that we have quite a few "trivial" comparisons like this, then we can
> drop it globally.
> 

It seems to me that clippy agrees not triggering this in test code at
least.

Regards,
Boqun

> Cheers,
> Miguel
Miguel Ojeda Nov. 3, 2024, 3:41 p.m. UTC | #4
On Sat, Nov 2, 2024 at 1:08 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Hmm.. so I think clippy won't warn for a normal Rust #[test] function:
>
>         https://github.com/rust-lang/rust-clippy/pull/7811

That is a very good point. It is a bit surprising that those details
are not documented, but we could mimic that behavior.

(Personally, I don't particularly enjoy exceptional/context-dependent
cases, unless it is something used everywhere, like `use`ing the
prelude that we have).

Cheers,
Miguel
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index b77f4495dcf4..b65035ede675 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12433,6 +12433,7 @@  F:	Documentation/dev-tools/kunit/
 F:	include/kunit/
 F:	lib/kunit/
 F:	rust/kernel/kunit.rs
+F:	rust/macros/kunit.rs
 F:	scripts/rustdoc_test_*
 F:	tools/testing/kunit/
 
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 85bc1faff0d5..71ce1d145be8 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -40,6 +40,8 @@  pub fn info(args: fmt::Arguments<'_>) {
     }
 }
 
+use macros::kunit_tests;
+
 /// Asserts that a boolean expression is `true` at runtime.
 ///
 /// Public but hidden since it should only be used from generated tests.
@@ -273,3 +275,11 @@  macro_rules! kunit_unsafe_test_suite {
         };
     };
 }
+
+#[kunit_tests(rust_kernel_kunit)]
+mod tests {
+    #[test]
+    fn rust_test_kunit_example_test() {
+        assert_eq!(1 + 1, 2);
+    }
+}
diff --git a/rust/macros/kunit.rs b/rust/macros/kunit.rs
new file mode 100644
index 000000000000..c421ff65f7f9
--- /dev/null
+++ b/rust/macros/kunit.rs
@@ -0,0 +1,168 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Procedural macro to run KUnit tests using a user-space like syntax.
+//!
+//! Copyright (c) 2023 José Expósito <jose.exposito89@gmail.com>
+
+use proc_macro::{Delimiter, Group, TokenStream, TokenTree};
+use std::fmt::Write;
+
+pub(crate) fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
+    if attr.to_string().is_empty() {
+        panic!("Missing test name in #[kunit_tests(test_name)] macro")
+    }
+
+    if attr.to_string().as_bytes().len() > 255 {
+        panic!("The test suite name `{}` exceeds the maximum length of 255 bytes.", attr)
+    }
+
+    let mut tokens: Vec<_> = ts.into_iter().collect();
+
+    // Scan for the "mod" keyword.
+    tokens
+        .iter()
+        .find_map(|token| match token {
+            TokenTree::Ident(ident) => match ident.to_string().as_str() {
+                "mod" => Some(true),
+                _ => None,
+            },
+            _ => None,
+        })
+        .expect("#[kunit_tests(test_name)] attribute should only be applied to modules");
+
+    // Retrieve the main body. The main body should be the last token tree.
+    let body = match tokens.pop() {
+        Some(TokenTree::Group(group)) if group.delimiter() == Delimiter::Brace => group,
+        _ => panic!("cannot locate main body of module"),
+    };
+
+    // Get the functions set as tests. Search for `[test]` -> `fn`.
+    let mut body_it = body.stream().into_iter();
+    let mut tests = Vec::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);
+                }
+                _ => continue,
+            },
+            _ => (),
+        }
+    }
+
+    // Add `#[cfg(CONFIG_KUNIT)]` before the module declaration.
+    let config_kunit = "#[cfg(CONFIG_KUNIT)]".to_owned().parse().unwrap();
+    tokens.insert(
+        0,
+        TokenTree::Group(Group::new(Delimiter::None, config_kunit)),
+    );
+
+    // Generate the test KUnit test suite and a test case for each `#[test]`.
+    // The code generated for the following test module:
+    //
+    // ```
+    // #[kunit_tests(kunit_test_suit_name)]
+    // mod tests {
+    //     #[test]
+    //     fn foo() {
+    //         assert_eq!(1, 1);
+    //     }
+    //
+    //     #[test]
+    //     fn bar() {
+    //         assert_eq!(2, 2);
+    //     }
+    // ```
+    //
+    // Looks like:
+    //
+    // ```
+    // unsafe extern "C" fn kunit_rust_wrapper_foo(_test: *mut kernel::bindings::kunit) {
+    //     foo();
+    // }
+    // static mut KUNIT_CASE_FOO: kernel::bindings::kunit_case =
+    //     kernel::kunit::kunit_case(foo, kunit_rust_wrapper_foo);
+    //
+    // unsafe extern "C" fn kunit_rust_wrapper_bar(_test: * mut kernel::bindings::kunit) {
+    //     bar();
+    // }
+    // static mut KUNIT_CASE_BAR: kernel::bindings::kunit_case =
+    //     kernel::kunit::kunit_case(bar, kunit_rust_wrapper_bar);
+    //
+    // static mut KUNIT_CASE_NULL: kernel::bindings::kunit_case = kernel::kunit::kunit_case_null();
+    //
+    // static mut TEST_CASES : &mut[kernel::bindings::kunit_case] = unsafe {
+    //     &mut [KUNIT_CASE_FOO, KUNIT_CASE_BAR, KUNIT_CASE_NULL]
+    // };
+    //
+    // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
+    // ```
+    let mut kunit_macros = "".to_owned();
+    let mut test_cases = "".to_owned();
+    for test in tests {
+        let kunit_wrapper_fn_name = format!("kunit_rust_wrapper_{}", test);
+        let kunit_case_name = format!("KUNIT_CASE_{}", test.to_uppercase());
+        let kunit_wrapper = format!(
+            "unsafe extern \"C\" fn {}(_test: *mut kernel::bindings::kunit) {{ {}(); }}",
+            kunit_wrapper_fn_name, test
+        );
+        let kunit_case = format!(
+            "static mut {}: kernel::bindings::kunit_case = kernel::kunit::kunit_case(kernel::c_str!(\"{}\"), {});",
+            kunit_case_name, test, kunit_wrapper_fn_name
+        );
+        writeln!(kunit_macros, "{kunit_wrapper}").unwrap();
+        writeln!(kunit_macros, "{kunit_case}").unwrap();
+        writeln!(test_cases, "{kunit_case_name},").unwrap();
+    }
+
+    writeln!(
+        kunit_macros,
+        "static mut KUNIT_CASE_NULL: kernel::bindings::kunit_case = kernel::kunit::kunit_case_null();"
+    )
+    .unwrap();
+
+    writeln!(
+        kunit_macros,
+        "static mut TEST_CASES : &mut[kernel::bindings::kunit_case] = unsafe {{ &mut[{test_cases} KUNIT_CASE_NULL] }};"
+    )
+    .unwrap();
+
+    writeln!(
+        kunit_macros,
+        "kernel::kunit_unsafe_test_suite!({attr}, TEST_CASES);"
+    )
+    .unwrap();
+
+    // Remove the `#[test]` macros.
+    // We do this at a token level, in order to preserve span information.
+    let mut new_body = vec![];
+    let mut body_it = body.stream().into_iter();
+
+    while let Some(token) = body_it.next() {
+        match token {
+            TokenTree::Punct(ref c) if c.as_char() == '#' => {
+                match body_it.next() {
+                    Some(TokenTree::Group(group)) if group.to_string() == "[test]" => (),
+                    Some(next) => { new_body.extend([token, next]); },
+                    _ => { new_body.push(token); },
+                }
+            },
+            _ => { new_body.push(token); },
+        }
+    }
+
+    let mut new_body = TokenStream::from_iter(new_body);
+    new_body.extend::<TokenStream>(kunit_macros.parse().unwrap());
+
+    tokens.push(TokenTree::Group(Group::new(
+        Delimiter::Brace,
+        new_body
+    )));
+
+    tokens.into_iter().collect()
+}
diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs
index 939ae00b723a..098925b99982 100644
--- a/rust/macros/lib.rs
+++ b/rust/macros/lib.rs
@@ -10,6 +10,7 @@ 
 mod quote;
 mod concat_idents;
 mod helpers;
+mod kunit;
 mod module;
 mod paste;
 mod pin_data;
@@ -430,3 +431,31 @@  pub fn paste(input: TokenStream) -> TokenStream {
 pub fn derive_zeroable(input: TokenStream) -> TokenStream {
     zeroable::derive(input)
 }
+
+/// Registers a KUnit test suite and its test cases using a user-space like syntax.
+///
+/// This macro should be used on modules. If `CONFIG_KUNIT` (in `.config`) is `n`, the target module
+/// is ignored.
+///
+/// # Examples
+///
+/// ```ignore
+/// # use macros::kunit_tests;
+///
+/// #[kunit_tests(kunit_test_suit_name)]
+/// mod tests {
+///     #[test]
+///     fn foo() {
+///         assert_eq!(1, 1);
+///     }
+///
+///     #[test]
+///     fn bar() {
+///         assert_eq!(2, 2);
+///     }
+/// }
+/// ```
+#[proc_macro_attribute]
+pub fn kunit_tests(attr: TokenStream, ts: TokenStream) -> TokenStream {
+    kunit::kunit_tests(attr, ts)
+}