From: Marco Elver <elver@google.com>
To: David Gow <davidgow@google.com>
Cc: "Bird, Tim" <Tim.Bird@sony.com>,
Arpitha Raghunandan <98.arpi@gmail.com>,
Brendan Higgins <brendanhiggins@google.com>,
Shuah Khan <skhan@linuxfoundation.org>,
Iurii Zaikin <yzaikin@google.com>, Theodore Ts'o <tytso@mit.edu>,
Andreas Dilger <adilger.kernel@dilger.ca>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@vger.kernel.org>,
KUnit Development <kunit-dev@googlegroups.com>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"linux-kernel-mentees@lists.linuxfoundation.org"
<linux-kernel-mentees@lists.linuxfoundation.org>,
"linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing
Date: Fri, 13 Nov 2020 11:30:56 +0100 [thread overview]
Message-ID: <20201113103056.GA1568882@elver.google.com> (raw)
In-Reply-To: <CABVgOSkjExNtGny=CDT1WVaXUVgSEaf7hwx8=VY4atN5ot10KQ@mail.gmail.com>
On Fri, Nov 13, 2020 at 01:17PM +0800, David Gow wrote:
> On Thu, Nov 12, 2020 at 8:37 PM Marco Elver <elver@google.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
next prev parent reply other threads:[~2020-11-13 10:31 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-06 19:21 [PATCH v6 1/2] kunit: Support for Parameterized Testing Arpitha Raghunandan
2020-11-06 19:22 ` [PATCH v6 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature Arpitha Raghunandan
2020-11-07 5:00 ` David Gow
2020-11-06 19:37 ` [PATCH v6 1/2] kunit: Support for Parameterized Testing Marco Elver
2020-11-07 4:58 ` David Gow
2020-11-07 10:06 ` Marco Elver
2020-11-09 6:49 ` Arpitha Raghunandan
2020-11-10 7:20 ` David Gow
2020-11-10 10:35 ` Marco Elver
2020-11-10 16:32 ` Arpitha Raghunandan
2020-11-10 16:41 ` Marco Elver
2020-11-10 16:50 ` Arpitha Raghunandan
2020-11-10 17:02 ` Bird, Tim
2020-11-10 23:27 ` David Gow
2020-11-11 16:55 ` Bird, Tim
2020-11-12 8:18 ` David Gow
2020-11-12 12:37 ` Marco Elver
2020-11-13 5:17 ` David Gow
2020-11-13 10:30 ` Marco Elver [this message]
2020-11-13 22:37 ` David Gow
2020-11-14 0:14 ` Marco Elver
2020-11-14 1:37 ` Arpitha Raghunandan
2020-11-14 3:17 ` David Gow
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201113103056.GA1568882@elver.google.com \
--to=elver@google.com \
--cc=98.arpi@gmail.com \
--cc=Tim.Bird@sony.com \
--cc=adilger.kernel@dilger.ca \
--cc=brendanhiggins@google.com \
--cc=davidgow@google.com \
--cc=kunit-dev@googlegroups.com \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-kernel-mentees@lists.linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=skhan@linuxfoundation.org \
--cc=tytso@mit.edu \
--cc=yzaikin@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).