On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx> wrote: > > On Thu, Feb 06 2025, Tamir Duberstein <tamird@xxxxxxxxx> wrote: > > > On Thu, Feb 6, 2025 at 4:27 AM Rasmus Villemoes > > <linux@xxxxxxxxxxxxxxxxxx> wrote: > >> > >> On Tue, 4 Feb 2025 at 20:36, Tamir Duberstein <tamird@xxxxxxxxx> wrote: > >> > > >> > This is one of just 3 remaining "Test Module" kselftests (the others > >> > being bitmap and scanf), the rest having been converted to KUnit. > >> > > >> > I tested this using: > >> > > >> > $ tools/testing/kunit/kunit.py run --arch arm64 --make_options LLVM=1 printf > >> > > >> > I have also sent out a series converting scanf[0]. > >> > > >> > Link: https://lore.kernel.org/all/20250204-scanf-kunit-convert-v3-0-386d7c3ee714@xxxxxxxxx/T/#u [0] > >> > > >> > >> Sorry, but NAK, not in this form. > >> > >> Please read the previous threads to understand what is wrong with this > >> mechanical approach. Not only is it wrong, it also actively makes the > >> test suite much less useful. > >> > >> https://lore.kernel.org/lkml/f408efbd-10f7-f1dd-9baa-0f1233cafffc@xxxxxxxxxxxxxxxxxx/ > >> https://lore.kernel.org/lkml/876cc862-56f1-7330-f988-0248bec2fbad@xxxxxxxxxxxxxxxxxx/ > >> https://lore.kernel.org/lkml/0ab618c7-8c5c-00ae-8e08-0c1b99f3bf5c@xxxxxxxxxxxxxxxxxx/ > >> > >> I think the previous attempt was close to something acceptable (around > >> https://lore.kernel.org/lkml/57976ff4-7845-d721-ced1-1fe439000a9b@xxxxxxxxxxxxxxxxxx/), > >> but I don't know what happened to it. > >> > >> Rasmus > > > > Thanks Rasmus, I wasn't aware of that prior effort. I've gone through > > and adopted your comments - the result is a first patch that is much > > smaller (104 insertions(+), 104 deletions(-)) and failure messages > > that are quite close to what is emitted now. I've taken care to keep > > all the control flow the same, as you requested. The previous > > discussion concluded with a promise to send another patch which didn't > > happen. May I send a v2 with these changes, or are there more > > fundamental objections? I'll also cc Arpitha and Brendan. The new > > failure output: > > > > # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95 > > vsnprintf(buf, 256, "%piS|%pIS", ...) wrote > > '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1' > > # ip4: EXPECTATION FAILED at lib/printf_kunit.c:95 > > vsnprintf(buf, 19, "%piS|%pIS", ...) wrote '127.000.000.001|12', > > expected '127-000.000.001|12' > > # ip4: EXPECTATION FAILED at lib/printf_kunit.c:131 > > kvasprintf(..., "%piS|%pIS", ...) returned > > '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1' > > That certainly addresses one of my main objections; I really don't want to see > "memcmp(..., ...) == 1, expected memcmp(..., ...) == 0". And you said > you've kept the control flow/early returns the same, so that should also > be ok. > > I'll have to see the actual code, of course. In general, I find reading > code using those KUNIT macros quite hard, because I'm not familiar with > those macros and when I try to look up what they do they turn out to be > defined in terms of other KUNIT macros 10 levels deep. > > But that still leaves a few points. First, I really like that "388 test > cases passed" tally or some other free-form summary (so that I can see > that I properly hooked up, compiled, and ran a new testcase inside > test_number(), so any kind of aggregation on those top-level test_* is > too coarse). This one I'm not sure how to address. What you're calling test cases here would typically be referred to as assertions, and I'm not aware of a way to report a count of assertions. > The other thing I want to know is if this would make it harder for me to > finish up that "deterministic random testing" thing I wrote [*], but > never got around to actually get it upstream. It seems like something > that a framework like kunit could usefully provide out-of-the-box (which > is why I attempted to get it into kselftest), but as long as I'd still > be able to add in something like that, and get a "xxx failed, random > seed used was 0xabcdef" line printed, and run the test again setting the > seed explicitly to that 0xabcdef value, I'm good. > > [*] https://lore.kernel.org/lkml/20201025214842.5924-4-linux@xxxxxxxxxxxxxxxxxx/ I can't speak for the framework, but it wouldn't be any harder to do in printf itself. I did it this way: +static struct rnd_state rnd_state; +static u64 seed; + static int printf_suite_init(struct kunit_suite *suite) { alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL); if (!alloced_buffer) return -1; test_buffer = alloced_buffer + PAD_SIZE; + + seed = get_random_u64(); + prandom_seed_state(&rnd_state, seed); return 0; } static void printf_suite_exit(struct kunit_suite *suite) { kfree(alloced_buffer); + if (kunit_suite_has_succeeded(suite) == KUNIT_FAILURE) { + pr_info("Seed: %llu\n", seed); + } } and the result (once I made one of the cases fail): printf_kunit: Seed: 11480747578984087668 # printf: pass:27 fail:1 skip:0 total:28 # Totals: pass:27 fail:1 skip:0 total:28 not ok 1 printf Thank you both for engaging with me here. I'll send v2 in a few minutes and we can continue the discussion there. Tamir
