| Message ID | 20241213081035.2069066-4-davidgow@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | rust: kunit: Support KUnit tests with a user-space like syntax | expand |
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
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); + } }