Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

[Marco Elver]

On Tue, 10 Nov 2020 at 17:32, Arpitha Raghunandan <98.arpi@xxxxxxxxx> wrote:
>
> On 10/11/20 4:05 pm, Marco Elver wrote:
> > On Tue, 10 Nov 2020 at 08:21, David Gow <davidgow@xxxxxxxxxx> wrote:
> > [...]
> >>>>
> >>>> The previous attempt [1] at something similar failed because it seems
> >>>> we'd need to teach kunit-tool new tricks [2], too.
> >>>> [1] https://lkml.kernel.org/r/20201105195503.GA2399621@xxxxxxxxxxxxxxxx
> >>>> [2] https://lkml.kernel.org/r/20201106123433.GA3563235@xxxxxxxxxxxxxxxx
> >>>>
> >>>> So if we go with a different format, we might need a patch before this
> >>>> one to make kunit-tool compatible with that type of diagnostic.
> >>>>
> >>>> Currently I think we have the following proposals for a format:
> >>>>
> >>>> 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> >>>> this works well, because no changes to kunit-tool are required, and it
> >>>> also picks up the diagnostic context for the case and displays that on
> >>>> test failure.
> >>>>
> >>>> 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> >>>> As-is, this needs a patch for kunit-tool as well. I just checked, and
> >>>> if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> >>>> (note the space after ':') it works without changing kunit-tool. ;-)
> >>>>
> >>>> 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> >>>> which I had played with earlier but kunit-tool is definitely not yet
> >>>> happy with.
> >>>>
> >>>> So my current preference is (2) with the extra space (no change to
> >>>> kunit-tool required). WDYT?
> >>>>
> >>
> >> Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
> >> what comes after the comment character except if it's an explicit
> >> subtest declaration or a crash. I'll try to put a patch together to
> >> fix it, but I'd rather not delay this just for that.
> >>
> >> In any thought about this a bit more, It turns out that the proposed
> >> KTAP spec[1] discourages the use of ':', except as part of a subtest
> >> declaration, or perhaps an as-yet-unspecified fully-qualified test
> >> name. The latter is what I was going for, but if it's actively
> >> breaking kunit_tool, we might want to hold off on it.
> >>
> >> If we were to try to treat these as subtests in accordance with that
> >> spec, the way we'd want to use one of these options:
> >> A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
> >> test case name, but otherwise treats things exactly the same way we
> >> treat existing subtests.
> >>
> >> B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
> >> "subtest", just gives repeated results with the same name.
> >>
> >> C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> >> -- This is equivalent to my suggestion with a separator of ":", or 2
> >> above with a separator of ": ". The in-progress spec doesn't yet
> >> specify how these fully-qualified names would work, other than that
> >> they'd use a colon somewhere, and if we comment it out, ": " is
> >> required.
> >>
> >>>
> >>> Which format do we finally go with?
> >>>
> >>
> >> I'm actually going to make another wild suggestion for this, which is
> >> a combination of (1) and (A):
> >> "# [test_case->name]: [ok|not ok] [index] - param-[index]"
> >>
> >> This gives us a KTAP-compliant result line, except prepended with "#
> >> [test_case->name]: ", which makes it formally a diagnostic line,
> >> rather than an actual subtest. Putting the test name at the start
> >> matches what kunit_tool is expecting at the moment. If we then want to
> >> turn it into a proper subtest, we can just get rid of that prefix (and
> >> add the appropriate counts elsewhere).
> >>
> >> Does that sound good?
> >
> > Sounds reasonable to me!  The repetition of [index] seems unnecessary
> > for now, but I think if we at some point have a way to get a string
> > representation of a param, we can substitute param-[index] with a
> > string that represents the param.
> >
>
> So, with this the inode-test.c will have the following output, right?
>
> TAP version 14
> 1..7
>     # Subtest: ext4_inode_test
>     1..1
>     # inode_test_xtimestamp_decoding: ok 0 - param-0

So the params should certainly be 0-indexed, but I think TAP starts
counting at 1, so maybe this should be:

    # inode_test_xtimestamp_decoding: ok 1 - param-0

and so on... ?

Right now it doesn't matter, but it will if we make these subsubtests
as David suggested.

>     # inode_test_xtimestamp_decoding: ok 1 - param-1
>     # inode_test_xtimestamp_decoding: ok 2 - param-2
>     # inode_test_xtimestamp_decoding: ok 3 - param-3
>     # inode_test_xtimestamp_decoding: ok 4 - param-4
>     # inode_test_xtimestamp_decoding: ok 5 - param-5
>     # inode_test_xtimestamp_decoding: ok 6 - param-6
>     # inode_test_xtimestamp_decoding: ok 7 - param-7
>     # inode_test_xtimestamp_decoding: ok 8 - param-8
>     # inode_test_xtimestamp_decoding: ok 9 - param-9
>     # inode_test_xtimestamp_decoding: ok 10 - param-10
>     # inode_test_xtimestamp_decoding: ok 11 - param-11
>     # inode_test_xtimestamp_decoding: ok 12 - param-12
>     # inode_test_xtimestamp_decoding: ok 13 - param-13
>     # inode_test_xtimestamp_decoding: ok 14 - param-14
>     # inode_test_xtimestamp_decoding: ok 15 - param-15
>     ok 1 - inode_test_xtimestamp_decoding
> ok 1 - ext4_inode_test
>
> I will send another patch with this change.
> Thanks!

Yes that looks right, but see the comment above ^

Thanks,
-- Marco