Message-ID: <CABVgOS=0Qm-Aa5qgh3PRxbcV0-2ZZaQCm-4HY0dx8597oY+yXw@mail.gmail.com>
Date:   Sat, 14 Nov 2020 06:37:44 +0800
From:   David Gow <davidgow@...gle.com>
To:     Marco Elver <elver@...gle.com>
Cc:     "Bird, Tim" <Tim.Bird@...y.com>,
        Arpitha Raghunandan <98.arpi@...il.com>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        "Theodore Ts'o" <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-kernel-mentees@...ts.linuxfoundation.org" 
        <linux-kernel-mentees@...ts.linuxfoundation.org>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Fri, Nov 13, 2020 at 6:31 PM Marco Elver <elver@...gle.com> wrote:
>
> On Fri, Nov 13, 2020 at 01:17PM +0800, David Gow wrote:
> > On Thu, Nov 12, 2020 at 8:37 PM Marco Elver <elver@...gle.com> wrote:
> [...]
> > > > (It also might be a little tricky with the current implementation to
> > > > produce the test plan, as the parameters come from a generator, and I
> > > > don't think there's a way of getting the number of parameters ahead of
> > > > time. That's a problem with the sub-subtest model, too, though at
> > > > least there it's a little more isolated from other tests.)
> > >
> > > The whole point of generators, as I envisage it, is to also provide the
> > > ability for varying parameters dependent on e.g. environment,
> > > configuration, number of CPUs, etc. The current array-based generator is
> > > the simplest possible use-case.
> > >
> > > However, we *can* require generators generate a deterministic number of
> > > parameters when called multiple times on the same system.
> >
> > I think this is a reasonable compromise, though it's not actually
> > essential. As I understand the TAP spec, the test plan is actually
> > optional (and/or can be at the end of the sequence of tests), though
> > kunit_tool currently only supports having it at the beginning (which
> > is strongly preferred by the spec anyway). I think we could get away
> > with having it at the bottom of the subtest results though, which
> > would save having to run the generator twice, when subtest support is
> > added to kunit_tool.
>
> I can't find this in the TAP spec, where should I look? Perhaps we
> shouldn't venture too far off the beaten path, given we might not be the
> only ones that want to parse this output.
>

It's in the "Test Lines and the Plan" section:
"The plan is optional but if there is a plan before the test points it
must be the first non-diagnostic line output by the test file. In
certain instances a test file may not know how many test points it
will ultimately be running. In this case the plan can be the last
non-diagnostic line in the output. The plan cannot appear in the
middle of the output, nor can it appear more than once."

My only concern with running through the generator multiple times to
get the count is that it might be slow and/or more difficult if
someone uses a more complicated generator. I can't think of anything
specific yet, though, so we can always do it for now and change it
later if a problematic case occurs.

> > > To that end, I propose a v7 (below) that takes care of getting number of
> > > parameters (and also displays descriptions for each parameter where
> > > available).
> > >
> > > Now it is up to you how you want to turn the output from diagnostic
> > > lines into something TAP compliant, because now we have the number of
> > > parameters and can turn it into a subsubtest. But I think kunit-tool
> > > doesn't understand subsubtests yet, so I suggest we take these patches,
> > > and then somebody can prepare kunit-tool.
> > >
> >
> > This sounds good to me. The only thing I'm not sure about is the
> > format of the parameter description: thus far test names be valid C
> > identifier names, due to the fact they're named after the test
> > function. I don't think there's a fundamental reason parameters (and
> > hence, potentially, subsubtests) need to follow that convention as
> > well, but it does look a bit odd.  Equally, the square brackets around
> > the description shouldn't be necessary according to the TAP spec, but
> > do seem to make things a little more readable, particuarly with the
> > names in the ext4 inode test. I'm not too worried about either of
> > those, though: I'm sure it'll look fine once I've got used to it.
>
> The parameter description doesn't need to be a C identifier. At least
> that's what I could immediately glean from TAP v13 spec (I'm looking
> here: https://testanything.org/tap-version-13-specification.html and see
> e.g. "ok 1 - Input file opened" ...).
>

Yeah: it looked a bit weird for everything else to be an identifier
(given that KUnit does require it for tests), but these parameter
descriptions not to be. It's not a problem, though, so let's go ahead
with it.

> [...]
> > > > In any case, I'm happy to leave the final decision here to Arpitha and
> > > > Marco, so long as we don't actually violate the TAP/KTAP spec and
> > > > kunit_tool is able to read at least the top-level result. My
> > > > preference is still to go either with the "# [test_case->name]:
> > > > [ok|not ok] [index] - param-[index]", or to get rid of the
> > > > per-parameter results entirely for now (or just print out a diagnostic
> > > > message on failure). In any case, it's a decision we can revisit once
> > > > we have support for named parameters, better tooling, or a better idea
> > > > of how people are actually using this.
> > >
> > > Right, so I think we'll be in a better place if we implement: 1)
> > > parameter to description conversion support, 2) counting parameters. So
> > > I decided to see what it looks like, and it wasn't too bad. I just don't
> > > know how you want to fix kunit-tool to make these non-diagnostic lines
> > > and not complain, but as I said, it'd be good to not block these
> > > patches.
> >
> > Yup, I tried this v7, and it looks good to me. The kunit_tool work
> > will probably be a touch more involved, so I definitely don't want to
> > hold up supporting this on that.
> >
> > My only thoughts on the v7 patch are:
> > - I don't think we actually need the parameter count yet (or perhaps
> > ever if we go with subtests as planned), so I be okay with getting rid
> > of that.
>
> As noted above, perhaps we should keep it for compatibility with other
> parsers and CI systems we don't have much control over. It'd be a shame
> if 99% of KUnit output can be parsed by some partially compliant parser,
> yet this would break it.

KUnit has only started providing the test plans in some cases pretty
recently, and the spec does make it optional, so I'm not particularly
worried about this breaking parsers. I'm not too worried about it
causing problems to have it either, though, so if you'd rather keep
it, that's fine by me as well.

> > - It'd be a possibility to get rid of the square brackets from the
> > output, and if we still want them, make them part of the test itself:
> > if this were TAP formatted, those brackets would be part of the
> > subsubtest name.
>
> I don't mind. It's just that we can't prescribe a format, and as
> seen below the descriptions include characters -<>=,. which can be
> confusing. But perhaps you're right, so let's remove them.
>
> But as noted, TAP doesn't seem to care. So let's remove them.
>

Yeah: I have a slight preference for removing them, as TAP parsers
would otherwise include them in the parameter name, which looks a
little weird.
Of course, the point is moot until we actually fix kunit_tool and make
these subtests, so there's no fundamental reason we couldn't leave
them in for now, and remove them then if you thought it was
significantly more readable. (Personally, I'd still err on the side of
removing them to avoid any unnecessary churn.)

Cheers,
-- David

Powered by blists - more mailing lists