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

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

On 15/11/20 2:28 pm, Marco Elver wrote:
> On Sat, 14 Nov 2020 at 13:38, 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 based on arrays. Generators may also
>> optionally provide a human-readable description of parameters, which is
>> displayed where available.
>>
>> Note, currently the result of each parameter run is displayed in
>> diagnostic lines, and only the overall test case output summarizes
>> TAP-compliant success or failure of all parameter runs. In future, when
>> supported by kunit-tool, these can be turned into subsubtest outputs.
>>
>> 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>
>> ---
>> Changes v6->v7:
>> - Clarify commit message.
>> - Introduce ability to optionally generate descriptions for parameters;
>>   if no description is provided, we'll still print 'param-N'.
>> - Change diagnostic line format to:
>>         # <test-case-name>: <ok|not ok> N - [<param description>]
>>
>> 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
>> Changes v6->v7:
>> - Clarify commit message.
>> - Introduce ability to optionally generate descriptions for parameters;
>>   if no description is provided, we'll still print 'param-N'.
>> - Change diagnostic line format to:
>>         # <test-case-name>: <ok|not ok> N - [<param description>]
>> - Before execution of parameterized test case, count number of
>>   parameters and display number of parameters. Currently also as a
>>   diagnostic line, but this may be used in future to generate a subsubtest
>>   plan. A requirement of this change is that generators must generate a
>>   deterministic number of parameters.
>>
>> 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 | 51 ++++++++++++++++++++++++++++++++++++++
>>  lib/kunit/test.c     | 59 ++++++++++++++++++++++++++++++++++----------
>>  2 files changed, 97 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index db1b0ae666c4..cf5f33b1c890 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -94,6 +94,9 @@ struct kunit;
>>  /* Size of log associated with test. */
>>  #define KUNIT_LOG_SIZE 512
>>
>> +/* Maximum size of parameter description string. */
>> +#define KUNIT_PARAM_DESC_SIZE 64
> 
> I think we need to make this larger, perhaps 128. I just noticed a few
> of the inode-test strings are >64 chars (and it should probably also
> use strncpy() to copy to description, which is my bad).
>

Okay, I will make the description size larger and use strncpy().
 
>>  /*
>>   * TAP specifies subtest stream indentation of 4 spaces, 8 spaces for a
>>   * sub-subtest.  See the "Subtests" section in
>> @@ -107,6 +110,7 @@ struct kunit;
> [...]
>> +/**
>> + * KUNIT_ARRAY_PARAM() - Define test parameter generator from an array.
>> + * @name:  prefix for the test parameter generator function.
>> + * @array: array of test parameters.
>> + * @get_desc: function to convert param to description; NULL to use default
>> + *
>> + * Define function @name_gen_params which uses @array to generate parameters.
>> + */
>> +#define KUNIT_ARRAY_PARAM(name, array, get_desc)                                               \
>> +       static const void *name##_gen_params(const void *prev, char *desc)                      \
>> +       {                                                                                       \
>> +               typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);     \
> 
> Why did you reintroduce a space between * and __next? AFAIK, this
> should follow the same style as the rest of the kernel, and it should
> just be 'thetype *ptr'.
> 

I introduced this space because checkpatch.pl gave an error without the space:
ERROR: need consistent spacing around '*' (ctx:WxV)
#1786: FILE: ./include/kunit/test.h:1786:
+		typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array);	\

But, if this is a mistake as it doesn't recognize __next to be a pointer, I will remove the space.

>> +               if (__next - (array) < ARRAY_SIZE((array))) {                                   \
>> +                       void (*__get_desc)(typeof(__next), char *) = get_desc;                  \
>> +                       if (__get_desc)                                                         \
>> +                               __get_desc(__next, desc);                                       \
>> +                       return __next;                                                          \
>> +               }                                                                               \
>> +               return NULL;                                                                    \
>> +       }
>> +
> 
> Thanks,
> -- Marco
> 

Thanks!