diff mbox series

[v6,3/3] rust: kunit: allow to know if we are in a test

Message ID 20250214074051.1619256-4-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 Feb. 14, 2025, 7:40 a.m. UTC
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>
Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---

Changes since v5:
https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
- Greatly improved documentation, which is both clearer and better
  matches the rustdoc norm. (Thanks, Miguel)
- The examples and safety comments are also both more idiomatic an
  cleaner. (Thanks, Miguel)
- More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)

Changes since v4:
https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@google.com/
- 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@google.com/
- 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@google.com/

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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

Comments

Tamir Duberstein Feb. 14, 2025, 2:41 p.m. UTC | #1
On Fri, Feb 14, 2025 at 2:42 AM David Gow <davidgow@google.com> wrote:
>
> 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>
> Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
>
> Changes since v5:
> https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
> - Greatly improved documentation, which is both clearer and better
>   matches the rustdoc norm. (Thanks, Miguel)
> - The examples and safety comments are also both more idiomatic an
>   cleaner. (Thanks, Miguel)
> - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
>
> Changes since v4:
> https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@google.com/
> - 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@google.com/
> - 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@google.com/
>
> 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> index 9e27b74a605b..3aad7a281b6d 100644
> --- a/rust/kernel/kunit.rs
> +++ b/rust/kernel/kunit.rs
> @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
>      };
>  }
>
> +/// 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.
> +///
> +/// # 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() {
> +///         return 100;
> +///     }
> +///
> +///     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:

[`bindings`] here, please. There are two more instances below but
those aren't doc comments, so I don't think bracketing them will do
anything.

> +///
> +/// ```
> +/// // 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);
> +/// ```

Isn't this swapping out the bindings module at compile time, and for
the whole build? In other words cfg(CONFIG_KUNIT) will apply to all
code, both test and non-test.

> +pub fn in_kunit_test() -> bool {
> +    // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
> +    // when KUnit is not enabled).
> +    unsafe { !bindings::kunit_get_current_test().is_null() }

Nit if you care about reducing unsafe blocks:

!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() {
> +        assert!(in_kunit_test());
> +    }
>  }
> --
> 2.48.1.601.g30ceb7b040-goog
>
>
David Gow Feb. 15, 2025, 9:03 a.m. UTC | #2
On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Fri, Feb 14, 2025 at 2:42 AM David Gow <davidgow@google.com> wrote:
> >
> > 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>
> > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > ---
> >
> > Changes since v5:
> > https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
> > - Greatly improved documentation, which is both clearer and better
> >   matches the rustdoc norm. (Thanks, Miguel)
> > - The examples and safety comments are also both more idiomatic an
> >   cleaner. (Thanks, Miguel)
> > - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
> >
> > Changes since v4:
> > https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@google.com/
> > - 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@google.com/
> > - 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@google.com/
> >
> > 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 66 insertions(+)
> >
> > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > index 9e27b74a605b..3aad7a281b6d 100644
> > --- a/rust/kernel/kunit.rs
> > +++ b/rust/kernel/kunit.rs
> > @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
> >      };
> >  }
> >
> > +/// 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.
> > +///
> > +/// # 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() {
> > +///         return 100;
> > +///     }
> > +///
> > +///     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:
>
> [`bindings`] here, please. There are two more instances below but
> those aren't doc comments, so I don't think bracketing them will do
> anything.
>

Done in v7. Alas, I'll have to keep getting used to the differences
between kerneldoc and rustdoc...

> > +///
> > +/// ```
> > +/// // 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);
> > +/// ```
>
> Isn't this swapping out the bindings module at compile time, and for
> the whole build? In other words cfg(CONFIG_KUNIT) will apply to all
> code, both test and non-test.
>

I believe so, so this is probably something best done only in test files.

Ideally, we'd have support for something like the KUnit function
mocking features here, but that's horribly C-specific at the moment.

> > +pub fn in_kunit_test() -> bool {
> > +    // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
> > +    // when KUnit is not enabled).
> > +    unsafe { !bindings::kunit_get_current_test().is_null() }
>
> Nit if you care about reducing unsafe blocks:
>
> !unsafe { bindings::kunit_get_current_test() }.is_null()
>
>

Huh, I thought this wouldn't work, but it's working fine for me here,
so I've made the change.

Thanks!

> > +}
> > +
> >  #[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() {
> > +        assert!(in_kunit_test());
> > +    }
> >  }
> > --
> > 2.48.1.601.g30ceb7b040-goog
> >
> >

Thanks a lot, these should be fixed in v7.

Cheers,
-- David
Tamir Duberstein Feb. 15, 2025, 1:04 p.m. UTC | #3
On Sat, Feb 15, 2025 at 4:03 AM David Gow <davidgow@google.com> wrote:
>
> On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
> >
> > On Fri, Feb 14, 2025 at 2:42 AM David Gow <davidgow@google.com> wrote:
> > >
> > > 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>
> > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > ---
> > >
> > > Changes since v5:
> > > https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
> > > - Greatly improved documentation, which is both clearer and better
> > >   matches the rustdoc norm. (Thanks, Miguel)
> > > - The examples and safety comments are also both more idiomatic an
> > >   cleaner. (Thanks, Miguel)
> > > - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
> > >
> > > Changes since v4:
> > > https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@google.com/
> > > - 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@google.com/
> > > - 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@google.com/
> > >
> > > 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 66 insertions(+)
> > >
> > > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > > index 9e27b74a605b..3aad7a281b6d 100644
> > > --- a/rust/kernel/kunit.rs
> > > +++ b/rust/kernel/kunit.rs
> > > @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
> > >      };
> > >  }
> > >
> > > +/// 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.
> > > +///
> > > +/// # 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() {
> > > +///         return 100;
> > > +///     }
> > > +///
> > > +///     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:
> >
> > [`bindings`] here, please. There are two more instances below but
> > those aren't doc comments, so I don't think bracketing them will do
> > anything.
> >
>
> Done in v7. Alas, I'll have to keep getting used to the differences
> between kerneldoc and rustdoc...
>
> > > +///
> > > +/// ```
> > > +/// // 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);
> > > +/// ```
> >
> > Isn't this swapping out the bindings module at compile time, and for
> > the whole build? In other words cfg(CONFIG_KUNIT) will apply to all
> > code, both test and non-test.
> >
>
> I believe so, so this is probably something best done only in test files.

Why would you need conditional compilation of this kind in test files?

What I was getting at with this comment is that this example might
mislead a user to think that this is how they should imbue their code
with test-specific behavior, which is not what this will do. Instead
this would break kernels built with CONFIG_KUNIT, which I think is not
where we want to be going. Right?

>
> Ideally, we'd have support for something like the KUnit function
> mocking features here, but that's horribly C-specific at the moment.
>
> > > +pub fn in_kunit_test() -> bool {
> > > +    // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
> > > +    // when KUnit is not enabled).
> > > +    unsafe { !bindings::kunit_get_current_test().is_null() }
> >
> > Nit if you care about reducing unsafe blocks:
> >
> > !unsafe { bindings::kunit_get_current_test() }.is_null()
> >
> >
>
> Huh, I thought this wouldn't work, but it's working fine for me here,
> so I've made the change.
>
> Thanks!
>
> > > +}
> > > +
> > >  #[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() {
> > > +        assert!(in_kunit_test());
> > > +    }
> > >  }
> > > --
> > > 2.48.1.601.g30ceb7b040-goog
> > >
> > >
>
> Thanks a lot, these should be fixed in v7.
>
> Cheers,
> -- David
David Gow Feb. 18, 2025, 8:51 a.m. UTC | #4
On Sat, 15 Feb 2025 at 21:04, Tamir Duberstein <tamird@gmail.com> wrote:
>
> On Sat, Feb 15, 2025 at 4:03 AM David Gow <davidgow@google.com> wrote:
> >
> > On Fri, 14 Feb 2025 at 22:41, Tamir Duberstein <tamird@gmail.com> wrote:
> > >
> > > On Fri, Feb 14, 2025 at 2:42 AM David Gow <davidgow@google.com> wrote:
> > > >
> > > > 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>
> > > > Co-developed-by: Miguel Ojeda <ojeda@kernel.org>
> > > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > > ---
> > > >
> > > > Changes since v5:
> > > > https://lore.kernel.org/all/20241213081035.2069066-4-davidgow@google.com/
> > > > - Greatly improved documentation, which is both clearer and better
> > > >   matches the rustdoc norm. (Thanks, Miguel)
> > > > - The examples and safety comments are also both more idiomatic an
> > > >   cleaner. (Thanks, Miguel)
> > > > - More things sit appropriately behind CONFIG_KUNIT (Thanks, Miguel)
> > > >
> > > > Changes since v4:
> > > > https://lore.kernel.org/linux-kselftest/20241101064505.3820737-4-davidgow@google.com/
> > > > - 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@google.com/
> > > > - 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@google.com/
> > > >
> > > > 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 | 66 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 66 insertions(+)
> > > >
> > > > diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
> > > > index 9e27b74a605b..3aad7a281b6d 100644
> > > > --- a/rust/kernel/kunit.rs
> > > > +++ b/rust/kernel/kunit.rs
> > > > @@ -286,11 +286,77 @@ macro_rules! kunit_unsafe_test_suite {
> > > >      };
> > > >  }
> > > >
> > > > +/// 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.
> > > > +///
> > > > +/// # 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() {
> > > > +///         return 100;
> > > > +///     }
> > > > +///
> > > > +///     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:
> > >
> > > [`bindings`] here, please. There are two more instances below but
> > > those aren't doc comments, so I don't think bracketing them will do
> > > anything.
> > >
> >
> > Done in v7. Alas, I'll have to keep getting used to the differences
> > between kerneldoc and rustdoc...
> >
> > > > +///
> > > > +/// ```
> > > > +/// // 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);
> > > > +/// ```
> > >
> > > Isn't this swapping out the bindings module at compile time, and for
> > > the whole build? In other words cfg(CONFIG_KUNIT) will apply to all
> > > code, both test and non-test.
> > >
> >
> > I believe so, so this is probably something best done only in test files.
>
> Why would you need conditional compilation of this kind in test files?
>
> What I was getting at with this comment is that this example might
> mislead a user to think that this is how they should imbue their code
> with test-specific behavior, which is not what this will do. Instead
> this would break kernels built with CONFIG_KUNIT, which I think is not
> where we want to be going. Right?
>

Hmm... I think I get this now. (I confess, I'm still wrapping my head
around the crate/module/file boundaries in Rust, so assumed there must
be something clever going on.)

I'd definitely rather CONFIG_KUNIT not break a kernel, particularly
since some distros do enable it (at least as a module) by default. So,
at the very least, this should be behind a more specific kconfig. But
it'd be better still to not break anything at all.

José: am I missing something here, or does this need either changing
or removing?

Maybe we should just have the 'mock' version include something like:
if in_kunit_test() {
    1234
} else {
    bindings::ktime_get_boot_fast_ns()
}

And in the long term, we can try to hook into the static stubbing
framework to allow this to be turned on and off for individual
functions. (I imagine we could get close with macros, which is what we
do in C, though the thought of trying to deal with function pointers
and Rust's lack of stable ABI here scares me.)

Either way, we're definitely going to need a follow-up documentation
patch for this whole feature, so if we want to get rid of or update
this example, we can either do it then, or now.

I'm afraid I'm unlikely to have time to work on Rust stuff probably
for the rest of the month, so I'll let the discussion play out (and if
people desperately want the patches before then, I'd be happy to fix
them in follow-ups).

Cheers,
-- David
diff mbox series

Patch

diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs
index 9e27b74a605b..3aad7a281b6d 100644
--- a/rust/kernel/kunit.rs
+++ b/rust/kernel/kunit.rs
@@ -286,11 +286,77 @@  macro_rules! kunit_unsafe_test_suite {
     };
 }
 
+/// 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.
+///
+/// # 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() {
+///         return 100;
+///     }
+///
+///     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;
+/// #[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);
+/// ```
+pub fn in_kunit_test() -> bool {
+    // SAFETY: `kunit_get_current_test()` is always safe to call (it has fallbacks for
+    // when KUnit is not enabled).
+    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() {
+        assert!(in_kunit_test());
+    }
 }