Re: [PATCH v5 2/3] rust: macros: add macro to easily run KUnit tests

Miguel Ojeda
2025-01-03T15:39:53Z

> + #[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