Hi all,
v5 here is a small set of fixes and a rebase of the previous versions. If there are no major issues, I'd like to land this soon so it can be used and tested ready for 6.14.
This series was originally written by José Expósito, and has been modified and updated by Matt Gilbride and myself. The original version can be found here: https://github.com/Rust-for-Linux/linux/pull/950
Add support for writing KUnit tests in Rust. While Rust doctests are already converted to KUnit tests and run, they're really better suited for examples, rather than as first-class unit tests.
This series implements a series of direct Rust bindings for KUnit tests, as well as a new macro which allows KUnit tests to be written using a close variant of normal Rust unit test syntax. The only change required is replacing '#[cfg(test)]' with '#[kunit_tests(kunit_test_suite_name)]'
An example test would look like: #[kunit_tests(rust_kernel_hid_driver)] mod tests { use super::*; use crate::{c_str, driver, hid, prelude::*}; use core::ptr;
struct SimpleTestDriver; impl Driver for SimpleTestDriver { type Data = (); }
#[test] fn rust_test_hid_driver_adapter() { let mut hid = bindings::hid_driver::default(); let name = c_str!("SimpleTestDriver"); static MODULE: ThisModule = unsafe { ThisModule::from_ptr(ptr::null_mut()) };
let res = unsafe { <hid::Adapter<SimpleTestDriver> as driver::DriverOps>::register(&mut hid, name, &MODULE) }; assert_eq!(res, Err(ENODEV)); // The mock returns -19 } }
Please give this a go, and make sure I haven't broken it! There's almost certainly a lot of improvements which can be made -- and there's a fair case to be made for replacing some of this with generated C code which can use the C macros -- but this is hopefully an adequate implementation for now, and the interface can (with luck) remain the same even if the implementation changes.
A few small notable missing features: - Attributes (like the speed of a test) are hardcoded to the default value. - Similarly, the module name attribute is hardcoded to NULL. In C, we use the KBUILD_MODNAME macro, but I couldn't find a way to use this from Rust which wasn't more ugly than just disabling it. - Assertions are not automatically rewritten to use KUnit assertions.
---
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-1-davidgow@go... - Rebased against 6.13-rc1 - Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel) - "Expect" that the sample assert_eq!(1+1, 2) produces a clippy warning due to a redundant assertion. (Thanks Boqun, Miguel) - Fix some missing safety comments, and remove some unneeded 'unsafe' blocks. (Thanks Boqun) - Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-2-davidgow@go... - The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!) - The #[kunit_tests()] macro now preserves span information, so errors can be better reported. (Thanks, Boqun!) - The example tests have been updated to no longer use assert_eq!() with a constant bool argument (which triggered a clippy warning now we have the span info).
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-1-davidgow@go... - Include missing rust/macros/kunit.rs file from v2. (Thanks Boqun!) - The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!) - The proc macro now emits an error if the suite name is too long. - We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-0-c80db349e3b5@google.com/... - Rebase on top of the latest rust-next (commit 718c4069896c) - Make kunit_case a const fn, rather than a macro (Thanks Boqun) - As a result, the null terminator is now created with kernel::kunit::kunit_case_null() - Use the C kunit_get_current_test() function to implement in_kunit_test(), rather than re-implementing it (less efficiently) ourselves.
Changes since the GitHub PR: - Rebased on top of kselftest/kunit - Add const_mut_refs feature This may conflict with https://lore.kernel.org/lkml/20230503090708.2524310-6-nmi@metaspace.dk/ - Add rust/macros/kunit.rs to the KUnit MAINTAINERS entry
---
José Expósito (3): rust: kunit: add KUnit case and suite macros rust: macros: add macro to easily run KUnit tests rust: kunit: allow to know if we are in a test
MAINTAINERS | 1 + rust/kernel/kunit.rs | 207 +++++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + rust/macros/kunit.rs | 168 +++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 29 ++++++ 5 files changed, 406 insertions(+) create mode 100644 rust/macros/kunit.rs
From: José Expósito jose.exposito89@gmail.com
Add a couple of Rust const functions and macros to allow to develop KUnit tests without relying on generated C code:
- The `kunit_unsafe_test_suite!` Rust macro is similar to the `kunit_test_suite` C macro. It requires a NULL-terminated array of test cases (see below). - The `kunit_case` Rust function is similar to the `KUNIT_CASE` C macro. It generates as case from the name and function. - The `kunit_case_null` Rust function generates a NULL test case, which is to be used as delimiter in `kunit_test_suite!`.
While these functions and macros can be used on their own, a future patch will introduce another macro to create KUnit tests using a user-space like syntax.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: Matt Gilbride mattgilbride@google.com Signed-off-by: Matt Gilbride mattgilbride@google.com Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com ---
Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-2-davidgow@go... - Rebased against 6.13-rc1 - Allowed an unused_unsafe warning after the behaviour of addr_of_mut!() changed in Rust 1.82. (Thanks Boqun, Miguel) - Fix a couple of minor rustfmt issues which were triggering checkpatch warnings.
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-4-davidgow@go... - The kunit_unsafe_test_suite!() macro now panic!s if the suite name is too long, triggering a compile error. (Thanks, Alice!)
Changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-2-davidgow@go... - The kunit_unsafe_test_suite!() macro will truncate the name of the suite if it is too long. (Thanks Alice!) - We no longer needlessly use UnsafeCell<> in kunit_unsafe_test_suite!(). (Thanks Alice!)
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-1-c80db349e3b5@google.com/ - Rebase on top of rust-next - As a result, KUnit attributes are new set. These are hardcoded to the defaults of "normal" speed and no module name. - Split the kunit_case!() macro into two const functions, kunit_case() and kunit_case_null() (for the NULL terminator).
--- rust/kernel/kunit.rs | 118 +++++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 2 files changed, 119 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..5003282cb4c4 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -161,3 +161,121 @@ macro_rules! kunit_assert_eq { $crate::kunit_assert!($name, $file, $diff, $left == $right); }}; } + +/// Represents an individual test case. +/// +/// The test case should have the signature +/// `unsafe extern "C" fn test_case(test: *mut crate::bindings::kunit)`. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. +/// Use `kunit_case_null` to generate such a delimeter. +const fn kunit_case( + name: &'static kernel::str::CStr, + run_case: unsafe extern "C" fn(*mut kernel::bindings::kunit), +) -> kernel::bindings::kunit_case { + kernel::bindings::kunit_case { + run_case: Some(run_case), + name: name.as_char_ptr(), + attr: kernel::bindings::kunit_attributes { + speed: kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL, + }, + generate_params: None, + status: kernel::bindings::kunit_status_KUNIT_SUCCESS, + module_name: core::ptr::null_mut(), + log: core::ptr::null_mut(), + } +} + +/// Represents the NULL test case delimiter. +/// +/// The `kunit_unsafe_test_suite!` macro expects a NULL-terminated list of test cases. This +/// function retuns such a delimiter. +const fn kunit_case_null() -> kernel::bindings::kunit_case { + kernel::bindings::kunit_case { + run_case: None, + name: core::ptr::null_mut(), + generate_params: None, + attr: kernel::bindings::kunit_attributes { + speed: kernel::bindings::kunit_speed_KUNIT_SPEED_NORMAL, + }, + status: kernel::bindings::kunit_status_KUNIT_SUCCESS, + module_name: core::ptr::null_mut(), + log: core::ptr::null_mut(), + } +} + +/// Registers a KUnit test suite. +/// +/// # Safety +/// +/// `test_cases` must be a NULL terminated array of test cases. +/// +/// # Examples +/// +/// ```ignore +/// unsafe extern "C" fn test_fn(_test: *mut crate::bindings::kunit) { +/// let actual = 1 + 1; +/// let expected = 2; +/// assert_eq!(actual, expected); +/// } +/// +/// static mut KUNIT_TEST_CASE: crate::bindings::kunit_case = crate::kunit_case(name, test_fn); +/// static mut KUNIT_NULL_CASE: crate::bindings::kunit_case = crate::kunit_case_null(); +/// static mut KUNIT_TEST_CASES: &mut[crate::bindings::kunit_case] = unsafe { +/// &mut[KUNIT_TEST_CASE, KUNIT_NULL_CASE] +/// }; +/// crate::kunit_unsafe_test_suite!(suite_name, KUNIT_TEST_CASES); +/// ``` +#[macro_export] +macro_rules! kunit_unsafe_test_suite { + ($name:ident, $test_cases:ident) => { + const _: () = { + static KUNIT_TEST_SUITE_NAME: [i8; 256] = { + let name_u8 = core::stringify!($name).as_bytes(); + let mut ret = [0; 256]; + + if name_u8.len() > 255 { + panic!(concat!( + "The test suite name `", + core::stringify!($name), + "` exceeds the maximum length of 255 bytes." + )); + } + + let mut i = 0; + while i < name_u8.len() { + ret[i] = name_u8[i] as i8; + i += 1; + } + + ret + }; + + static mut KUNIT_TEST_SUITE: $crate::bindings::kunit_suite = + $crate::bindings::kunit_suite { + name: KUNIT_TEST_SUITE_NAME, + // SAFETY: User is expected to pass a correct `test_cases`, hence this macro + // named 'unsafe'. + test_cases: unsafe { $test_cases.as_mut_ptr() }, + suite_init: None, + suite_exit: None, + init: None, + exit: None, + attr: $crate::bindings::kunit_attributes { + speed: $crate::bindings::kunit_speed_KUNIT_SPEED_NORMAL, + }, + status_comment: [0; 256usize], + debugfs: core::ptr::null_mut(), + log: core::ptr::null_mut(), + suite_init_err: 0, + is_init: false, + }; + + #[used] + #[link_section = ".kunit_test_suites"] + static mut KUNIT_TEST_SUITE_ENTRY: *const $crate::bindings::kunit_suite = + // SAFETY: `KUNIT_TEST_SUITE` is static. + unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) }; + }; + }; +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index e1065a7551a3..02a70c5ad8ee 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -18,6 +18,7 @@ #![feature(inline_const)] #![feature(lint_reasons)] #![feature(unsize)] +#![feature(const_mut_refs)]
// Ensure conditional compilation based on the kernel configuration works; // otherwise we may silently break things like initcall handling.
On Fri, Dec 13, 2024 at 9:10 AM David Gow davidgow@google.com wrote:
+/// The test case should have the signature +/// `unsafe extern "C" fn test_case(test: *mut crate::bindings::kunit)`.
I guess this is here so that people can copy-paste it (i.e. users)? However, given it is private (i.e. users are not expected to manually use this function) and that it is already in the signature (`run_case`), I wonder if we need to mention it this paragraph.
Moreover, does it need to be `unsafe fn`? Safe ones coerce into unsafe ones (i.e. not in the parameter, but in the one that the user defines)
+/// Use `kunit_case_null` to generate such a delimeter.
Typo: delimiter
+/// function retuns such a delimiter.
Typo: returns
+/// Registers a KUnit test suite. +/// +/// # Safety +/// +/// `test_cases` must be a NULL terminated array of test cases.
I guess "test cases" here also means "valid ones", i.e. without invalid pointers and so on inside, right? Perhaps it is good to at least mention "valid" clearly. Otherwise, one can read it as "any possible instance of `kunit_case`", which would be unsound, no?
We could go finer-grained, and explain exactly what needs to be valid, which would be good.
A third alternative would be to mention that these test cases must be created using the two functions above (`kunit_case*()`), and make the `kunit_case()` one require a suitable `run_case` function pointer (i.e. making it `unsafe`). That way the requirements are a bit more localized, even if it means making that one `unsafe fn`.
+/// unsafe extern "C" fn test_fn(_test: *mut crate::bindings::kunit) {
Does this function need to be `unsafe`? (please see above for the comment on the docs of `kunit_case`). If so, then we would need a `# Safety` section if the example was built (see below).
+/// static mut KUNIT_TEST_CASE: crate::bindings::kunit_case = crate::kunit_case(name, test_fn);
Shouldn't this `name` be a `CStr` for this example? (The example is ignored, but it would be ideal to keep it up to date).
+/// static mut KUNIT_TEST_CASES: &mut[crate::bindings::kunit_case] = unsafe { +/// &mut[KUNIT_TEST_CASE, KUNIT_NULL_CASE] +/// };
These should have a space after `&mut`. However, why do we need the mutable reference here anyway? (please see discussion below and in the next patch)
In addition, doesn't this end up with duplicated statics? i.e. one in the array, and an independent one. So we can instead put them directly into the array, which would avoid `unsafe` in the initializer too. (This applies to the generation in the next patch, too).
Finally, should we make this documentation `#[doc(hidden)]` (but keeping the docs)? i.e. it is not expected to be used by users directly anyway (and currently the example wouldn't work, since e.g. `kunit_case` is private).
Speaking of the example, we could fix it and make it non-`ignore` (assuming we made `kunit_case*` public which we will probably eventually need, in which case they should also be `#[doc(hidden)]`), e.g.:
/// ``` /// extern "C" fn test_fn(_test: *mut kernel::bindings::kunit) { /// let actual = 1 + 1; /// let expected = 2; /// assert_eq!(actual, expected); /// } /// /// static mut KUNIT_TEST_CASES: [kernel::bindings::kunit_case; 2] = [ /// kernel::kunit::kunit_case(kernel::c_str!("name"), test_fn), /// kernel::kunit::kunit_case_null(), /// ]; /// kernel::kunit_unsafe_test_suite!(suite_name, KUNIT_TEST_CASES); /// ```
(If so, we should then use explicit names, since otherwise the KUnit output in the log is confusing.)
static KUNIT_TEST_SUITE_NAME: [i8; 256] = {
This should probably be `::kernel::ffi::c_char` instead (and then the cast below needs a change too).
And I think we can make this a `const` instead, since we just use it to initialize the array in the `static mut`, no?
let name_u8 = core::stringify!($name).as_bytes();
Since it is a macro, I would use absolute paths, e.g. `::core::...`.
static mut KUNIT_TEST_SUITE: $crate::bindings::kunit_suite =$crate::bindings::kunit_suite {name: KUNIT_TEST_SUITE_NAME,// SAFETY: User is expected to pass a correct `test_cases`, hence this macro
Nit: usually we say "by the safety preconditions" or similar, instead of "User is expected to pass...".
// named 'unsafe'.
`unsafe`. (Also, not an English speaker, but should this be "is named" instead?)
test_cases: unsafe { $test_cases.as_mut_ptr() },
This warns due to `static_mut_refs` starting Rust 1.83:
error: creating a mutable reference to mutable static is discouraged --> rust/kernel/kunit.rs:261:42 | 261 | test_cases: unsafe { $test_cases.as_mut_ptr() }, | ^^^^^^^^^^^ mutable reference to mutable static
https://doc.rust-lang.org/nightly/edition-guide/rust-2024/static-mut-referen...
It can still be `allow`ed; however, doesn't the call to `as_mut_ptr()` create an intermediate mutable reference that we should avoid anyway?
Instead, can we have an array static directly, rather than a mutable reference static to an array, and use `addr_of_mut!` here? Then we also avoid adding the `const_mut_refs` feature (even if it is stable now).
That is, we would have (with all the feedback in place) something like:
static mut TEST_CASES: [kernel::bindings::kunit_case; 3] = [ kernel::kunit::kunit_case(kernel::c_str!("foo"), kunit_rust_wrapper_foo), kernel::kunit::kunit_case(kernel::c_str!("bar"), kunit_rust_wrapper_bar), kernel::kunit::kunit_case_null(), ];
And then:
test_cases: unsafe { ::core::ptr::addr_of_mut!($test_cases).cast::<::kernel::bindings::kunit_case>() },
So no mutable references in sight.
#![feature(unsize)] +#![feature(const_mut_refs)]
The list should be kept sorted.
However, we can avoid enabling the feature I think, if we avoid the `&mut`s (please see above).
Cheers, Miguel
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 v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-3-davidgow@go... - Rebased against 6.13-rc1 - "Expect" that the sample assert_eq!(1+1, 2) produces a clippy warning due to a redundant assertion. (Thanks Boqun, Miguel)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-6-davidgow@go... - 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@go... - 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 | 16 ++++- rust/macros/kunit.rs | 168 +++++++++++++++++++++++++++++++++++++++++++ rust/macros/lib.rs | 29 ++++++++ 4 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 rust/macros/kunit.rs
diff --git a/MAINTAINERS b/MAINTAINERS index e6e71b05710b..cbe5a99eefea 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -12562,6 +12562,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 5003282cb4c4..a92f12da77d5 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. @@ -272,10 +274,22 @@ macro_rules! kunit_unsafe_test_suite { };
#[used] + #[allow(unused_unsafe)] #[link_section = ".kunit_test_suites"] static mut KUNIT_TEST_SUITE_ENTRY: *const $crate::bindings::kunit_suite = // SAFETY: `KUNIT_TEST_SUITE` is static. - unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) }; + unsafe { + core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) + }; }; }; } + +#[kunit_tests(rust_kernel_kunit)] +mod tests { + #[test] + fn rust_test_kunit_example_test() { + #![expect(clippy::eq_op)] + 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 4ab94e44adfe..beff19cc6d16 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; @@ -492,3 +493,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) +}
On Fri, Dec 13, 2024 at 9:10 AM David Gow davidgow@google.com wrote:
#[allow(unused_unsafe)]
Should this be in the first patch?
unsafe { core::ptr::addr_of_mut!(KUNIT_TEST_SUITE) };
unsafe {core::ptr::addr_of_mut!(KUNIT_TEST_SUITE)};
Spurious change?
I thought this should perhaps have been in the previous patch directly, but running `rustfmt` shows the previous patch is the correct formatting.
`rustfmt` also needs to be run for these files -- it reveals a few more changes needed.
+//! Copyright (c) 2023 José Expósito jose.exposito89@gmail.com
(This is not something we need to change now, since it is orthogonal and debatable, but I think copyright lines should not be in the generated documentation unless we really want contact points in docs. I think it could go in a `//` comment after the `SPDX` line instead.)
+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)- }
We could do the `to_string()` step once creating a single string (and we can keep it as a `String`, too):
let attr = attr.to_string();
- // Scan for the "mod" keyword.
Formatting: `mod`
.expect("#[kunit_tests(test_name)] attribute should only be applied to modules");
Formatting: `#[kunit_tests(test_name)]`
_ => panic!("cannot locate main body of module"),
Formatting: Cannot
- // #[test]
- // fn bar() {
- // assert_eq!(2, 2);
- // }
- // ```
Missing closing `}` of the `mod`.
- // static mut KUNIT_CASE_FOO: kernel::bindings::kunit_case =
- // kernel::kunit::kunit_case(foo, kunit_rust_wrapper_foo);
I think this is not in sync with the generation, i.e. missing kernel::c_str!("foo")
(and ditto for the other one)
- // static mut TEST_CASES : &mut[kernel::bindings::kunit_case] = unsafe {
Formatting: extra space and missing space.
- // &mut [KUNIT_CASE_FOO, KUNIT_CASE_BAR, KUNIT_CASE_NULL]
- // };
- //
- // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES);
- // ```
With the suggestions from the previous patch (i.e. avoiding unused/duplicated `static`s, intermediate mutable references, unstable feature and `unsafe`), we can simplify the entire example down to:
// ``` // unsafe extern "C" fn kunit_rust_wrapper_foo(_test: *mut kernel::bindings::kunit) { foo(); } // unsafe extern "C" fn kunit_rust_wrapper_bar(_test: * mut kernel::bindings::kunit) { bar(); } // // static mut TEST_CASES: [kernel::bindings::kunit_case; 3] = [ // kernel::kunit::kunit_case(kernel::c_str!("foo"), kunit_rust_wrapper_foo), // kernel::kunit::kunit_case(kernel::c_str!("bar"), kunit_rust_wrapper_bar), // kernel::kunit::kunit_case_null() // ]; // // kernel::kunit_unsafe_test_suite!(kunit_test_suit_name, TEST_CASES); // ```
...with the corresponding removals in the actual generation code below, of course.
Cheers, Miguel
From: José Expósito jose.exposito89@gmail.com
In some cases, we need to call test-only code from outside the test case, for example, to mock a function or a module.
In order to check whether we are in a test or not, we need to test if `CONFIG_KUNIT` is set. Unfortunately, we cannot rely only on this condition because: - a test could be running in another thread, - some distros compile KUnit in production kernels, so checking at runtime that `current->kunit_test != NULL` is required.
Forturately, KUnit provides an optimised check in `kunit_get_current_test()`, which checks CONFIG_KUNIT, a global static key, and then the current thread's running KUnit test.
Add a safe wrapper function around this to know whether or not we are in a KUnit test and examples showing how to mock a function and a module.
Signed-off-by: José Expósito jose.exposito89@gmail.com Co-developed-by: David Gow davidgow@google.com Signed-off-by: David Gow davidgow@google.com --- Changes since v4: https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@go... - Rebased against 6.13-rc1 - Fix some missing safety comments, and remove some unneeded 'unsafe' blocks. (Thanks Boqun)
Changes since v3: https://lore.kernel.org/linux-kselftest/20241030045719.3085147-8-davidgow@go... - The example test has been updated to no longer use assert_eq!() with a constant bool argument (fixes a clippy warning).
No changes since v2: https://lore.kernel.org/linux-kselftest/20241029092422.2884505-4-davidgow@go...
Changes since v1: https://lore.kernel.org/lkml/20230720-rustbind-v1-3-c80db349e3b5@google.com/ - Rebased on top of rust-next. - Use the `kunit_get_current_test()` C function, which wasn't previously available, instead of rolling our own. - (Thanks also to Boqun for suggesting a nicer way of implementing this, which I tried, but the `kunit_get_current_test()` version obsoleted.)
--- rust/kernel/kunit.rs | 75 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+)
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index a92f12da77d5..2196e35e5d75 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -285,11 +285,86 @@ macro_rules! kunit_unsafe_test_suite { }; }
+/// In some cases, you need to call test-only code from outside the test case, for example, to +/// create a function mock. This function can be invoked to know whether we are currently running a +/// KUnit test or not. +/// +/// # Examples +/// +/// This example shows how a function can be mocked to return a well-known value while testing: +/// +/// ``` +/// # use kernel::kunit::in_kunit_test; +/// # +/// fn fn_mock_example(n: i32) -> i32 { +/// if in_kunit_test() { +/// 100 +/// } else { +/// n + 1 +/// } +/// } +/// +/// let mock_res = fn_mock_example(5); +/// assert_eq!(mock_res, 100); +/// ``` +/// +/// Sometimes, you don't control the code that needs to be mocked. This example shows how the +/// `bindings` module can be mocked: +/// +/// ``` +/// // Import our mock naming it as the real module. +/// #[cfg(CONFIG_KUNIT)] +/// use bindings_mock_example as bindings; +/// +/// // This module mocks `bindings`. +/// mod bindings_mock_example { +/// use kernel::kunit::in_kunit_test; +/// use kernel::bindings::u64_; +/// +/// // Make the other binding functions available. +/// pub(crate) use kernel::bindings::*; +/// +/// /// Mock `ktime_get_boot_fast_ns` to return a well-known value when running a KUnit test. +/// pub(crate) fn ktime_get_boot_fast_ns() -> u64_ { +/// if in_kunit_test() { +/// 1234 +/// } else { +/// // SAFETY: ktime_get_boot_fast_ns() is safe to call, and just returns a u64. +/// // Additionally, this is never actually called in this example, as we're in a test +/// // and it's mocked out. +/// unsafe { kernel::bindings::ktime_get_boot_fast_ns() } +/// } +/// } +/// } +/// +/// // This is the function we want to test. Since `bindings` has been mocked, we can use its +/// // functions seamlessly. +/// fn get_boot_ns() -> u64 { +/// bindings::ktime_get_boot_fast_ns() +/// } +/// +/// let time = get_boot_ns(); +/// assert_eq!(time, 1234); +/// ``` +pub fn in_kunit_test() -> bool { + // SAFETY: kunit_get_current_test() is always safe to call from C (it has fallbacks for + // when KUnit is not enabled), and we're only comparing the result to NULL. + unsafe { !bindings::kunit_get_current_test().is_null() } +} + #[kunit_tests(rust_kernel_kunit)] mod tests { + use super::*; + #[test] fn rust_test_kunit_example_test() { #![expect(clippy::eq_op)] assert_eq!(1 + 1, 2); } + + #[test] + fn rust_test_kunit_in_kunit_test() { + let in_kunit = in_kunit_test(); + assert!(in_kunit); + } }
On Fri, Dec 13, 2024 at 9:10 AM David Gow davidgow@google.com wrote:
+/// In some cases, you need to call test-only code from outside the test case, for example, to +/// create a function mock. This function can be invoked to know whether we are currently running a +/// KUnit test or not.
The documentation of items use the first paragraph as a "short description" in some places, so ideally it should be as short as possible (e.g. one line), similar to Git commit titles.
So what about:
/// Returns whether we are currently running a KUnit test. /// /// In some cases, you need to call test-only code from outside the test case, for example, to /// create a function mock. This function allows to change behavior depending on whether we are /// currently running a KUnit test or not.
I tweaked the second sentence to avoid repetition, and to take the chance to mention "allows to change behavior" instead, since that is what we want to achieve.
+/// #
Nit: currently the style we use doesn't keep empty `#` lines to separate.
+/// fn fn_mock_example(n: i32) -> i32 { +/// if in_kunit_test() { +/// 100 +/// } else { +/// n + 1 +/// } +/// }
Early return would look better here since we really want to do something completely different, and would avoid indentation in the "normal code".
+/// // This module mocks `bindings`.
This could perhaps be documentation (`///`), but either way it is fine.
+/// mod bindings_mock_example {
Could this get a `#[cfg(CONFIG_KUNIT)]` too?
+/// if in_kunit_test() { +/// 1234 +/// } else { +/// // SAFETY: ktime_get_boot_fast_ns() is safe to call, and just returns a u64.
Formatting: `ktime_get_boot_fast_ns()` and `u64`
Perhaps emphasize with "always safe"?
Also, why does the `u64` need to be part of the safety comment?
+/// // Additionally, this is never actually called in this example, as we're in a test +/// // and it's mocked out.
If the function is safe to call, should we have this as part of the `SAFETY` comment then? We can move it above, if we need to keep it, or we could just remove it.
In any case, if the `else` is dead code, why do we have it? i.e. shouldn't the mock just return the 1234 value? (see below)
+/// // This is the function we want to test. Since `bindings` has been mocked, we can use its +/// // functions seamlessly. +/// fn get_boot_ns() -> u64 { +/// bindings::ktime_get_boot_fast_ns()
I think this wouldn't work: `ktime_get_boot_fast_ns()` is unsafe when `CONFIG_KUNIT` is disabled, so it wouldn't build for an actual user.
Unless I am missing something, mocks should keep the `unsafe` status (i.e. in general, the signature should be kept the same), and the `SAFETY` comment should be here, i.e. in the "normal code", not above in the mock (we should probably mention this as a guideline in `Documentation/rust/testing.rst` too, when the docs are added there for this).
And by doing that, we can remove all the usage of `bindings` inside the mocking module, and we keep the "normal code" looking as normal as possible, i.e. we don't hide `// SAFETY` comments inside mocking modules.
With all put together, we get something like this:
/// ``` /// // Import our mock naming it as the real module. /// #[cfg(CONFIG_KUNIT)] /// use bindings_mock_example as bindings; /// #[cfg(not(CONFIG_KUNIT))] /// use kernel::bindings; /// /// // This module mocks `bindings`. /// #[cfg(CONFIG_KUNIT)] /// mod bindings_mock_example { /// /// Mock `ktime_get_boot_fast_ns` to return a well-known value when running a KUnit test. /// pub(crate) fn ktime_get_boot_fast_ns() -> u64 { /// 1234 /// } /// } /// /// // This is the function we want to test. Since `bindings` has been mocked, we can use its /// // functions seamlessly. /// fn get_boot_ns() -> u64 { /// // SAFETY: `ktime_get_boot_fast_ns()` is always safe to call. /// unsafe { bindings::ktime_get_boot_fast_ns() } /// } /// /// let time = get_boot_ns(); /// assert_eq!(time, 1234); /// ```
I added a `#[cfg(CONFIG_KUNIT)]` for the mocking module here, like for the other example.
+pub fn in_kunit_test() -> bool {
- // SAFETY: kunit_get_current_test() is always safe to call from C (it has fallbacks for
Formatting: `kunit_get_current_test()`
Also, I think we should remove "from C" since it may be confusing -- or what is it trying to say here? i.e. it is always safe to call from both C and Rust, no? Or is there something I am missing?
- // when KUnit is not enabled), and we're only comparing the result to NULL.
Does "and we're only comparing the result to NULL" need to be part of the safety comment? i.e. comparing a pointer is safe (and `is_null()` too).
let in_kunit = in_kunit_test();assert!(in_kunit);
I would simplify and call directly the function.
Cheers, Miguel
On Fri, Dec 13, 2024 at 9:10 AM David Gow davidgow@google.com wrote:
v5 here is a small set of fixes and a rebase of the previous versions. If there are no major issues, I'd like to land this soon so it can be used and tested ready for 6.14.
Thanks as usual David for keeping this one alive over time.
I was thinking of applying this or giving you an Acked-by so that you take it, but I ended up noticing a few things that I think need fixing (the recommended mocking wouldn't work, warns/errors with 1.83, duplicated/unused `static`s, intermediate mutable references being created), so I sent a review. The good news is that we can simplify the code (especially the generated one) while fixing those at the same time. I hope the review helps.
I have put the review result here too (in a single commit, since I was testing and didn't expect to fixup, sorry), in case it helps to see the final result:
https://github.com/ojeda/linux/commit/151681df53ac8ad52086f6758b51c6fb441442...
If you agree with the changes (at least the big ones, i.e. that I didn't miss something), then I think we can take it through KUnit or Rust, though it may be a good idea to have the result in the list for a few days (and/or put it early next cycle) given the magnitude of the changes.
Finally, it is not a blocker for 6.14 or otherwise, but we should eventually add/explain the new features in `Documentation/rust/testing.rst`.
Thanks again!
Cheers, Miguel
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Miguel Ojeda