Message-ID: <20201113103056.GA1568882@elver.google.com>
Date:   Fri, 13 Nov 2020 11:30:56 +0100
From:   Marco Elver <elver@...gle.com>
To:     David Gow <davidgow@...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 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.

> > 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" ...).

[...]
> > > 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.

> - 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.

[...]
> > I hope this is a reasonable compromise for now.
> 
> Yeah: this seems like a great compromise until kunit_tool is improved.

Thank you!

-- Marco

Powered by blists - more mailing lists