On Sun, 15 Nov 2020 at 13:18, Arpitha Raghunandan <98.arpi@xxxxxxxxx> wrote:
>
> On 15/11/20 2:28 pm, Marco Elver wrote:
> > On Sat, 14 Nov 2020 at 13:38, Arpitha Raghunandan <98.arpi@xxxxxxxxx> 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@xxxxxxxxx>
> >> Co-developed-by: Marco Elver <elver@xxxxxxxxxx>
> >> Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> >> ---
> >> 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().
Thanks. There's also a report by the test robot now which noticed this.
> >> /*
> >> * 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.
I think checkpatch.pl thinks this is a multiplication. It's definitely
a false positive. Please do format it like a normal pointer.
Thanks,
-- Marco
