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

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Tue, Feb 11 2025, David Gow <davidgow@google.com> wrote:

> On Mon, 10 Feb 2025 at 19:57, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>>
>> On Fri, Feb 07 2025, Tamir Duberstein <tamird@gmail.com> wrote:
>>
>> > On Fri, Feb 7, 2025 at 5:01 AM Rasmus Villemoes
>> > <linux@rasmusvillemoes.dk> wrote:
>> >>
>> >> On Thu, Feb 06 2025, Tamir Duberstein <tamird@gmail.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.

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.

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. 

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.

Rasmus