Message-ID: <CABVgOSmYWfBPod8Dq=vdYvmOUE31Nft1Ad-0nRHZ9J2W_f-q_Q@mail.gmail.com>
Date: Wed, 12 Feb 2025 17:25:35 +0800
From: David Gow <davidgow@...gle.com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Tamir Duberstein <tamird@...il.com>, Petr Mladek <pmladek@...e.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	Sergey Senozhatsky <senozhatsky@...omium.org>, Andrew Morton <akpm@...ux-foundation.org>, 
	Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 0/2] printf: convert self-test to KUnit

On Tue, 11 Feb 2025 at 16:40, Rasmus Villemoes <linux@...musvillemoes.dk> wrote:
>
> On Tue, Feb 11 2025, David Gow <davidgow@...gle.com> wrote:
>
> > On Mon, 10 Feb 2025 at 19:57, Rasmus Villemoes <linux@...musvillemoes.dk> wrote:
> >>
> >> On Fri, Feb 07 2025, Tamir Duberstein <tamird@...il.com> wrote:
> >>
> >> > On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes
> >> > <linux@...musvillemoes.dk> wrote:
> >> >>
> >> >> On Thu, Feb 06 2025, Tamir Duberstein <tamird@...il.com> wrote:
> >> >>
> >> >>
> >> >> 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.
> >> >
> >>
> >> I'm not sure that's accurate.
> >>
> >> The thing is, each of the current test() instances results in four
> >> different tests being done, which is roughly why we end up at the 4*97
> >> == 388, but each of those tests has several assertions being done -
> >> depending on which variant of the test we're doing (i.e. the buffer
> >> length used or if we're passing it through kasprintf), we may do only
> >> some of those assertions, and we do an early return in case one of those
> >> assertions fail (because it wouldn't be safe to do the following
> >> assertions, and the test as such has failed already). So there are far
> >> more assertions than those 388.
> >>
> >> OTOH, that the number reported is 388 is more a consequence of the
> >> implementation than anything explicitly designed. I can certainly live
> >> with 388 being replaced by 97, i.e. that each current test() invocation
> >> would count as one KUNIT case, as that would still allow me to detect a
> >> PEBKAC when I've added a new test() instance and failed to actually run
> >> that.
> >
> > It'd be possible to split things up further into tests, at the cost of
> > it being a more extensive refactoring, if having the more granular
> > count tracked by KUnit were desired.
>
> I think the problem is that kunit is simply not a good framework to do
> these kinds of tests in, and certainly it's very hard to retrofit kunit
> after the fact.
>

I think I'd have to disagree on the whole (though I'm admittedly
biased in KUnit's favour), but I can definitely see that the printf
tests do provide some unique challenges, and that either way, a port
would require either some code churn or bloat, a need to reinterpret
things (such as what unit a 'test' is), or both.

Ultimately, I don't want to force KUnit on anyone if it's going to
make things more difficult, so it's ultimately up to you. My personal
feeling is that this could work well as a KUnit test, but due to the
churn involved, it may not be worth it if no-one wants to take
advantage of the tooling.

> It'd also be possible to make
> > these more explicitly data driven via a parameterised test (so each
> > input/output pair is listed in an array, and automatically gets
> > converted to a KUnit subtest).
>
> So that "array of input/output" very much doesn't work for these
> specific tests: We really want the format string/varargs to be checked
> by the compiler, and besides, there's no way to store the necessary
> varargs and generate a call from those in an array. Moreover, we verify a
> lot more than just that the correct string is produced; it's also a
> matter of the right return value regardless of the passed buffer size, etc.

Ah, that makes sense. I suspect with enough work and some friendly
compiler developers, this could be make to work, but it definitely
doesn't seem worth the effort to me.

> That's also why is nigh impossible to simply change __test() into
> (another) macro that expands to something that defines an individual
> struct kunit_case, because the framework is really built around the
> notion that each case can be represented by a void function call and the
> name of the test is the stringification of the function name.

Yeah: it may be possible to do something with KUnit's parameter
generating functions (you can have a function which generates a void*
test context, as well as a string test name: this could be a struct
with a format string and a va_list), but it's definitely getting
complicated.

> So I don't mind the conversion to kunit if that really helps other
> people, as long as the basic functionality is still present and doesn't
> impede future extensions - and certainly I don't want to end up in a
> situation where somebody adds a new %p extension but cannot really add a
> test for it because kunit makes that hard.
>
> But I hope you all agree that it doesn't make much _sense_ to consider
> test_number() and test_string() and so on individual "test cases"; the
> atomic units of test being done in the printf suite is each invocation
> of the __test() function, with one specific format string/varargs
> combination.

I think this is -- to some extent -- a matter of interpretation. I
don't think it's wrong to use KUnit test cases to refer to a "thing
being tested" (e.g., a specific format specifier) rather than an
"individual invocation": lots of KUnit tests already group very
related things together. But given the way there are several "checks"
within each __test() invocation mirrors this already, I understand why
it'd make sense to keep that as the "test case".

I don't have any immediate plans personally to work on the
printf/scanf code, so your opinion here definitely matters more than
mine. But if this does end up as a KUnit test, I'll definitely keep an
eye on it as part of my regular KUnit test runs.

Cheers,
-- David

>
> Rasmus

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5294 bytes)

Powered by blists - more mailing lists