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

[Miguel Ojeda]

On Fri, Dec 13, 2024 at 9:10 AM David Gow <davidgow@xxxxxxxxxx> 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@xxxxxxxxx>

(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