On Fri, Dec 13, 2024 at 9:10 AM David Gow <[email protected]> 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
