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

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

On 10/11/20 10:11 pm, Marco Elver wrote:
> On Tue, 10 Nov 2020 at 17:32, Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>>
>> 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
> 
> 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.
> 

Okay yes, I will make the first index (following [ok|not ok]) start with 1
and let the params remain 0-indexed.
Thanks!

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