Message-ID: <CANiq72=A2kT4ZHCH2c7DVv+WQpH+c2vKcib-Kh7=6LWyUv9Xaw@mail.gmail.com>
Date: Fri, 3 Jan 2025 16:43:15 +0100
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: David Gow <davidgow@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, José Expósito <jose.exposito89@...il.com>, 
	Rae Moar <rmoar@...gle.com>, Boqun Feng <boqun.feng@...il.com>, 
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, 
	Benno Lossin <benno.lossin@...ton.me>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Alice Ryhl <aliceryhl@...gle.com>, Matt Gilbride <mattgilbride@...gle.com>, 
	Brendan Higgins <brendan.higgins@...ux.dev>, kunit-dev@...glegroups.com, 
	linux-kselftest@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] rust: kunit: allow to know if we are in a test

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

Powered by blists - more mailing lists