Re: [PATCH 0/2] printf: convert self-test to KUnit

[David Gow]

On Thu, 6 Feb 2025 at 23:42, 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'
>

This failure message looks good to me. The ones in the current patch
are very verbose, and while the memory comparisons could be useful for
the overflow/buffer size tests, for simple string comparisons, having
the string in a readable format is best.

Rasmus: the KUnit framework has since added a summary line to the
output by default, which should also make this less of a regression
from the existing format:
# printf: pass:28 fail:0 skip:0 total:28

Cheers,
-- David

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature