On Sat, Nov 19, 2022 at 12:13 AM 'David Gow' via KUnit Development kunit-dev@googlegroups.com wrote:
In order to detect if a KUnit test is running, and to access its context, the 'kunit_test' member of the current task_struct is used. Usually, this is accessed directly or via the kunit_fail_current_task() function.
In order to speed up the case where no test is running, add a wrapper, kunit_get_current_test(), which uses the static key to fail early. Equally, Speed up kunit_fail_current_test() by using the static key.
This should make it convenient for code to call this unconditionally in fakes or error paths, without worrying that this will slow the code down significantly.
If CONFIG_KUNIT=n (or m), this compiles away to nothing. If CONFIG_KUNIT=y, it will compile down to a NOP (on most architectures) if no KUnit test is currently running.
Note that kunit_get_current_test() does not work if KUnit is built as a module. This mirrors the existing restriction on kunit_fail_current_test().
Note that the definition of kunit_fail_current_test() still wraps an empty, inline function if KUnit is not built-in. This is to ensure that the printf format string __attribute__ will still work.
Also update the documentation to suggest users use the new kunit_get_current_test() function, update the example, and to describe the behaviour when KUnit is disabled better.
Cc: Jonathan Corbet corbet@lwn.net Cc: Sadiya Kazi sadiyakazi@google.com Signed-off-by: David Gow davidgow@google.com
Reviewed-by: Daniel Latypov dlatypov@google.com
Looks good to me, but some small optional nits about the Documentation.
I'm a bit sad that the kunit_fail_current_test() macro is now a bit more complicated (has two definitions). Optional: perhaps it's long enough now that we should have a comment after the #endif, i.e. #endif /* IS_BUILTIN(CONFIG_KUNIT) */
<snip>
diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst index 2737863ef365..e70014b82350 100644 --- a/Documentation/dev-tools/kunit/usage.rst +++ b/Documentation/dev-tools/kunit/usage.rst @@ -625,17 +625,21 @@ as shown in next section: *Accessing The Current Test*. Accessing The Current Test
-In some cases, we need to call test-only code from outside the test file. -For example, see example in section *Injecting Test-Only Code* or if -we are providing a fake implementation of an ops struct. Using -``kunit_test`` field in ``task_struct``, we can access it via -``current->kunit_test``. +In some cases, we need to call test-only code from outside the test file, +for example, when providing a fake implementation of a function, or to fail
nit: there are two spaces after "for example, "
Personal preference: I'd rather keep "For example," as the start of a new sentence.
+any current test from within an error handler. +We can do this via the ``kunit_test`` field in ``task_struct``, which we can +access using the ``kunit_get_current_test`` function in ``kunit/test-bug.h``.
Personal preference: kunit_get_current_test() IMO that would make it easier to pick up when the reader is quickly scanning over.
-The example below includes how to implement "mocking": +``kunit_get_current_test`` requires KUnit be built-in to the kernel, i.e. +``CONFIG_KUNIT=y``. It is safe to call even if KUnit is not enabled, is built as a module, +or no test is currently running, in which case it will quickly return ``NULL``.
I find this sentence a bit confusing.
I think it's trying to convey that * it can be called no matter how the kernel is built or what cmdline args are given * but it doesn't work properly for CONFIG_KUNIT=m * for CONFIG_KUNIT=n, it's a static inline func that just returns NULL * when booting with `kunit.enabled=0`, it's fast (thanks to static keys)
But the current wording basically says "the func requires CONFIG_KUNIT=y" then says it's safe to call it even if CONFIG_KUNIT=n. It feels a bit whiplashy.
Should this be reworded to say the function can be used regardless of whether KUnit is enabled but add a `note` block about how it doesn't properly for CONFIG_KUNIT=m?
+The example below uses this to implement a "mock" implementation of a function, ``foo``:
.. code-block:: c
#include <linux/sched.h> /* for current */
#include <kunit/test-bug.h> /* for kunit_get_current_test */ struct test_data { int foo_result;@@ -644,7 +648,7 @@ The example below includes how to implement "mocking":
static int fake_foo(int arg) {
struct kunit *test = current->kunit_test;
struct kunit *test = kunit_get_current_test(); struct test_data *test_data = test->priv; KUNIT_EXPECT_EQ(test, test_data->want_foo_called_with, arg);@@ -675,7 +679,7 @@ Each test can have multiple resources which have string names providing the same flexibility as a ``priv`` member, but also, for example, allowing helper functions to create resources without conflicting with each other. It is also possible to define a clean up function for each resource, making it easy to -avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/test.rst. +avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/resource.rst.
Oops, thanks for cleaning this up. I guess I forgot to update this when splitting out resource.rst or my change raced with the rewrite of this file.
Failing The Current Test
@@ -703,3 +707,6 @@ structures as shown below: static void my_debug_function(void) { } #endif
+Note that ``kunit_fail_current_test`` requires KUnit be built-in to the kernel, i.e. +``CONFIG_KUNIT=y``. It is safe to call even if KUnit is not enabled, is built as a module, +or no test is currently running, but will do nothing.
This is the same wording as above. I think it's clearer since what it's trying to convey is simpler, so it's probably fine.
But if we do end up thinking of a good way to reword the previous bit, we might want to reword it here too.