[PATCH] Documentation: kunit: Fix kunit_device_register() example

[Robin Murphy]

kunit_device_register() only returns error pointers, not NULL.
Furthermore for regular users who aren't testing the KUnit API
itself, errors most likely represent major system failure (e.g. OOM
or sysfs collision) beyond the scope of their own test conditions.
Replace the assert with straightforward error handling for clarity.

Signed-off-by: Robin Murphy <robin....@arm.com>
---
This seemed the logical conclusion by inspection, but please do correct
me if I've misunderstood the intent...
---
Documentation/dev-tools/kunit/usage.rst | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
index 038f480074fd..3452c739dd44 100644
--- a/Documentation/dev-tools/kunit/usage.rst
+++ b/Documentation/dev-tools/kunit/usage.rst
@@ -873,7 +873,8 @@ For example:

// Create a fake device.
fake_device = kunit_device_register(test, "my_device");
- KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_device)
+ if (IS_ERR(fake_device))
+ return;

// Pass it to functions which need a device.
dev_managed_string = devm_kstrdup(fake_device, "Hello, World!");
--
2.34.1

[Robin Murphy]

On 2025-10-15 2:46 pm, Robin Murphy wrote:
> kunit_device_register() only returns error pointers, not NULL.
> Furthermore for regular users who aren't testing the KUnit API
> itself, errors most likely represent major system failure (e.g. OOM
> or sysfs collision) beyond the scope of their own test conditions.
> Replace the assert with straightforward error handling for clarity.
>
> Signed-off-by: Robin Murphy <robin....@arm.com>
> ---
> This seemed the logical conclusion by inspection, but please do correct
> me if I've misunderstood the intent...
> ---
> Documentation/dev-tools/kunit/usage.rst | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst
> index 038f480074fd..3452c739dd44 100644
> --- a/Documentation/dev-tools/kunit/usage.rst
> +++ b/Documentation/dev-tools/kunit/usage.rst
> @@ -873,7 +873,8 @@ For example:
>
> // Create a fake device.
> fake_device = kunit_device_register(test, "my_device");
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_device)
> + if (IS_ERR(fake_device))
> + return;

On further consideration, I guess kunit_skip() (as used in various other
places) is actually what I want here?

Basically, as someone looking at KUnit with fresh eyes it seems
intuitive to me that not being able to run a test case is a not a
failure of the thing being tested, so shouldn't be reported as such, and
thus this example stood out. I for one wouldn't want to be getting CI
notifications to go and debug a "regression" in my code just because a
runner OOM'd, for example :)

Thanks,
Robin.

[David Gow]

Hi Robin,

Ultimately, I think this is up to the individual test author -- in
many cases, I think failing to create a device (for any reason) should
be considered a test failure. Certainly this is the pattern which
exists in most tests thus far. In general, KUNIT_ASSERT_* is used to
verify these sorts of failures (after which the test cannot continue).

That being said, I definitely think you'd need to use at least
kunit_skip() -- with a good message -- if you wanted to split
"infrastructure failures" out: having a test marked "success" in
situations where it couldn't run properly (as in the original patch)
would be even more misleading. kunit_skip() is definitely used to skip
tests if prerequisites aren't found, but this tends to be done at the
start of the test with deterministic, known-in-advance requirements
(e.g number of processors available), rather than in response to an
OOM situation. (And, realistically, if the system is so memory
constrained that we're getting OOMs here, chances are you'll want to
know about it and re-run the tests anyway.

Regardless, I'd prefer to keep the example in the documentation as-is:
KUNIT_ASSERT* will correctly exit the test and clean up in this case,
whereas manually writing the IS_ERR/return bit is a bit more
contingent on this not being in an init/helper/etc function.

But separating "test failures" from "infrastructure failures" is a
good idea in general, and our current splitting things up into
"failed", "skipped", and "crashed" (used by the tooling for cases
where the kernel dies without outputting the result) is clearly not
ideal for these OOM-adjacent cases.

Cheers,
-- David