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

[Arpitha Raghunandan <98.arpi@gmail.com>]

On 10/11/20 4:05 pm, Marco Elver wrote:
> On Tue, 10 Nov 2020 at 08:21, David Gow <davidgow@google.com> 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@elver.google.com
>>>> [2] https://lkml.kernel.org/r/20201106123433.GA3563235@elver.google.com
>>>>
>>>> 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
    # 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!

> Note that once we want to make it a real subtest, we'd need to run the
> generator twice, once to get the number of params and then to run the
> tests. If we require that param generators are deterministic in the
> number of params generated, this is not a problem.
> 
> Thanks,
> -- Marco
>