On Fri, Mar 7, 2025 at 11:01 AM Petr Mladek pmladek@suse.com wrote:
On Thu 2025-03-06 09:25:43, Tamir Duberstein wrote:
On Thu, Mar 6, 2025 at 7:25 AM Petr Mladek pmladek@suse.com wrote:
On Fri 2025-02-21 15:34:30, Tamir Duberstein wrote:
Convert the printf() self-test to a KUnit test.
In the interest of keeping the patch reasonably-sized this doesn't refactor the tests into proper parameterized tests - it's all one big test case.
--- a/lib/test_printf.c +++ b/lib/tests/printf_kunit.c @@ -57,52 +58,50 @@ do_test(int bufsize, const char *expect, int elen, va_end(aq);
if (ret != elen) {
pr_warn("vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d\n",
tc_fail("vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d",
It looks a bit strange that the 1st patch replaces pr_warn() with tc_fail() which hides KUNIT_FAIL().
And the 2nd patch replaces tc_fail() with KUNIT_FAIL().
It looks like a non-necessary churn.
It would be better to avoid the temporary "tc_fail" and swith to KUNIT_FAIL() already in this patch.
I did not find any comment about this in the earier versions of the patchset.
Is it just a result of the evolution of the patchset or is there any motivation for this?
The motivation was to keep the width of the macro the same in this first patch for ease of review, particularly in the 7 instances where the invocation wraps to a second line. If you prefer I go straight to KUNIT_FAIL, I can make that change.
I see. It might have been useful when the patch removed the trailing '\n'. But you are going to add it back. So there won't be any hidden change. So I would prefer to go straight to KUNIT_FAIL().
👍 I've restored all the newlines and added a few previously missing ones.
@@ -842,13 +836,15 @@ test_pointer(void) fourcc_pointer(); }
-static void __init selftest(void) +static void printf_test(struct kunit *test) { alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL); if (!alloced_buffer) return;
I would use here:
KUNIT_ASSERT_NOT_NULL(test, alloced_buffer);And move the same change for the other kmalloc() location from the 2nd patch.
I didn't do that here because I was trying to keep this patch as small as possible, and I wrote that in the commit message.
As for using KUNIT_ASSERT_NOT_NULL here, that would have to change back to an error return in the 2nd patch because this code moves into `suite_init`, which is called with `struct kunit_suite` rather than `struct kunit_test`, and KUnit assertion macros do not work with the former (and for good reason, because failures in suite setup cannot be attributed to a particular test case).
I see. KUNIT_ASSERT_NOT_NULL() can't be used in the .suite_exit() callback.
So I'd prefer to leave this as is.
I agree to leave this as is.
👍