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.murphy@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!");
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.murphy@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.
// Pass it to functions which need a device. dev_managed_string = devm_kstrdup(fake_device, "Hello, World!");
Hi Robin,
On Fri, 17 Oct 2025 at 01:45, Robin Murphy robin.murphy@arm.com wrote:
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.murphy@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 :)
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
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Robin Murphy