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

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

On 07/11/20 3:36 pm, Marco Elver wrote:
> On Sat, 7 Nov 2020 at 05:58, David Gow <davidgow@google.com> wrote:
>> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>>>
>>> Implementation of support for parameterized testing in KUnit.
>>> This approach requires the creation of a test case using the
>>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
>>> This generator function should return the next parameter given the
>>> previous parameter in parameterized tests. It also provides
>>> a macro to generate common-case generators.
>>>
>>> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
>>> Co-developed-by: Marco Elver <elver@google.com>
>>> Signed-off-by: Marco Elver <elver@google.com>
>>> ---
>>
>> This looks good to me! A couple of minor thoughts about the output
>> format below, but I'm quite happy to have this as-is regardless.
>>
>> Reviewed-by: David Gow <davidgow@google.com>
>>
>> Cheers,
>> -- David
>>
>>> Changes v5->v6:
>>> - Fix alignment to maintain consistency
>>> Changes v4->v5:
>>> - Update kernel-doc comments.
>>> - Use const void* for generator return and prev value types.
>>> - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
>>> - Rework parameterized test case execution strategy: each parameter is executed
>>>   as if it was its own test case, with its own test initialization and cleanup
>>>   (init and exit are called, etc.). However, we cannot add new test cases per TAP
>>>   protocol once we have already started execution. Instead, log the result of
>>>   each parameter run as a diagnostic comment.
>>> Changes v3->v4:
>>> - Rename kunit variables
>>> - Rename generator function helper macro
>>> - Add documentation for generator approach
>>> - Display test case name in case of failure along with param index
>>> Changes v2->v3:
>>> - Modifictaion of generator macro and method
>>> Changes v1->v2:
>>> - Use of a generator method to access test case parameters
>>>
>>>  include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
>>>  lib/kunit/test.c     | 46 +++++++++++++++++++++++++++++++-------------
>>>  2 files changed, 69 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>> index db1b0ae666c4..16616d3974f9 100644
>>> --- a/include/kunit/test.h
>>> +++ b/include/kunit/test.h
>>> @@ -107,6 +107,7 @@ struct kunit;
> [...]
>>> -       kunit_suite_for_each_test_case(suite, test_case)
>>> -               kunit_run_case_catch_errors(suite, test_case);
>>> +       kunit_suite_for_each_test_case(suite, test_case) {
>>> +               struct kunit test = { .param_value = NULL, .param_index = 0 };
>>> +               bool test_success = true;
>>> +
>>> +               if (test_case->generate_params)
>>> +                       test.param_value = test_case->generate_params(NULL);
>>> +
>>> +               do {
>>> +                       kunit_run_case_catch_errors(suite, test_case, &test);
>>> +                       test_success &= test_case->success;
>>> +
>>> +                       if (test_case->generate_params) {
>>> +                               kunit_log(KERN_INFO, &test,
>>> +                                         KUNIT_SUBTEST_INDENT
>>> +                                         "# %s: param-%d %s",
>>
>> Would it make sense to have this imitate the TAP format a bit more?
>> So, have "# [ok|not ok] - [name]" as the format? [name] could be
>> something like "[test_case->name]:param-[index]" or similar.
>> If we keep it commented out and don't indent it further, it won't
>> formally be a nested test (though if we wanted to support those later,
>> it'd be easy to add), but I think it would be nicer to be consistent
>> here.
> 
> 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?
> 

Which format do we finally go with?

>> My other suggestion -- albeit one outside the scope of this initial
>> version -- would be to allow the "param-%d" name to be overridden
>> somehow by a test. For example, the ext4 inode test has names for all
>> its test cases: it'd be nice to be able to display those instead (even
>> if they're not formatted as identifiers as-is).
> 
> Right, I was thinking about this, but it'd need a way to optionally
> pass another function that converts const void* params to readable
> strings. But as you say, we should do that as a follow-up patch later
> because it might require a few more iterations.
> 
> [...]
> 
> Thanks,
> -- Marco
>