[v4,1/2] kunit: Support for Parameterized Testing

Commit Message

Arpitha Raghunandan Oct. 27, 2020, 5:46 p.m. UTC

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 <[e-mail]>
Co-developed-by: Marco Elver <[e-mail]>
Signed-off-by: Marco Elver <[e-mail]>
---
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 | 34 ++++++++++++++++++++++++++++++++++
 lib/kunit/test.c     | 21 ++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

Comments

Marco Elver Oct. 27, 2020, 7:21 p.m. UTC | #1

On Tue, 27 Oct 2020 at 18:47, Arpitha Raghunandan <[e-mail]> 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 <[e-mail]>
> Co-developed-by: Marco Elver <[e-mail]>
> Signed-off-by: Marco Elver <[e-mail]>
> ---
> 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 | 34 ++++++++++++++++++++++++++++++++++
>  lib/kunit/test.c     | 21 ++++++++++++++++++++-
>  2 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 9197da792336..ec2307ee9bb0 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -107,6 +107,13 @@ struct kunit;
>   *
>   * @run_case: the function representing the actual test case.
>   * @name:     the name of the test case.
> + * @generate_params: the generator function for parameterized tests.
> + *
> + * The generator function is used to lazily generate a series of
> + * arbitrarily typed values that fit into a void*. The argument @prev
> + * is the previously returned value, which should be used to derive the
> + * next value; @prev is set to NULL on the initial generator call.
> + * When no more values are available, the generator must return NULL.
>   *

Hmm, should this really be the first paragraph? I think it should be
the paragraph before "Example:" maybe. But then that paragraph should
refer to generate_params e.g. "The generator function @generate_params
is used to ........".

The other option you have is to move this paragraph to the kernel-doc
comment for KUNIT_CASE_PARAM, which seems to be missing a kernel-doc
comment.

>   * A test case is a function with the signature,
>   * ``void (*)(struct kunit *)``
> @@ -141,6 +148,7 @@ struct kunit;
>  struct kunit_case {
>         void (*run_case)(struct kunit *test);
>         const char *name;
> +       void* (*generate_params)(void *prev);
>
>         /* private: internal use only. */
>         bool success;
> @@ -162,6 +170,9 @@ static inline char *kunit_status_to_string(bool status)
>   * &struct kunit_case for an example on how to use it.
>   */
>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }

I.e. create a new kernel-doc comment for KUNIT_CASE_PARAM here, and
simply move the paragraph describing the generator protocol into that
comment.

> +#define KUNIT_CASE_PARAM(test_name, gen_params)                        \
> +               { .run_case = test_name, .name = #test_name,    \
> +                 .generate_params = gen_params }
>
>  /**
>   * struct kunit_suite - describes a related collection of &struct kunit_case
> @@ -208,6 +219,15 @@ struct kunit {
>         const char *name; /* Read only after initialization! */
>         char *log; /* Points at case log after initialization */
>         struct kunit_try_catch try_catch;
> +       /* param_value points to test case parameters in parameterized tests */

Hmm, not quite: param_value is the current parameter value for a test
case. Most likely it's a pointer, but it doesn't need to be.

> +       void *param_value;
> +       /*
> +        * param_index stores the index of the parameter in
> +        * parameterized tests. param_index + 1 is printed
> +        * to indicate the parameter that causes the test
> +        * to fail in case of test failure.
> +        */

I think this comment needs to be reformatted, because you can use at
the very least use 80 cols per line. (If you use vim, visual select
and do 'gq'.)

> +       int param_index;
>         /*
>          * success starts as true, and may only be set to false during a
>          * test case; thus, it is safe to update this across multiple
> @@ -1742,4 +1762,18 @@ do {                                                                            \
>                                                 fmt,                           \
>                                                 ##__VA_ARGS__)
>
> +/**
> + * KUNIT_ARRAY_PARAM() - Helper method for test parameter generators
> + *                      required in parameterized tests.
> + * @name:  prefix of the name for the test parameter generator function.
> + *        It will be suffixed by "_gen_params".
> + * @array: a user-supplied pointer to an array of test parameters.
> + */
> +#define KUNIT_ARRAY_PARAM(name, array)                                                         \
> +       static void *name##_gen_params(void *prev)                                              \
> +       {                                                                                       \
> +               typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);     \
> +               return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;                  \
> +       }
> +
>  #endif /* _KUNIT_TEST_H */
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 750704abe89a..8ad908b61494 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -127,6 +127,12 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite,
>  }
>  EXPORT_SYMBOL_GPL(kunit_test_case_num);
>
> +static void kunit_print_failed_param(struct kunit *test)
> +{
> +       kunit_err(test, "\n\tTest failed at:\n\ttest case: %s\n\tparameter: %d\n",
> +                                               test->name, test->param_index + 1);
> +}

Hmm, perhaps I wasn't clear, but I think I also misunderstood how the
test case successes are presented: they are not, and it's all bunched
into a single test case.

Firstly, kunit_err() already prints the test name, so if we want
something like "  # : the_test_case_name: failed at parameter #X",
simply having

    kunit_err(test, "failed at parameter #%d\n", test->param_index + 1)

would be what you want.

But I think I missed that parameters do not actually produce a set of
test cases (sorry for noticing late). I think in their current form,
the parameterized tests would not be useful for my tests, because each
of my tests have test cases that have specific init and exit
functions. For each parameter, these would also need to run.

Ideally, each parameter produces its own independent test case
"test_case#param_index". That way, CI systems will also be able to
logically separate different test case params, simply because each
param produced its own distinct test case.

So, for example, we would get a series of test cases from something
like KUNIT_CASE_PARAM(test_case, foo_gen_params), and in the output
we'd see:

    ok X - test_case#1
    ok X - test_case#2
    ok X - test_case#3
    ok X - test_case#4
    ....

Would that make more sense?

That way we'd ensure that test-case specific initialization and
cleanup done in init and exit functions is properly taken care of, and
you wouldn't need kunit_print_failed_param().

AFAIK, for what I propose you'd have to modify kunit_print_ok_not_ok()
(show param_index if parameterized test) and probably
kunit_run_case_catch_errors() (generate params and set
test->param_value and param_index).

Was there a reason why each param cannot be a distinct test case? If
not, I think this would be more useful.

>  static void kunit_print_string_stream(struct kunit *test,
>                                       struct string_stream *stream)
>  {
> @@ -168,6 +174,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
>         assert->format(assert, stream);
>
>         kunit_print_string_stream(test, stream);
> +       if (test->param_value)
> +               kunit_print_failed_param(test);
>
>         WARN_ON(string_stream_destroy(stream));
>  }
> @@ -239,7 +247,18 @@ static void kunit_run_case_internal(struct kunit *test,
>                 }
>         }
>
> -       test_case->run_case(test);
> +       if (!test_case->generate_params) {
> +               test_case->run_case(test);
> +       } else {
> +               test->param_value = test_case->generate_params(NULL);
> +               test->param_index = 0;
> +
> +               while (test->param_value) {
> +                       test_case->run_case(test);
> +                       test->param_value = test_case->generate_params(test->param_value);
> +                       test->param_index++;
> +               }
> +       }

Thanks,
-- Marco

Arpitha Raghunandan Oct. 28, 2020, 8:45 a.m. UTC | #2

On 28/10/20 12:51 am, Marco Elver wrote:
> On Tue, 27 Oct 2020 at 18:47, Arpitha Raghunandan <[e-mail]> 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 <[e-mail]>
>> Co-developed-by: Marco Elver <[e-mail]>
>> Signed-off-by: Marco Elver <[e-mail]>
>> ---
>> 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 | 34 ++++++++++++++++++++++++++++++++++
>>  lib/kunit/test.c     | 21 ++++++++++++++++++++-
>>  2 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index 9197da792336..ec2307ee9bb0 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -107,6 +107,13 @@ struct kunit;
>>   *
>>   * @run_case: the function representing the actual test case.
>>   * @name:     the name of the test case.
>> + * @generate_params: the generator function for parameterized tests.
>> + *
>> + * The generator function is used to lazily generate a series of
>> + * arbitrarily typed values that fit into a void*. The argument @prev
>> + * is the previously returned value, which should be used to derive the
>> + * next value; @prev is set to NULL on the initial generator call.
>> + * When no more values are available, the generator must return NULL.
>>   *
> 
> Hmm, should this really be the first paragraph? I think it should be
> the paragraph before "Example:" maybe. But then that paragraph should
> refer to generate_params e.g. "The generator function @generate_params
> is used to ........".
> 
> The other option you have is to move this paragraph to the kernel-doc
> comment for KUNIT_CASE_PARAM, which seems to be missing a kernel-doc
> comment.
> 
>>   * A test case is a function with the signature,
>>   * ``void (*)(struct kunit *)``
>> @@ -141,6 +148,7 @@ struct kunit;
>>  struct kunit_case {
>>         void (*run_case)(struct kunit *test);
>>         const char *name;
>> +       void* (*generate_params)(void *prev);
>>
>>         /* private: internal use only. */
>>         bool success;
>> @@ -162,6 +170,9 @@ static inline char *kunit_status_to_string(bool status)
>>   * &struct kunit_case for an example on how to use it.
>>   */
>>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
> 
> I.e. create a new kernel-doc comment for KUNIT_CASE_PARAM here, and
> simply move the paragraph describing the generator protocol into that
> comment.
> 

I will make this change.

>> +#define KUNIT_CASE_PARAM(test_name, gen_params)                        \
>> +               { .run_case = test_name, .name = #test_name,    \
>> +                 .generate_params = gen_params }
>>
>>  /**
>>   * struct kunit_suite - describes a related collection of &struct kunit_case
>> @@ -208,6 +219,15 @@ struct kunit {
>>         const char *name; /* Read only after initialization! */
>>         char *log; /* Points at case log after initialization */
>>         struct kunit_try_catch try_catch;
>> +       /* param_value points to test case parameters in parameterized tests */
> 
> Hmm, not quite: param_value is the current parameter value for a test
> case. Most likely it's a pointer, but it doesn't need to be.
> 
>> +       void *param_value;
>> +       /*
>> +        * param_index stores the index of the parameter in
>> +        * parameterized tests. param_index + 1 is printed
>> +        * to indicate the parameter that causes the test
>> +        * to fail in case of test failure.
>> +        */
> 
> I think this comment needs to be reformatted, because you can use at
> the very least use 80 cols per line. (If you use vim, visual select
> and do 'gq'.)
> 
>> +       int param_index;
>>         /*
>>          * success starts as true, and may only be set to false during a
>>          * test case; thus, it is safe to update this across multiple
>> @@ -1742,4 +1762,18 @@ do {                                                                            \
>>                                                 fmt,                           \
>>                                                 ##__VA_ARGS__)
>>
>> +/**
>> + * KUNIT_ARRAY_PARAM() - Helper method for test parameter generators
>> + *                      required in parameterized tests.
>> + * @name:  prefix of the name for the test parameter generator function.
>> + *        It will be suffixed by "_gen_params".
>> + * @array: a user-supplied pointer to an array of test parameters.
>> + */
>> +#define KUNIT_ARRAY_PARAM(name, array)                                                         \
>> +       static void *name##_gen_params(void *prev)                                              \
>> +       {                                                                                       \
>> +               typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);     \
>> +               return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;                  \
>> +       }
>> +
>>  #endif /* _KUNIT_TEST_H */
>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
>> index 750704abe89a..8ad908b61494 100644
>> --- a/lib/kunit/test.c
>> +++ b/lib/kunit/test.c
>> @@ -127,6 +127,12 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite,
>>  }
>>  EXPORT_SYMBOL_GPL(kunit_test_case_num);
>>
>> +static void kunit_print_failed_param(struct kunit *test)
>> +{
>> +       kunit_err(test, "\n\tTest failed at:\n\ttest case: %s\n\tparameter: %d\n",
>> +                                               test->name, test->param_index + 1);
>> +}
> 
> Hmm, perhaps I wasn't clear, but I think I also misunderstood how the
> test case successes are presented: they are not, and it's all bunched
> into a single test case.
> 
> Firstly, kunit_err() already prints the test name, so if we want
> something like "  # : the_test_case_name: failed at parameter #X",
> simply having
> 
>     kunit_err(test, "failed at parameter #%d\n", test->param_index + 1)
> 
> would be what you want.
> 
> But I think I missed that parameters do not actually produce a set of
> test cases (sorry for noticing late). I think in their current form,
> the parameterized tests would not be useful for my tests, because each
> of my tests have test cases that have specific init and exit
> functions. For each parameter, these would also need to run.
> 
> Ideally, each parameter produces its own independent test case
> "test_case#param_index". That way, CI systems will also be able to
> logically separate different test case params, simply because each
> param produced its own distinct test case.
> 
> So, for example, we would get a series of test cases from something
> like KUNIT_CASE_PARAM(test_case, foo_gen_params), and in the output
> we'd see:
> 
>     ok X - test_case#1
>     ok X - test_case#2
>     ok X - test_case#3
>     ok X - test_case#4
>     ....
> 
> Would that make more sense?
> 
> That way we'd ensure that test-case specific initialization and
> cleanup done in init and exit functions is properly taken care of, and
> you wouldn't need kunit_print_failed_param().
> 
> AFAIK, for what I propose you'd have to modify kunit_print_ok_not_ok()
> (show param_index if parameterized test) and probably
> kunit_run_case_catch_errors() (generate params and set
> test->param_value and param_index).
> 
> Was there a reason why each param cannot be a distinct test case? If
> not, I think this would be more useful.
> 

Oh, I hadn't considered this earlier. I will try it out for the next version.

>>  static void kunit_print_string_stream(struct kunit *test,
>>                                       struct string_stream *stream)
>>  {
>> @@ -168,6 +174,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
>>         assert->format(assert, stream);
>>
>>         kunit_print_string_stream(test, stream);
>> +       if (test->param_value)
>> +               kunit_print_failed_param(test);
>>
>>         WARN_ON(string_stream_destroy(stream));
>>  }
>> @@ -239,7 +247,18 @@ static void kunit_run_case_internal(struct kunit *test,
>>                 }
>>         }
>>
>> -       test_case->run_case(test);
>> +       if (!test_case->generate_params) {
>> +               test_case->run_case(test);
>> +       } else {
>> +               test->param_value = test_case->generate_params(NULL);
>> +               test->param_index = 0;
>> +
>> +               while (test->param_value) {
>> +                       test_case->run_case(test);
>> +                       test->param_value = test_case->generate_params(test->param_value);
>> +                       test->param_index++;
>> +               }
>> +       }
> 
> Thanks,
> -- Marco
> 

I'll make all the suggested changes.
Thanks!

Arpitha Raghunandan Nov. 5, 2020, 7:31 a.m. UTC | #3

On 28/10/20 12:51 am, Marco Elver wrote:
> On Tue, 27 Oct 2020 at 18:47, Arpitha Raghunandan <[e-mail]> 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 <[e-mail]>
>> Co-developed-by: Marco Elver <[e-mail]>
>> Signed-off-by: Marco Elver <[e-mail]>
>> ---
>> 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 | 34 ++++++++++++++++++++++++++++++++++
>>  lib/kunit/test.c     | 21 ++++++++++++++++++++-
>>  2 files changed, 54 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index 9197da792336..ec2307ee9bb0 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -107,6 +107,13 @@ struct kunit;
>>   *
>>   * @run_case: the function representing the actual test case.
>>   * @name:     the name of the test case.
>> + * @generate_params: the generator function for parameterized tests.
>> + *
>> + * The generator function is used to lazily generate a series of
>> + * arbitrarily typed values that fit into a void*. The argument @prev
>> + * is the previously returned value, which should be used to derive the
>> + * next value; @prev is set to NULL on the initial generator call.
>> + * When no more values are available, the generator must return NULL.
>>   *
> 
> Hmm, should this really be the first paragraph? I think it should be
> the paragraph before "Example:" maybe. But then that paragraph should
> refer to generate_params e.g. "The generator function @generate_params
> is used to ........".
> 
> The other option you have is to move this paragraph to the kernel-doc
> comment for KUNIT_CASE_PARAM, which seems to be missing a kernel-doc
> comment.
> 
>>   * A test case is a function with the signature,
>>   * ``void (*)(struct kunit *)``
>> @@ -141,6 +148,7 @@ struct kunit;
>>  struct kunit_case {
>>         void (*run_case)(struct kunit *test);
>>         const char *name;
>> +       void* (*generate_params)(void *prev);
>>
>>         /* private: internal use only. */
>>         bool success;
>> @@ -162,6 +170,9 @@ static inline char *kunit_status_to_string(bool status)
>>   * &struct kunit_case for an example on how to use it.
>>   */
>>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
> 
> I.e. create a new kernel-doc comment for KUNIT_CASE_PARAM here, and
> simply move the paragraph describing the generator protocol into that
> comment.
> 
>> +#define KUNIT_CASE_PARAM(test_name, gen_params)                        \
>> +               { .run_case = test_name, .name = #test_name,    \
>> +                 .generate_params = gen_params }
>>
>>  /**
>>   * struct kunit_suite - describes a related collection of &struct kunit_case
>> @@ -208,6 +219,15 @@ struct kunit {
>>         const char *name; /* Read only after initialization! */
>>         char *log; /* Points at case log after initialization */
>>         struct kunit_try_catch try_catch;
>> +       /* param_value points to test case parameters in parameterized tests */
> 
> Hmm, not quite: param_value is the current parameter value for a test
> case. Most likely it's a pointer, but it doesn't need to be.
> 
>> +       void *param_value;
>> +       /*
>> +        * param_index stores the index of the parameter in
>> +        * parameterized tests. param_index + 1 is printed
>> +        * to indicate the parameter that causes the test
>> +        * to fail in case of test failure.
>> +        */
> 
> I think this comment needs to be reformatted, because you can use at
> the very least use 80 cols per line. (If you use vim, visual select
> and do 'gq'.)
> 
>> +       int param_index;
>>         /*
>>          * success starts as true, and may only be set to false during a
>>          * test case; thus, it is safe to update this across multiple
>> @@ -1742,4 +1762,18 @@ do {                                                                            \
>>                                                 fmt,                           \
>>                                                 ##__VA_ARGS__)
>>
>> +/**
>> + * KUNIT_ARRAY_PARAM() - Helper method for test parameter generators
>> + *                      required in parameterized tests.
>> + * @name:  prefix of the name for the test parameter generator function.
>> + *        It will be suffixed by "_gen_params".
>> + * @array: a user-supplied pointer to an array of test parameters.
>> + */
>> +#define KUNIT_ARRAY_PARAM(name, array)                                                         \
>> +       static void *name##_gen_params(void *prev)                                              \
>> +       {                                                                                       \
>> +               typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);     \
>> +               return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;                  \
>> +       }
>> +
>>  #endif /* _KUNIT_TEST_H */
>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
>> index 750704abe89a..8ad908b61494 100644
>> --- a/lib/kunit/test.c
>> +++ b/lib/kunit/test.c
>> @@ -127,6 +127,12 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite,
>>  }
>>  EXPORT_SYMBOL_GPL(kunit_test_case_num);
>>
>> +static void kunit_print_failed_param(struct kunit *test)
>> +{
>> +       kunit_err(test, "\n\tTest failed at:\n\ttest case: %s\n\tparameter: %d\n",
>> +                                               test->name, test->param_index + 1);
>> +}
> 
> Hmm, perhaps I wasn't clear, but I think I also misunderstood how the
> test case successes are presented: they are not, and it's all bunched
> into a single test case.
> 
> Firstly, kunit_err() already prints the test name, so if we want
> something like "  # : the_test_case_name: failed at parameter #X",
> simply having
> 
>     kunit_err(test, "failed at parameter #%d\n", test->param_index + 1)
> 
> would be what you want.
> 
> But I think I missed that parameters do not actually produce a set of
> test cases (sorry for noticing late). I think in their current form,
> the parameterized tests would not be useful for my tests, because each
> of my tests have test cases that have specific init and exit
> functions. For each parameter, these would also need to run.
> 
> Ideally, each parameter produces its own independent test case
> "test_case#param_index". That way, CI systems will also be able to
> logically separate different test case params, simply because each
> param produced its own distinct test case.
> 
> So, for example, we would get a series of test cases from something
> like KUNIT_CASE_PARAM(test_case, foo_gen_params), and in the output
> we'd see:
> 
>     ok X - test_case#1
>     ok X - test_case#2
>     ok X - test_case#3
>     ok X - test_case#4
>     ....
> 
> Would that make more sense?
> 
> That way we'd ensure that test-case specific initialization and
> cleanup done in init and exit functions is properly taken care of, and
> you wouldn't need kunit_print_failed_param().
> 
> AFAIK, for what I propose you'd have to modify kunit_print_ok_not_ok()
> (show param_index if parameterized test) and probably
> kunit_run_case_catch_errors() (generate params and set
> test->param_value and param_index).
> 
> Was there a reason why each param cannot be a distinct test case? If
> not, I think this would be more useful.
> 

I tried adding support to run each parameter as a distinct test case by
making changes to kunit_run_case_catch_errors(). The issue here is that
since the results are displayed in KTAP format, this change will result in
each parameter being considered a subtest of another subtest (test case
in KUnit). To make this work, a lot of changes in other parts will be required,
and it will get complicated. Running all parameters as one test case seems
to be a better option right now. So for now, I will modify what is displayed
by kunit_err() in case of test failure.

>>  static void kunit_print_string_stream(struct kunit *test,
>>                                       struct string_stream *stream)
>>  {
>> @@ -168,6 +174,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
>>         assert->format(assert, stream);
>>
>>         kunit_print_string_stream(test, stream);
>> +       if (test->param_value)
>> +               kunit_print_failed_param(test);
>>
>>         WARN_ON(string_stream_destroy(stream));
>>  }
>> @@ -239,7 +247,18 @@ static void kunit_run_case_internal(struct kunit *test,
>>                 }
>>         }
>>
>> -       test_case->run_case(test);
>> +       if (!test_case->generate_params) {
>> +               test_case->run_case(test);
>> +       } else {
>> +               test->param_value = test_case->generate_params(NULL);
>> +               test->param_index = 0;
>> +
>> +               while (test->param_value) {
>> +                       test_case->run_case(test);
>> +                       test->param_value = test_case->generate_params(test->param_value);
>> +                       test->param_index++;
>> +               }
>> +       }
> 
> Thanks,
> -- Marco
>

Marco Elver Nov. 5, 2020, 8:30 a.m. UTC | #4

On Thu, 5 Nov 2020 at 08:32, Arpitha Raghunandan <[e-mail]> wrote:
>
> On 28/10/20 12:51 am, Marco Elver wrote:
> > On Tue, 27 Oct 2020 at 18:47, Arpitha Raghunandan <[e-mail]> 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 <[e-mail]>
> >> Co-developed-by: Marco Elver <[e-mail]>
> >> Signed-off-by: Marco Elver <[e-mail]>
> >> ---
> >> 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 | 34 ++++++++++++++++++++++++++++++++++
> >>  lib/kunit/test.c     | 21 ++++++++++++++++++++-
> >>  2 files changed, 54 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >> index 9197da792336..ec2307ee9bb0 100644
> >> --- a/include/kunit/test.h
> >> +++ b/include/kunit/test.h
> >> @@ -107,6 +107,13 @@ struct kunit;
> >>   *
> >>   * @run_case: the function representing the actual test case.
> >>   * @name:     the name of the test case.
> >> + * @generate_params: the generator function for parameterized tests.
> >> + *
> >> + * The generator function is used to lazily generate a series of
> >> + * arbitrarily typed values that fit into a void*. The argument @prev
> >> + * is the previously returned value, which should be used to derive the
> >> + * next value; @prev is set to NULL on the initial generator call.
> >> + * When no more values are available, the generator must return NULL.
> >>   *
> >
> > Hmm, should this really be the first paragraph? I think it should be
> > the paragraph before "Example:" maybe. But then that paragraph should
> > refer to generate_params e.g. "The generator function @generate_params
> > is used to ........".
> >
> > The other option you have is to move this paragraph to the kernel-doc
> > comment for KUNIT_CASE_PARAM, which seems to be missing a kernel-doc
> > comment.
> >
> >>   * A test case is a function with the signature,
> >>   * ``void (*)(struct kunit *)``
> >> @@ -141,6 +148,7 @@ struct kunit;
> >>  struct kunit_case {
> >>         void (*run_case)(struct kunit *test);
> >>         const char *name;
> >> +       void* (*generate_params)(void *prev);
> >>
> >>         /* private: internal use only. */
> >>         bool success;
> >> @@ -162,6 +170,9 @@ static inline char *kunit_status_to_string(bool status)
> >>   * &struct kunit_case for an example on how to use it.
> >>   */
> >>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
> >
> > I.e. create a new kernel-doc comment for KUNIT_CASE_PARAM here, and
> > simply move the paragraph describing the generator protocol into that
> > comment.
> >
> >> +#define KUNIT_CASE_PARAM(test_name, gen_params)                        \
> >> +               { .run_case = test_name, .name = #test_name,    \
> >> +                 .generate_params = gen_params }
> >>
> >>  /**
> >>   * struct kunit_suite - describes a related collection of &struct kunit_case
> >> @@ -208,6 +219,15 @@ struct kunit {
> >>         const char *name; /* Read only after initialization! */
> >>         char *log; /* Points at case log after initialization */
> >>         struct kunit_try_catch try_catch;
> >> +       /* param_value points to test case parameters in parameterized tests */
> >
> > Hmm, not quite: param_value is the current parameter value for a test
> > case. Most likely it's a pointer, but it doesn't need to be.
> >
> >> +       void *param_value;
> >> +       /*
> >> +        * param_index stores the index of the parameter in
> >> +        * parameterized tests. param_index + 1 is printed
> >> +        * to indicate the parameter that causes the test
> >> +        * to fail in case of test failure.
> >> +        */
> >
> > I think this comment needs to be reformatted, because you can use at
> > the very least use 80 cols per line. (If you use vim, visual select
> > and do 'gq'.)
> >
> >> +       int param_index;
> >>         /*
> >>          * success starts as true, and may only be set to false during a
> >>          * test case; thus, it is safe to update this across multiple
> >> @@ -1742,4 +1762,18 @@ do {                                                                            \
> >>                                                 fmt,                           \
> >>                                                 ##__VA_ARGS__)
> >>
> >> +/**
> >> + * KUNIT_ARRAY_PARAM() - Helper method for test parameter generators
> >> + *                      required in parameterized tests.
> >> + * @name:  prefix of the name for the test parameter generator function.
> >> + *        It will be suffixed by "_gen_params".
> >> + * @array: a user-supplied pointer to an array of test parameters.
> >> + */
> >> +#define KUNIT_ARRAY_PARAM(name, array)                                                         \
> >> +       static void *name##_gen_params(void *prev)                                              \
> >> +       {                                                                                       \
> >> +               typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);     \
> >> +               return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;                  \
> >> +       }
> >> +
> >>  #endif /* _KUNIT_TEST_H */
> >> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> >> index 750704abe89a..8ad908b61494 100644
> >> --- a/lib/kunit/test.c
> >> +++ b/lib/kunit/test.c
> >> @@ -127,6 +127,12 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite,
> >>  }
> >>  EXPORT_SYMBOL_GPL(kunit_test_case_num);
> >>
> >> +static void kunit_print_failed_param(struct kunit *test)
> >> +{
> >> +       kunit_err(test, "\n\tTest failed at:\n\ttest case: %s\n\tparameter: %d\n",
> >> +                                               test->name, test->param_index + 1);
> >> +}
> >
> > Hmm, perhaps I wasn't clear, but I think I also misunderstood how the
> > test case successes are presented: they are not, and it's all bunched
> > into a single test case.
> >
> > Firstly, kunit_err() already prints the test name, so if we want
> > something like "  # : the_test_case_name: failed at parameter #X",
> > simply having
> >
> >     kunit_err(test, "failed at parameter #%d\n", test->param_index + 1)
> >
> > would be what you want.
> >
> > But I think I missed that parameters do not actually produce a set of
> > test cases (sorry for noticing late). I think in their current form,
> > the parameterized tests would not be useful for my tests, because each
> > of my tests have test cases that have specific init and exit
> > functions. For each parameter, these would also need to run.
> >
> > Ideally, each parameter produces its own independent test case
> > "test_case#param_index". That way, CI systems will also be able to
> > logically separate different test case params, simply because each
> > param produced its own distinct test case.
> >
> > So, for example, we would get a series of test cases from something
> > like KUNIT_CASE_PARAM(test_case, foo_gen_params), and in the output
> > we'd see:
> >
> >     ok X - test_case#1
> >     ok X - test_case#2
> >     ok X - test_case#3
> >     ok X - test_case#4
> >     ....
> >
> > Would that make more sense?
> >
> > That way we'd ensure that test-case specific initialization and
> > cleanup done in init and exit functions is properly taken care of, and
> > you wouldn't need kunit_print_failed_param().
> >
> > AFAIK, for what I propose you'd have to modify kunit_print_ok_not_ok()
> > (show param_index if parameterized test) and probably
> > kunit_run_case_catch_errors() (generate params and set
> > test->param_value and param_index).
> >
> > Was there a reason why each param cannot be a distinct test case? If
> > not, I think this would be more useful.
> >
>
> I tried adding support to run each parameter as a distinct test case by
> making changes to kunit_run_case_catch_errors(). The issue here is that
> since the results are displayed in KTAP format, this change will result in
> each parameter being considered a subtest of another subtest (test case
> in KUnit).

Do you have example output? That might help understand what's going on.

> To make this work, a lot of changes in other parts will be required,
> and it will get complicated. Running all parameters as one test case seems
> to be a better option right now. So for now, I will modify what is displayed
> by kunit_err() in case of test failure.
>
> >>  static void kunit_print_string_stream(struct kunit *test,
> >>                                       struct string_stream *stream)
> >>  {
> >> @@ -168,6 +174,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
> >>         assert->format(assert, stream);
> >>
> >>         kunit_print_string_stream(test, stream);
> >> +       if (test->param_value)
> >> +               kunit_print_failed_param(test);
> >>
> >>         WARN_ON(string_stream_destroy(stream));
> >>  }
> >> @@ -239,7 +247,18 @@ static void kunit_run_case_internal(struct kunit *test,
> >>                 }
> >>         }
> >>
> >> -       test_case->run_case(test);
> >> +       if (!test_case->generate_params) {
> >> +               test_case->run_case(test);
> >> +       } else {
> >> +               test->param_value = test_case->generate_params(NULL);
> >> +               test->param_index = 0;
> >> +
> >> +               while (test->param_value) {
> >> +                       test_case->run_case(test);
> >> +                       test->param_value = test_case->generate_params(test->param_value);
> >> +                       test->param_index++;
> >> +               }
> >> +       }
> >
> > Thanks,
> > -- Marco
> >
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [e-mail].
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/73c4e46c-10f1-9362-b4fb-94ea9d74e9b2%40gmail.com.

Arpitha Raghunandan Nov. 5, 2020, 2:30 p.m. UTC | #5

On 05/11/20 2:00 pm, Marco Elver wrote:
> On Thu, 5 Nov 2020 at 08:32, Arpitha Raghunandan <[e-mail]> wrote:
>>
>> On 28/10/20 12:51 am, Marco Elver wrote:
>>> On Tue, 27 Oct 2020 at 18:47, Arpitha Raghunandan <[e-mail]> 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 <[e-mail]>
>>>> Co-developed-by: Marco Elver <[e-mail]>
>>>> Signed-off-by: Marco Elver <[e-mail]>
>>>> ---
>>>> 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 | 34 ++++++++++++++++++++++++++++++++++
>>>>  lib/kunit/test.c     | 21 ++++++++++++++++++++-
>>>>  2 files changed, 54 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>>> index 9197da792336..ec2307ee9bb0 100644
>>>> --- a/include/kunit/test.h
>>>> +++ b/include/kunit/test.h
>>>> @@ -107,6 +107,13 @@ struct kunit;
>>>>   *
>>>>   * @run_case: the function representing the actual test case.
>>>>   * @name:     the name of the test case.
>>>> + * @generate_params: the generator function for parameterized tests.
>>>> + *
>>>> + * The generator function is used to lazily generate a series of
>>>> + * arbitrarily typed values that fit into a void*. The argument @prev
>>>> + * is the previously returned value, which should be used to derive the
>>>> + * next value; @prev is set to NULL on the initial generator call.
>>>> + * When no more values are available, the generator must return NULL.
>>>>   *
>>>
>>> Hmm, should this really be the first paragraph? I think it should be
>>> the paragraph before "Example:" maybe. But then that paragraph should
>>> refer to generate_params e.g. "The generator function @generate_params
>>> is used to ........".
>>>
>>> The other option you have is to move this paragraph to the kernel-doc
>>> comment for KUNIT_CASE_PARAM, which seems to be missing a kernel-doc
>>> comment.
>>>
>>>>   * A test case is a function with the signature,
>>>>   * ``void (*)(struct kunit *)``
>>>> @@ -141,6 +148,7 @@ struct kunit;
>>>>  struct kunit_case {
>>>>         void (*run_case)(struct kunit *test);
>>>>         const char *name;
>>>> +       void* (*generate_params)(void *prev);
>>>>
>>>>         /* private: internal use only. */
>>>>         bool success;
>>>> @@ -162,6 +170,9 @@ static inline char *kunit_status_to_string(bool status)
>>>>   * &struct kunit_case for an example on how to use it.
>>>>   */
>>>>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
>>>
>>> I.e. create a new kernel-doc comment for KUNIT_CASE_PARAM here, and
>>> simply move the paragraph describing the generator protocol into that
>>> comment.
>>>
>>>> +#define KUNIT_CASE_PARAM(test_name, gen_params)                        \
>>>> +               { .run_case = test_name, .name = #test_name,    \
>>>> +                 .generate_params = gen_params }
>>>>
>>>>  /**
>>>>   * struct kunit_suite - describes a related collection of &struct kunit_case
>>>> @@ -208,6 +219,15 @@ struct kunit {
>>>>         const char *name; /* Read only after initialization! */
>>>>         char *log; /* Points at case log after initialization */
>>>>         struct kunit_try_catch try_catch;
>>>> +       /* param_value points to test case parameters in parameterized tests */
>>>
>>> Hmm, not quite: param_value is the current parameter value for a test
>>> case. Most likely it's a pointer, but it doesn't need to be.
>>>
>>>> +       void *param_value;
>>>> +       /*
>>>> +        * param_index stores the index of the parameter in
>>>> +        * parameterized tests. param_index + 1 is printed
>>>> +        * to indicate the parameter that causes the test
>>>> +        * to fail in case of test failure.
>>>> +        */
>>>
>>> I think this comment needs to be reformatted, because you can use at
>>> the very least use 80 cols per line. (If you use vim, visual select
>>> and do 'gq'.)
>>>
>>>> +       int param_index;
>>>>         /*
>>>>          * success starts as true, and may only be set to false during a
>>>>          * test case; thus, it is safe to update this across multiple
>>>> @@ -1742,4 +1762,18 @@ do {                                                                            \
>>>>                                                 fmt,                           \
>>>>                                                 ##__VA_ARGS__)
>>>>
>>>> +/**
>>>> + * KUNIT_ARRAY_PARAM() - Helper method for test parameter generators
>>>> + *                      required in parameterized tests.
>>>> + * @name:  prefix of the name for the test parameter generator function.
>>>> + *        It will be suffixed by "_gen_params".
>>>> + * @array: a user-supplied pointer to an array of test parameters.
>>>> + */
>>>> +#define KUNIT_ARRAY_PARAM(name, array)                                                         \
>>>> +       static void *name##_gen_params(void *prev)                                              \
>>>> +       {                                                                                       \
>>>> +               typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);     \
>>>> +               return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;                  \
>>>> +       }
>>>> +
>>>>  #endif /* _KUNIT_TEST_H */
>>>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
>>>> index 750704abe89a..8ad908b61494 100644
>>>> --- a/lib/kunit/test.c
>>>> +++ b/lib/kunit/test.c
>>>> @@ -127,6 +127,12 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite,
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(kunit_test_case_num);
>>>>
>>>> +static void kunit_print_failed_param(struct kunit *test)
>>>> +{
>>>> +       kunit_err(test, "\n\tTest failed at:\n\ttest case: %s\n\tparameter: %d\n",
>>>> +                                               test->name, test->param_index + 1);
>>>> +}
>>>
>>> Hmm, perhaps I wasn't clear, but I think I also misunderstood how the
>>> test case successes are presented: they are not, and it's all bunched
>>> into a single test case.
>>>
>>> Firstly, kunit_err() already prints the test name, so if we want
>>> something like "  # : the_test_case_name: failed at parameter #X",
>>> simply having
>>>
>>>     kunit_err(test, "failed at parameter #%d\n", test->param_index + 1)
>>>
>>> would be what you want.
>>>
>>> But I think I missed that parameters do not actually produce a set of
>>> test cases (sorry for noticing late). I think in their current form,
>>> the parameterized tests would not be useful for my tests, because each
>>> of my tests have test cases that have specific init and exit
>>> functions. For each parameter, these would also need to run.
>>>
>>> Ideally, each parameter produces its own independent test case
>>> "test_case#param_index". That way, CI systems will also be able to
>>> logically separate different test case params, simply because each
>>> param produced its own distinct test case.
>>>
>>> So, for example, we would get a series of test cases from something
>>> like KUNIT_CASE_PARAM(test_case, foo_gen_params), and in the output
>>> we'd see:
>>>
>>>     ok X - test_case#1
>>>     ok X - test_case#2
>>>     ok X - test_case#3
>>>     ok X - test_case#4
>>>     ....
>>>
>>> Would that make more sense?
>>>
>>> That way we'd ensure that test-case specific initialization and
>>> cleanup done in init and exit functions is properly taken care of, and
>>> you wouldn't need kunit_print_failed_param().
>>>
>>> AFAIK, for what I propose you'd have to modify kunit_print_ok_not_ok()
>>> (show param_index if parameterized test) and probably
>>> kunit_run_case_catch_errors() (generate params and set
>>> test->param_value and param_index).
>>>
>>> Was there a reason why each param cannot be a distinct test case? If
>>> not, I think this would be more useful.
>>>
>>
>> I tried adding support to run each parameter as a distinct test case by
>> making changes to kunit_run_case_catch_errors(). The issue here is that
>> since the results are displayed in KTAP format, this change will result in
>> each parameter being considered a subtest of another subtest (test case
>> in KUnit).
> 
> Do you have example output? That might help understand what's going on.
> 

The change that I tried can be seen here (based on the v4 patch):
https://gist.github.com/arpi-r/4822899087ca4cc34572ed9e45cc5fee.

Using the kunit tool, I get this error:

[19:20:41] [ERROR]  expected 7 test suites, but got -1
[ERROR] no tests run!
[19:20:41] ============================================================
[19:20:41] Testing complete. 0 tests run. 0 failed. 0 crashed.

But this error is only because of how the tool displays the results.
The test actually does run, as can be seen in the dmesg output:

TAP version 14
1..7
    # Subtest: ext4_inode_test
    1..1
    ok 1 - inode_test_xtimestamp_decoding 1
    ok 1 - inode_test_xtimestamp_decoding 2
    ok 1 - inode_test_xtimestamp_decoding 3
    ok 1 - inode_test_xtimestamp_decoding 4
    ok 1 - inode_test_xtimestamp_decoding 5
    ok 1 - inode_test_xtimestamp_decoding 6
    ok 1 - inode_test_xtimestamp_decoding 7
    ok 1 - inode_test_xtimestamp_decoding 8
    ok 1 - inode_test_xtimestamp_decoding 9
    ok 1 - inode_test_xtimestamp_decoding 10
    ok 1 - inode_test_xtimestamp_decoding 11
    ok 1 - inode_test_xtimestamp_decoding 12
    ok 1 - inode_test_xtimestamp_decoding 13
    ok 1 - inode_test_xtimestamp_decoding 14
    ok 1 - inode_test_xtimestamp_decoding 15
    ok 1 - inode_test_xtimestamp_decoding 16
ok 1 - ext4_inode_test
(followed by other kunit test outputs)

>> To make this work, a lot of changes in other parts will be required,
>> and it will get complicated. Running all parameters as one test case seems
>> to be a better option right now. So for now, I will modify what is displayed
>> by kunit_err() in case of test failure.
>>
>>>>  static void kunit_print_string_stream(struct kunit *test,
>>>>                                       struct string_stream *stream)
>>>>  {
>>>> @@ -168,6 +174,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
>>>>         assert->format(assert, stream);
>>>>
>>>>         kunit_print_string_stream(test, stream);
>>>> +       if (test->param_value)
>>>> +               kunit_print_failed_param(test);
>>>>
>>>>         WARN_ON(string_stream_destroy(stream));
>>>>  }
>>>> @@ -239,7 +247,18 @@ static void kunit_run_case_internal(struct kunit *test,
>>>>                 }
>>>>         }
>>>>
>>>> -       test_case->run_case(test);
>>>> +       if (!test_case->generate_params) {
>>>> +               test_case->run_case(test);
>>>> +       } else {
>>>> +               test->param_value = test_case->generate_params(NULL);
>>>> +               test->param_index = 0;
>>>> +
>>>> +               while (test->param_value) {
>>>> +                       test_case->run_case(test);
>>>> +                       test->param_value = test_case->generate_params(test->param_value);
>>>> +                       test->param_index++;
>>>> +               }
>>>> +       }
>>>
>>> Thanks,
>>> -- Marco
>>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to [e-mail].
>> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/73c4e46c-10f1-9362-b4fb-94ea9d74e9b2%40gmail.com.

Marco Elver Nov. 5, 2020, 3:02 p.m. UTC | #6

On Thu, 5 Nov 2020 at 15:30, Arpitha Raghunandan <[e-mail]> wrote:
>
> On 05/11/20 2:00 pm, Marco Elver wrote:
> > On Thu, 5 Nov 2020 at 08:32, Arpitha Raghunandan <[e-mail]> wrote:
> >>
> >> On 28/10/20 12:51 am, Marco Elver wrote:
> >>> On Tue, 27 Oct 2020 at 18:47, Arpitha Raghunandan <[e-mail]> 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 <[e-mail]>
> >>>> Co-developed-by: Marco Elver <[e-mail]>
> >>>> Signed-off-by: Marco Elver <[e-mail]>
> >>>> ---
> >>>> 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 | 34 ++++++++++++++++++++++++++++++++++
> >>>>  lib/kunit/test.c     | 21 ++++++++++++++++++++-
> >>>>  2 files changed, 54 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >>>> index 9197da792336..ec2307ee9bb0 100644
> >>>> --- a/include/kunit/test.h
> >>>> +++ b/include/kunit/test.h
> >>>> @@ -107,6 +107,13 @@ struct kunit;
> >>>>   *
> >>>>   * @run_case: the function representing the actual test case.
> >>>>   * @name:     the name of the test case.
> >>>> + * @generate_params: the generator function for parameterized tests.
> >>>> + *
> >>>> + * The generator function is used to lazily generate a series of
> >>>> + * arbitrarily typed values that fit into a void*. The argument @prev
> >>>> + * is the previously returned value, which should be used to derive the
> >>>> + * next value; @prev is set to NULL on the initial generator call.
> >>>> + * When no more values are available, the generator must return NULL.
> >>>>   *
> >>>
> >>> Hmm, should this really be the first paragraph? I think it should be
> >>> the paragraph before "Example:" maybe. But then that paragraph should
> >>> refer to generate_params e.g. "The generator function @generate_params
> >>> is used to ........".
> >>>
> >>> The other option you have is to move this paragraph to the kernel-doc
> >>> comment for KUNIT_CASE_PARAM, which seems to be missing a kernel-doc
> >>> comment.
> >>>
> >>>>   * A test case is a function with the signature,
> >>>>   * ``void (*)(struct kunit *)``
> >>>> @@ -141,6 +148,7 @@ struct kunit;
> >>>>  struct kunit_case {
> >>>>         void (*run_case)(struct kunit *test);
> >>>>         const char *name;
> >>>> +       void* (*generate_params)(void *prev);
> >>>>
> >>>>         /* private: internal use only. */
> >>>>         bool success;
> >>>> @@ -162,6 +170,9 @@ static inline char *kunit_status_to_string(bool status)
> >>>>   * &struct kunit_case for an example on how to use it.
> >>>>   */
> >>>>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
> >>>
> >>> I.e. create a new kernel-doc comment for KUNIT_CASE_PARAM here, and
> >>> simply move the paragraph describing the generator protocol into that
> >>> comment.
> >>>
> >>>> +#define KUNIT_CASE_PARAM(test_name, gen_params)                        \
> >>>> +               { .run_case = test_name, .name = #test_name,    \
> >>>> +                 .generate_params = gen_params }
> >>>>
> >>>>  /**
> >>>>   * struct kunit_suite - describes a related collection of &struct kunit_case
> >>>> @@ -208,6 +219,15 @@ struct kunit {
> >>>>         const char *name; /* Read only after initialization! */
> >>>>         char *log; /* Points at case log after initialization */
> >>>>         struct kunit_try_catch try_catch;
> >>>> +       /* param_value points to test case parameters in parameterized tests */
> >>>
> >>> Hmm, not quite: param_value is the current parameter value for a test
> >>> case. Most likely it's a pointer, but it doesn't need to be.
> >>>
> >>>> +       void *param_value;
> >>>> +       /*
> >>>> +        * param_index stores the index of the parameter in
> >>>> +        * parameterized tests. param_index + 1 is printed
> >>>> +        * to indicate the parameter that causes the test
> >>>> +        * to fail in case of test failure.
> >>>> +        */
> >>>
> >>> I think this comment needs to be reformatted, because you can use at
> >>> the very least use 80 cols per line. (If you use vim, visual select
> >>> and do 'gq'.)
> >>>
> >>>> +       int param_index;
> >>>>         /*
> >>>>          * success starts as true, and may only be set to false during a
> >>>>          * test case; thus, it is safe to update this across multiple
> >>>> @@ -1742,4 +1762,18 @@ do {                                                                            \
> >>>>                                                 fmt,                           \
> >>>>                                                 ##__VA_ARGS__)
> >>>>
> >>>> +/**
> >>>> + * KUNIT_ARRAY_PARAM() - Helper method for test parameter generators
> >>>> + *                      required in parameterized tests.
> >>>> + * @name:  prefix of the name for the test parameter generator function.
> >>>> + *        It will be suffixed by "_gen_params".
> >>>> + * @array: a user-supplied pointer to an array of test parameters.
> >>>> + */
> >>>> +#define KUNIT_ARRAY_PARAM(name, array)                                                         \
> >>>> +       static void *name##_gen_params(void *prev)                                              \
> >>>> +       {                                                                                       \
> >>>> +               typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);     \
> >>>> +               return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;                  \
> >>>> +       }
> >>>> +
> >>>>  #endif /* _KUNIT_TEST_H */
> >>>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> >>>> index 750704abe89a..8ad908b61494 100644
> >>>> --- a/lib/kunit/test.c
> >>>> +++ b/lib/kunit/test.c
> >>>> @@ -127,6 +127,12 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite,
> >>>>  }
> >>>>  EXPORT_SYMBOL_GPL(kunit_test_case_num);
> >>>>
> >>>> +static void kunit_print_failed_param(struct kunit *test)
> >>>> +{
> >>>> +       kunit_err(test, "\n\tTest failed at:\n\ttest case: %s\n\tparameter: %d\n",
> >>>> +                                               test->name, test->param_index + 1);
> >>>> +}
> >>>
> >>> Hmm, perhaps I wasn't clear, but I think I also misunderstood how the
> >>> test case successes are presented: they are not, and it's all bunched
> >>> into a single test case.
> >>>
> >>> Firstly, kunit_err() already prints the test name, so if we want
> >>> something like "  # : the_test_case_name: failed at parameter #X",
> >>> simply having
> >>>
> >>>     kunit_err(test, "failed at parameter #%d\n", test->param_index + 1)
> >>>
> >>> would be what you want.
> >>>
> >>> But I think I missed that parameters do not actually produce a set of
> >>> test cases (sorry for noticing late). I think in their current form,
> >>> the parameterized tests would not be useful for my tests, because each
> >>> of my tests have test cases that have specific init and exit
> >>> functions. For each parameter, these would also need to run.
> >>>
> >>> Ideally, each parameter produces its own independent test case
> >>> "test_case#param_index". That way, CI systems will also be able to
> >>> logically separate different test case params, simply because each
> >>> param produced its own distinct test case.
> >>>
> >>> So, for example, we would get a series of test cases from something
> >>> like KUNIT_CASE_PARAM(test_case, foo_gen_params), and in the output
> >>> we'd see:
> >>>
> >>>     ok X - test_case#1
> >>>     ok X - test_case#2
> >>>     ok X - test_case#3
> >>>     ok X - test_case#4
> >>>     ....
> >>>
> >>> Would that make more sense?
> >>>
> >>> That way we'd ensure that test-case specific initialization and
> >>> cleanup done in init and exit functions is properly taken care of, and
> >>> you wouldn't need kunit_print_failed_param().
> >>>
> >>> AFAIK, for what I propose you'd have to modify kunit_print_ok_not_ok()
> >>> (show param_index if parameterized test) and probably
> >>> kunit_run_case_catch_errors() (generate params and set
> >>> test->param_value and param_index).
> >>>
> >>> Was there a reason why each param cannot be a distinct test case? If
> >>> not, I think this would be more useful.
> >>>
> >>
> >> I tried adding support to run each parameter as a distinct test case by
> >> making changes to kunit_run_case_catch_errors(). The issue here is that
> >> since the results are displayed in KTAP format, this change will result in
> >> each parameter being considered a subtest of another subtest (test case
> >> in KUnit).
> >
> > Do you have example output? That might help understand what's going on.
> >
>
> The change that I tried can be seen here (based on the v4 patch):
> https://gist.github.com/arpi-r/4822899087ca4cc34572ed9e45cc5fee.
>
> Using the kunit tool, I get this error:
>
> [19:20:41] [ERROR]  expected 7 test suites, but got -1
> [ERROR] no tests run!
> [19:20:41] ============================================================
> [19:20:41] Testing complete. 0 tests run. 0 failed. 0 crashed.
>
> But this error is only because of how the tool displays the results.
> The test actually does run, as can be seen in the dmesg output:
>
> TAP version 14
> 1..7
>     # Subtest: ext4_inode_test
>     1..1
>     ok 1 - inode_test_xtimestamp_decoding 1
>     ok 1 - inode_test_xtimestamp_decoding 2
>     ok 1 - inode_test_xtimestamp_decoding 3
>     ok 1 - inode_test_xtimestamp_decoding 4
>     ok 1 - inode_test_xtimestamp_decoding 5
>     ok 1 - inode_test_xtimestamp_decoding 6
>     ok 1 - inode_test_xtimestamp_decoding 7
>     ok 1 - inode_test_xtimestamp_decoding 8
>     ok 1 - inode_test_xtimestamp_decoding 9
>     ok 1 - inode_test_xtimestamp_decoding 10
>     ok 1 - inode_test_xtimestamp_decoding 11
>     ok 1 - inode_test_xtimestamp_decoding 12
>     ok 1 - inode_test_xtimestamp_decoding 13
>     ok 1 - inode_test_xtimestamp_decoding 14
>     ok 1 - inode_test_xtimestamp_decoding 15
>     ok 1 - inode_test_xtimestamp_decoding 16
> ok 1 - ext4_inode_test
> (followed by other kunit test outputs)

Hmm, interesting. Let me play with your patch a bit.

One option is to just have the test case number increment as well,
i.e. have this:
|    ok 1 - inode_test_xtimestamp_decoding#1
|    ok 2 - inode_test_xtimestamp_decoding#2
|    ok 3 - inode_test_xtimestamp_decoding#3
|    ok 4 - inode_test_xtimestamp_decoding#4
|    ok 5 - inode_test_xtimestamp_decoding#5
...

Or is there something else I missed?

Thanks,
-- Marco

Marco Elver Nov. 5, 2020, 7:55 p.m. UTC | #7

On Thu, Nov 05, 2020 at 04:02PM +0100, Marco Elver wrote:
> On Thu, 5 Nov 2020 at 15:30, Arpitha Raghunandan <[e-mail]> wrote:
[...]
> > >> I tried adding support to run each parameter as a distinct test case by
> > >> making changes to kunit_run_case_catch_errors(). The issue here is that
> > >> since the results are displayed in KTAP format, this change will result in
> > >> each parameter being considered a subtest of another subtest (test case
> > >> in KUnit).
> > >
> > > Do you have example output? That might help understand what's going on.
> > >
> >
> > The change that I tried can be seen here (based on the v4 patch):
> > https://gist.github.com/arpi-r/4822899087ca4cc34572ed9e45cc5fee.
> >
> > Using the kunit tool, I get this error:
> >
> > [19:20:41] [ERROR]  expected 7 test suites, but got -1
> > [ERROR] no tests run!
> > [19:20:41] ============================================================
> > [19:20:41] Testing complete. 0 tests run. 0 failed. 0 crashed.
> >
> > But this error is only because of how the tool displays the results.
> > The test actually does run, as can be seen in the dmesg output:
> >
> > TAP version 14
> > 1..7
> >     # Subtest: ext4_inode_test
> >     1..1
> >     ok 1 - inode_test_xtimestamp_decoding 1
> >     ok 1 - inode_test_xtimestamp_decoding 2
> >     ok 1 - inode_test_xtimestamp_decoding 3
> >     ok 1 - inode_test_xtimestamp_decoding 4
> >     ok 1 - inode_test_xtimestamp_decoding 5
> >     ok 1 - inode_test_xtimestamp_decoding 6
> >     ok 1 - inode_test_xtimestamp_decoding 7
> >     ok 1 - inode_test_xtimestamp_decoding 8
> >     ok 1 - inode_test_xtimestamp_decoding 9
> >     ok 1 - inode_test_xtimestamp_decoding 10
> >     ok 1 - inode_test_xtimestamp_decoding 11
> >     ok 1 - inode_test_xtimestamp_decoding 12
> >     ok 1 - inode_test_xtimestamp_decoding 13
> >     ok 1 - inode_test_xtimestamp_decoding 14
> >     ok 1 - inode_test_xtimestamp_decoding 15
> >     ok 1 - inode_test_xtimestamp_decoding 16
> > ok 1 - ext4_inode_test
> > (followed by other kunit test outputs)
> 
> Hmm, interesting. Let me play with your patch a bit.
> 
> One option is to just have the test case number increment as well,
> i.e. have this:
> |    ok 1 - inode_test_xtimestamp_decoding#1
> |    ok 2 - inode_test_xtimestamp_decoding#2
> |    ok 3 - inode_test_xtimestamp_decoding#3
> |    ok 4 - inode_test_xtimestamp_decoding#4
> |    ok 5 - inode_test_xtimestamp_decoding#5
> ...
> 
> Or is there something else I missed?

Right, so TAP wants the exact number of tests it will run ahead of time.
In which case we can still put the results of each parameterized test in
a diagnostic. Please see my proposed patch below, which still does
proper initialization/destruction of each parameter case as if it was
its own test case.

With it the output looks as follows:

| TAP version 14
| 1..6
|     # Subtest: ext4_inode_test
|     1..1
|     # ok param#0 - inode_test_xtimestamp_decoding
|     # ok param#1 - inode_test_xtimestamp_decoding
|     # ok param#2 - inode_test_xtimestamp_decoding
|     # ok param#3 - inode_test_xtimestamp_decoding
|     # ok param#4 - inode_test_xtimestamp_decoding
|     # ok param#5 - inode_test_xtimestamp_decoding
|     # ok param#6 - inode_test_xtimestamp_decoding
|     # ok param#7 - inode_test_xtimestamp_decoding
|     # ok param#8 - inode_test_xtimestamp_decoding
|     # ok param#9 - inode_test_xtimestamp_decoding
|     # ok param#10 - inode_test_xtimestamp_decoding
|     # ok param#11 - inode_test_xtimestamp_decoding
|     # ok param#12 - inode_test_xtimestamp_decoding
|     # ok param#13 - inode_test_xtimestamp_decoding
|     # ok param#14 - inode_test_xtimestamp_decoding
|     # ok param#15 - inode_test_xtimestamp_decoding
|     ok 1 - inode_test_xtimestamp_decoding
| ok 1 - ext4_inode_test

Would that be reasonable? If so, feel free to take the patch and
test/adjust as required.

I'm not sure on the best format -- is there is a recommended format for
parameterized test result output? If not, I suppose we can put anything
we like into the diagnostic.

Thanks,
-- Marco

------ >8 ------

From ccbf4e2e190a2d7c6a94a51c9b1fb3b9a940e578 Mon Sep 17 00:00:00 2001
From: Arpitha Raghunandan <[e-mail]>
Date: Tue, 27 Oct 2020 23:16:30 +0530
Subject: [PATCH] kunit: Support for Parameterized Testing

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 <[e-mail]>
Co-developed-by: Marco Elver <[e-mail]>
Signed-off-by: Marco Elver <[e-mail]>
---
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 9197da792336..ae5488a37e48 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -107,6 +107,7 @@ struct kunit;
  *
  * @run_case: the function representing the actual test case.
  * @name:     the name of the test case.
+ * @generate_params: the generator function for parameterized tests.
  *
  * A test case is a function with the signature,
  * ``void (*)(struct kunit *)``
@@ -141,6 +142,7 @@ struct kunit;
 struct kunit_case {
 	void (*run_case)(struct kunit *test);
 	const char *name;
+	const void* (*generate_params)(const void *prev);
 
 	/* private: internal use only. */
 	bool success;
@@ -163,6 +165,22 @@ static inline char *kunit_status_to_string(bool status)
  */
 #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
 
+/**
+ * KUNIT_CASE_PARAM - A helper for creation a parameterized &struct kunit_case
+ *
+ * @test_name: a reference to a test case function.
+ * @gen_params: a reference to a parameter generator function.
+ *
+ * The generator function ``const void* gen_params(const void *prev)`` is used
+ * to lazily generate a series of arbitrarily typed values that fit into a
+ * void*. The argument @prev is the previously returned value, which should be
+ * used to derive the next value; @prev is set to NULL on the initial generator
+ * call.  When no more values are available, the generator must return NULL.
+ */
+#define KUNIT_CASE_PARAM(test_name, gen_params)			\
+		{ .run_case = test_name, .name = #test_name,	\
+		  .generate_params = gen_params }
+
 /**
  * struct kunit_suite - describes a related collection of &struct kunit_case
  *
@@ -208,6 +226,10 @@ struct kunit {
 	const char *name; /* Read only after initialization! */
 	char *log; /* Points at case log after initialization */
 	struct kunit_try_catch try_catch;
+	/* param_value is the current parameter value for a test case. */
+	const void *param_value;
+	/* param_index stores the index of the parameter in parameterized tests. */
+	int param_index;
 	/*
 	 * success starts as true, and may only be set to false during a
 	 * test case; thus, it is safe to update this across multiple
@@ -1742,4 +1764,18 @@ do {									       \
 						fmt,			       \
 						##__VA_ARGS__)
 
+/**
+ * KUNIT_ARRAY_PARAM() - Define test parameter generator from an array.
+ * @name:  prefix for the test parameter generator function.
+ * @array: array of test parameters.
+ *
+ * Define function @name_gen_params which uses @array to generate parameters.
+ */
+#define KUNIT_ARRAY_PARAM(name, array)								\
+	static const void *name##_gen_params(const void *prev)					\
+	{											\
+		typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);	\
+		return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;			\
+	}
+
 #endif /* _KUNIT_TEST_H */
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 750704abe89a..453ebe4da77d 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -325,29 +325,25 @@ static void kunit_catch_run_case(void *data)
  * occur in a test case and reports them as failures.
  */
 static void kunit_run_case_catch_errors(struct kunit_suite *suite,
-					struct kunit_case *test_case)
+					struct kunit_case *test_case,
+					struct kunit *test)
 {
 	struct kunit_try_catch_context context;
 	struct kunit_try_catch *try_catch;
-	struct kunit test;
 
-	kunit_init_test(&test, test_case->name, test_case->log);
-	try_catch = &test.try_catch;
+	kunit_init_test(test, test_case->name, test_case->log);
+	try_catch = &test->try_catch;
 
 	kunit_try_catch_init(try_catch,
-			     &test,
+			     test,
 			     kunit_try_run_case,
 			     kunit_catch_run_case);
-	context.test = &test;
+	context.test = test;
 	context.suite = suite;
 	context.test_case = test_case;
 	kunit_try_catch_run(try_catch, &context);
 
-	test_case->success = test.success;
-
-	kunit_print_ok_not_ok(&test, true, test_case->success,
-			      kunit_test_case_num(suite, test_case),
-			      test_case->name);
+	test_case->success = test->success;
 }
 
 int kunit_run_tests(struct kunit_suite *suite)
@@ -356,8 +352,32 @@ int kunit_run_tests(struct kunit_suite *suite)
 
 	kunit_print_subtest_start(suite);
 
-	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",
+					  kunit_status_to_string(test.success),
+					  test.param_index, test_case->name);
+				test.param_value = test_case->generate_params(test.param_value);
+				test.param_index++;
+			}
+		} while (test.param_value);
+
+		kunit_print_ok_not_ok(&test, true, test_success,
+				      kunit_test_case_num(suite, test_case),
+				      test_case->name);
+	}
 
 	kunit_print_subtest_end(suite);

Arpitha Raghunandan Nov. 6, 2020, 5:54 a.m. UTC | #8

On 06/11/20 1:25 am, Marco Elver wrote:
> On Thu, Nov 05, 2020 at 04:02PM +0100, Marco Elver wrote:
>> On Thu, 5 Nov 2020 at 15:30, Arpitha Raghunandan <[e-mail]> wrote:
> [...]
>>>>> I tried adding support to run each parameter as a distinct test case by
>>>>> making changes to kunit_run_case_catch_errors(). The issue here is that
>>>>> since the results are displayed in KTAP format, this change will result in
>>>>> each parameter being considered a subtest of another subtest (test case
>>>>> in KUnit).
>>>>
>>>> Do you have example output? That might help understand what's going on.
>>>>
>>>
>>> The change that I tried can be seen here (based on the v4 patch):
>>> https://gist.github.com/arpi-r/4822899087ca4cc34572ed9e45cc5fee.
>>>
>>> Using the kunit tool, I get this error:
>>>
>>> [19:20:41] [ERROR]  expected 7 test suites, but got -1
>>> [ERROR] no tests run!
>>> [19:20:41] ============================================================
>>> [19:20:41] Testing complete. 0 tests run. 0 failed. 0 crashed.
>>>
>>> But this error is only because of how the tool displays the results.
>>> The test actually does run, as can be seen in the dmesg output:
>>>
>>> TAP version 14
>>> 1..7
>>>     # Subtest: ext4_inode_test
>>>     1..1
>>>     ok 1 - inode_test_xtimestamp_decoding 1
>>>     ok 1 - inode_test_xtimestamp_decoding 2
>>>     ok 1 - inode_test_xtimestamp_decoding 3
>>>     ok 1 - inode_test_xtimestamp_decoding 4
>>>     ok 1 - inode_test_xtimestamp_decoding 5
>>>     ok 1 - inode_test_xtimestamp_decoding 6
>>>     ok 1 - inode_test_xtimestamp_decoding 7
>>>     ok 1 - inode_test_xtimestamp_decoding 8
>>>     ok 1 - inode_test_xtimestamp_decoding 9
>>>     ok 1 - inode_test_xtimestamp_decoding 10
>>>     ok 1 - inode_test_xtimestamp_decoding 11
>>>     ok 1 - inode_test_xtimestamp_decoding 12
>>>     ok 1 - inode_test_xtimestamp_decoding 13
>>>     ok 1 - inode_test_xtimestamp_decoding 14
>>>     ok 1 - inode_test_xtimestamp_decoding 15
>>>     ok 1 - inode_test_xtimestamp_decoding 16
>>> ok 1 - ext4_inode_test
>>> (followed by other kunit test outputs)
>>
>> Hmm, interesting. Let me play with your patch a bit.
>>
>> One option is to just have the test case number increment as well,
>> i.e. have this:
>> |    ok 1 - inode_test_xtimestamp_decoding#1
>> |    ok 2 - inode_test_xtimestamp_decoding#2
>> |    ok 3 - inode_test_xtimestamp_decoding#3
>> |    ok 4 - inode_test_xtimestamp_decoding#4
>> |    ok 5 - inode_test_xtimestamp_decoding#5
>> ...
>>
>> Or is there something else I missed?
> 
> Right, so TAP wants the exact number of tests it will run ahead of time.
> In which case we can still put the results of each parameterized test in
> a diagnostic. Please see my proposed patch below, which still does
> proper initialization/destruction of each parameter case as if it was
> its own test case.
> 
> With it the output looks as follows:
> 
> | TAP version 14
> | 1..6
> |     # Subtest: ext4_inode_test
> |     1..1
> |     # ok param#0 - inode_test_xtimestamp_decoding
> |     # ok param#1 - inode_test_xtimestamp_decoding
> |     # ok param#2 - inode_test_xtimestamp_decoding
> |     # ok param#3 - inode_test_xtimestamp_decoding
> |     # ok param#4 - inode_test_xtimestamp_decoding
> |     # ok param#5 - inode_test_xtimestamp_decoding
> |     # ok param#6 - inode_test_xtimestamp_decoding
> |     # ok param#7 - inode_test_xtimestamp_decoding
> |     # ok param#8 - inode_test_xtimestamp_decoding
> |     # ok param#9 - inode_test_xtimestamp_decoding
> |     # ok param#10 - inode_test_xtimestamp_decoding
> |     # ok param#11 - inode_test_xtimestamp_decoding
> |     # ok param#12 - inode_test_xtimestamp_decoding
> |     # ok param#13 - inode_test_xtimestamp_decoding
> |     # ok param#14 - inode_test_xtimestamp_decoding
> |     # ok param#15 - inode_test_xtimestamp_decoding
> |     ok 1 - inode_test_xtimestamp_decoding
> | ok 1 - ext4_inode_test
> 
> Would that be reasonable? If so, feel free to take the patch and
> test/adjust as required.
> 
> I'm not sure on the best format -- is there is a recommended format for
> parameterized test result output? If not, I suppose we can put anything
> we like into the diagnostic.
> 

I think this format of output should be fine for parameterized tests.
But, this patch has the same issue as earlier. While, the tests run and
this is the output that can be seen using dmesg, it still causes an issue on
using the kunit tool. It gives a similar error:

[11:07:38] [ERROR]  expected 7 test suites, but got -1
[11:07:38] [ERROR] expected_suite_index -1, but got 2
[11:07:38] [ERROR] got unexpected test suite: kunit-try-catch-test
[ERROR] no tests run!
[11:07:38] ============================================================
[11:07:38] Testing complete. 0 tests run. 0 failed. 0 crashed.

Thanks!

> Thanks,
> -- Marco
> 
> ------ >8 ------
> 
> From ccbf4e2e190a2d7c6a94a51c9b1fb3b9a940e578 Mon Sep 17 00:00:00 2001
> From: Arpitha Raghunandan <[e-mail]>
> Date: Tue, 27 Oct 2020 23:16:30 +0530
> Subject: [PATCH] kunit: Support for Parameterized Testing
> 
> 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 <[e-mail]>
> Co-developed-by: Marco Elver <[e-mail]>
> Signed-off-by: Marco Elver <[e-mail]>
> ---
> 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 9197da792336..ae5488a37e48 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -107,6 +107,7 @@ struct kunit;
>   *
>   * @run_case: the function representing the actual test case.
>   * @name:     the name of the test case.
> + * @generate_params: the generator function for parameterized tests.
>   *
>   * A test case is a function with the signature,
>   * ``void (*)(struct kunit *)``
> @@ -141,6 +142,7 @@ struct kunit;
>  struct kunit_case {
>  	void (*run_case)(struct kunit *test);
>  	const char *name;
> +	const void* (*generate_params)(const void *prev);
>  
>  	/* private: internal use only. */
>  	bool success;
> @@ -163,6 +165,22 @@ static inline char *kunit_status_to_string(bool status)
>   */
>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
>  
> +/**
> + * KUNIT_CASE_PARAM - A helper for creation a parameterized &struct kunit_case
> + *
> + * @test_name: a reference to a test case function.
> + * @gen_params: a reference to a parameter generator function.
> + *
> + * The generator function ``const void* gen_params(const void *prev)`` is used
> + * to lazily generate a series of arbitrarily typed values that fit into a
> + * void*. The argument @prev is the previously returned value, which should be
> + * used to derive the next value; @prev is set to NULL on the initial generator
> + * call.  When no more values are available, the generator must return NULL.
> + */
> +#define KUNIT_CASE_PARAM(test_name, gen_params)			\
> +		{ .run_case = test_name, .name = #test_name,	\
> +		  .generate_params = gen_params }
> +
>  /**
>   * struct kunit_suite - describes a related collection of &struct kunit_case
>   *
> @@ -208,6 +226,10 @@ struct kunit {
>  	const char *name; /* Read only after initialization! */
>  	char *log; /* Points at case log after initialization */
>  	struct kunit_try_catch try_catch;
> +	/* param_value is the current parameter value for a test case. */
> +	const void *param_value;
> +	/* param_index stores the index of the parameter in parameterized tests. */
> +	int param_index;
>  	/*
>  	 * success starts as true, and may only be set to false during a
>  	 * test case; thus, it is safe to update this across multiple
> @@ -1742,4 +1764,18 @@ do {									       \
>  						fmt,			       \
>  						##__VA_ARGS__)
>  
> +/**
> + * KUNIT_ARRAY_PARAM() - Define test parameter generator from an array.
> + * @name:  prefix for the test parameter generator function.
> + * @array: array of test parameters.
> + *
> + * Define function @name_gen_params which uses @array to generate parameters.
> + */
> +#define KUNIT_ARRAY_PARAM(name, array)								\
> +	static const void *name##_gen_params(const void *prev)					\
> +	{											\
> +		typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);	\
> +		return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;			\
> +	}
> +
>  #endif /* _KUNIT_TEST_H */
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 750704abe89a..453ebe4da77d 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -325,29 +325,25 @@ static void kunit_catch_run_case(void *data)
>   * occur in a test case and reports them as failures.
>   */
>  static void kunit_run_case_catch_errors(struct kunit_suite *suite,
> -					struct kunit_case *test_case)
> +					struct kunit_case *test_case,
> +					struct kunit *test)
>  {
>  	struct kunit_try_catch_context context;
>  	struct kunit_try_catch *try_catch;
> -	struct kunit test;
>  
> -	kunit_init_test(&test, test_case->name, test_case->log);
> -	try_catch = &test.try_catch;
> +	kunit_init_test(test, test_case->name, test_case->log);
> +	try_catch = &test->try_catch;
>  
>  	kunit_try_catch_init(try_catch,
> -			     &test,
> +			     test,
>  			     kunit_try_run_case,
>  			     kunit_catch_run_case);
> -	context.test = &test;
> +	context.test = test;
>  	context.suite = suite;
>  	context.test_case = test_case;
>  	kunit_try_catch_run(try_catch, &context);
>  
> -	test_case->success = test.success;
> -
> -	kunit_print_ok_not_ok(&test, true, test_case->success,
> -			      kunit_test_case_num(suite, test_case),
> -			      test_case->name);
> +	test_case->success = test->success;
>  }
>  
>  int kunit_run_tests(struct kunit_suite *suite)
> @@ -356,8 +352,32 @@ int kunit_run_tests(struct kunit_suite *suite)
>  
>  	kunit_print_subtest_start(suite);
>  
> -	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",
> +					  kunit_status_to_string(test.success),
> +					  test.param_index, test_case->name);
> +				test.param_value = test_case->generate_params(test.param_value);
> +				test.param_index++;
> +			}
> +		} while (test.param_value);
> +
> +		kunit_print_ok_not_ok(&test, true, test_success,
> +				      kunit_test_case_num(suite, test_case),
> +				      test_case->name);
> +	}
>  
>  	kunit_print_subtest_end(suite);
>  
>

Marco Elver Nov. 6, 2020, 8:11 a.m. UTC | #9

On Fri, 6 Nov 2020 at 06:54, Arpitha Raghunandan <[e-mail]> wrote:
>
> On 06/11/20 1:25 am, Marco Elver wrote:
> > On Thu, Nov 05, 2020 at 04:02PM +0100, Marco Elver wrote:
> >> On Thu, 5 Nov 2020 at 15:30, Arpitha Raghunandan <[e-mail]> wrote:
> > [...]
> >>>>> I tried adding support to run each parameter as a distinct test case by
> >>>>> making changes to kunit_run_case_catch_errors(). The issue here is that
> >>>>> since the results are displayed in KTAP format, this change will result in
> >>>>> each parameter being considered a subtest of another subtest (test case
> >>>>> in KUnit).
> >>>>
> >>>> Do you have example output? That might help understand what's going on.
> >>>>
> >>>
> >>> The change that I tried can be seen here (based on the v4 patch):
> >>> https://gist.github.com/arpi-r/4822899087ca4cc34572ed9e45cc5fee.
> >>>
> >>> Using the kunit tool, I get this error:
> >>>
> >>> [19:20:41] [ERROR]  expected 7 test suites, but got -1
> >>> [ERROR] no tests run!
> >>> [19:20:41] ============================================================
> >>> [19:20:41] Testing complete. 0 tests run. 0 failed. 0 crashed.
> >>>
> >>> But this error is only because of how the tool displays the results.
> >>> The test actually does run, as can be seen in the dmesg output:
> >>>
> >>> TAP version 14
> >>> 1..7
> >>>     # Subtest: ext4_inode_test
> >>>     1..1
> >>>     ok 1 - inode_test_xtimestamp_decoding 1
> >>>     ok 1 - inode_test_xtimestamp_decoding 2
> >>>     ok 1 - inode_test_xtimestamp_decoding 3
> >>>     ok 1 - inode_test_xtimestamp_decoding 4
> >>>     ok 1 - inode_test_xtimestamp_decoding 5
> >>>     ok 1 - inode_test_xtimestamp_decoding 6
> >>>     ok 1 - inode_test_xtimestamp_decoding 7
> >>>     ok 1 - inode_test_xtimestamp_decoding 8
> >>>     ok 1 - inode_test_xtimestamp_decoding 9
> >>>     ok 1 - inode_test_xtimestamp_decoding 10
> >>>     ok 1 - inode_test_xtimestamp_decoding 11
> >>>     ok 1 - inode_test_xtimestamp_decoding 12
> >>>     ok 1 - inode_test_xtimestamp_decoding 13
> >>>     ok 1 - inode_test_xtimestamp_decoding 14
> >>>     ok 1 - inode_test_xtimestamp_decoding 15
> >>>     ok 1 - inode_test_xtimestamp_decoding 16
> >>> ok 1 - ext4_inode_test
> >>> (followed by other kunit test outputs)
> >>
> >> Hmm, interesting. Let me play with your patch a bit.
> >>
> >> One option is to just have the test case number increment as well,
> >> i.e. have this:
> >> |    ok 1 - inode_test_xtimestamp_decoding#1
> >> |    ok 2 - inode_test_xtimestamp_decoding#2
> >> |    ok 3 - inode_test_xtimestamp_decoding#3
> >> |    ok 4 - inode_test_xtimestamp_decoding#4
> >> |    ok 5 - inode_test_xtimestamp_decoding#5
> >> ...
> >>
> >> Or is there something else I missed?
> >
> > Right, so TAP wants the exact number of tests it will run ahead of time.
> > In which case we can still put the results of each parameterized test in
> > a diagnostic. Please see my proposed patch below, which still does
> > proper initialization/destruction of each parameter case as if it was
> > its own test case.
> >
> > With it the output looks as follows:
> >
> > | TAP version 14
> > | 1..6
> > |     # Subtest: ext4_inode_test
> > |     1..1
> > |     # ok param#0 - inode_test_xtimestamp_decoding
> > |     # ok param#1 - inode_test_xtimestamp_decoding
> > |     # ok param#2 - inode_test_xtimestamp_decoding
> > |     # ok param#3 - inode_test_xtimestamp_decoding
> > |     # ok param#4 - inode_test_xtimestamp_decoding
> > |     # ok param#5 - inode_test_xtimestamp_decoding
> > |     # ok param#6 - inode_test_xtimestamp_decoding
> > |     # ok param#7 - inode_test_xtimestamp_decoding
> > |     # ok param#8 - inode_test_xtimestamp_decoding
> > |     # ok param#9 - inode_test_xtimestamp_decoding
> > |     # ok param#10 - inode_test_xtimestamp_decoding
> > |     # ok param#11 - inode_test_xtimestamp_decoding
> > |     # ok param#12 - inode_test_xtimestamp_decoding
> > |     # ok param#13 - inode_test_xtimestamp_decoding
> > |     # ok param#14 - inode_test_xtimestamp_decoding
> > |     # ok param#15 - inode_test_xtimestamp_decoding
> > |     ok 1 - inode_test_xtimestamp_decoding
> > | ok 1 - ext4_inode_test
> >
> > Would that be reasonable? If so, feel free to take the patch and
> > test/adjust as required.
> >
> > I'm not sure on the best format -- is there is a recommended format for
> > parameterized test result output? If not, I suppose we can put anything
> > we like into the diagnostic.
> >
>
> I think this format of output should be fine for parameterized tests.
> But, this patch has the same issue as earlier. While, the tests run and
> this is the output that can be seen using dmesg, it still causes an issue on
> using the kunit tool. It gives a similar error:
>
> [11:07:38] [ERROR]  expected 7 test suites, but got -1
> [11:07:38] [ERROR] expected_suite_index -1, but got 2
> [11:07:38] [ERROR] got unexpected test suite: kunit-try-catch-test
> [ERROR] no tests run!
> [11:07:38] ============================================================
> [11:07:38] Testing complete. 0 tests run. 0 failed. 0 crashed.
>

I'd suggest testing without these patches and diffing the output.
AFAIK we're not adding any new non-# output, so it might be a
pre-existing bug in some parsing code. Either that, or the parsing
code does not respect the # correctly?

Thanks,
-- Marco

Marco Elver Nov. 6, 2020, 12:34 p.m. UTC | #10

On Fri, Nov 06, 2020 at 09:11AM +0100, Marco Elver wrote:
> On Fri, 6 Nov 2020 at 06:54, Arpitha Raghunandan <[e-mail]> wrote:
[...]
> > I think this format of output should be fine for parameterized tests.
> > But, this patch has the same issue as earlier. While, the tests run and
> > this is the output that can be seen using dmesg, it still causes an issue on
> > using the kunit tool. It gives a similar error:
> >
> > [11:07:38] [ERROR]  expected 7 test suites, but got -1
> > [11:07:38] [ERROR] expected_suite_index -1, but got 2
> > [11:07:38] [ERROR] got unexpected test suite: kunit-try-catch-test
> > [ERROR] no tests run!
> > [11:07:38] ============================================================
> > [11:07:38] Testing complete. 0 tests run. 0 failed. 0 crashed.
> >
> 
> I'd suggest testing without these patches and diffing the output.
> AFAIK we're not adding any new non-# output, so it might be a
> pre-existing bug in some parsing code. Either that, or the parsing
> code does not respect the # correctly?

Hmm, tools/testing/kunit/kunit_parser.py has

	SUBTEST_DIAGNOSTIC = re.compile(r'^[\s]+# .*?: (.*)$')

, which seems to expect a ': ' in there. Not sure if this is required by
TAP, but let's leave this alone for now.

We can change the output to not trip this up, which might also be more a
consistent diagnostic format vs. other diagnostics. See the revised
patch below. With it the output is as follows:

| TAP version 14
| 1..6
|     # Subtest: ext4_inode_test
|     1..1
|     # inode_test_xtimestamp_decoding: param-0 ok
|     # inode_test_xtimestamp_decoding: param-1 ok
|     # inode_test_xtimestamp_decoding: param-2 ok
|     # inode_test_xtimestamp_decoding: param-3 ok
|     # inode_test_xtimestamp_decoding: param-4 ok
|     # inode_test_xtimestamp_decoding: param-5 ok
|     # inode_test_xtimestamp_decoding: param-6 ok
|     # inode_test_xtimestamp_decoding: param-7 ok
|     # inode_test_xtimestamp_decoding: param-8 ok
|     # inode_test_xtimestamp_decoding: param-9 ok
|     # inode_test_xtimestamp_decoding: param-10 ok
|     # inode_test_xtimestamp_decoding: param-11 ok
|     # inode_test_xtimestamp_decoding: param-12 ok
|     # inode_test_xtimestamp_decoding: param-13 ok
|     # inode_test_xtimestamp_decoding: param-14 ok
|     # inode_test_xtimestamp_decoding: param-15 ok
|     ok 1 - inode_test_xtimestamp_decoding
| ok 1 - ext4_inode_test

And kunit-tool seems to be happy as well.

Thanks,
-- Marco

------ >8 ------

From 13a94d75d6b1b430e89fcff2cd76629b56b9d636 Mon Sep 17 00:00:00 2001
From: Arpitha Raghunandan <[e-mail]>
Date: Tue, 27 Oct 2020 23:16:30 +0530
Subject: [PATCH] kunit: Support for Parameterized Testing

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 <[e-mail]>
Co-developed-by: Marco Elver <[e-mail]>
Signed-off-by: Marco Elver <[e-mail]>
---
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 9197da792336..ae5488a37e48 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -107,6 +107,7 @@ struct kunit;
  *
  * @run_case: the function representing the actual test case.
  * @name:     the name of the test case.
+ * @generate_params: the generator function for parameterized tests.
  *
  * A test case is a function with the signature,
  * ``void (*)(struct kunit *)``
@@ -141,6 +142,7 @@ struct kunit;
 struct kunit_case {
 	void (*run_case)(struct kunit *test);
 	const char *name;
+	const void* (*generate_params)(const void *prev);
 
 	/* private: internal use only. */
 	bool success;
@@ -163,6 +165,22 @@ static inline char *kunit_status_to_string(bool status)
  */
 #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
 
+/**
+ * KUNIT_CASE_PARAM - A helper for creation a parameterized &struct kunit_case
+ *
+ * @test_name: a reference to a test case function.
+ * @gen_params: a reference to a parameter generator function.
+ *
+ * The generator function ``const void* gen_params(const void *prev)`` is used
+ * to lazily generate a series of arbitrarily typed values that fit into a
+ * void*. The argument @prev is the previously returned value, which should be
+ * used to derive the next value; @prev is set to NULL on the initial generator
+ * call.  When no more values are available, the generator must return NULL.
+ */
+#define KUNIT_CASE_PARAM(test_name, gen_params)			\
+		{ .run_case = test_name, .name = #test_name,	\
+		  .generate_params = gen_params }
+
 /**
  * struct kunit_suite - describes a related collection of &struct kunit_case
  *
@@ -208,6 +226,10 @@ struct kunit {
 	const char *name; /* Read only after initialization! */
 	char *log; /* Points at case log after initialization */
 	struct kunit_try_catch try_catch;
+	/* param_value is the current parameter value for a test case. */
+	const void *param_value;
+	/* param_index stores the index of the parameter in parameterized tests. */
+	int param_index;
 	/*
 	 * success starts as true, and may only be set to false during a
 	 * test case; thus, it is safe to update this across multiple
@@ -1742,4 +1764,18 @@ do {									       \
 						fmt,			       \
 						##__VA_ARGS__)
 
+/**
+ * KUNIT_ARRAY_PARAM() - Define test parameter generator from an array.
+ * @name:  prefix for the test parameter generator function.
+ * @array: array of test parameters.
+ *
+ * Define function @name_gen_params which uses @array to generate parameters.
+ */
+#define KUNIT_ARRAY_PARAM(name, array)								\
+	static const void *name##_gen_params(const void *prev)					\
+	{											\
+		typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);	\
+		return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;			\
+	}
+
 #endif /* _KUNIT_TEST_H */
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 750704abe89a..329fee9e0634 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -325,29 +325,25 @@ static void kunit_catch_run_case(void *data)
  * occur in a test case and reports them as failures.
  */
 static void kunit_run_case_catch_errors(struct kunit_suite *suite,
-					struct kunit_case *test_case)
+					struct kunit_case *test_case,
+					struct kunit *test)
 {
 	struct kunit_try_catch_context context;
 	struct kunit_try_catch *try_catch;
-	struct kunit test;
 
-	kunit_init_test(&test, test_case->name, test_case->log);
-	try_catch = &test.try_catch;
+	kunit_init_test(test, test_case->name, test_case->log);
+	try_catch = &test->try_catch;
 
 	kunit_try_catch_init(try_catch,
-			     &test,
+			     test,
 			     kunit_try_run_case,
 			     kunit_catch_run_case);
-	context.test = &test;
+	context.test = test;
 	context.suite = suite;
 	context.test_case = test_case;
 	kunit_try_catch_run(try_catch, &context);
 
-	test_case->success = test.success;
-
-	kunit_print_ok_not_ok(&test, true, test_case->success,
-			      kunit_test_case_num(suite, test_case),
-			      test_case->name);
+	test_case->success = test->success;
 }
 
 int kunit_run_tests(struct kunit_suite *suite)
@@ -356,8 +352,32 @@ int kunit_run_tests(struct kunit_suite *suite)
 
 	kunit_print_subtest_start(suite);
 
-	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",
+					  test_case->name, test.param_index,
+					  kunit_status_to_string(test.success));
+				test.param_value = test_case->generate_params(test.param_value);
+				test.param_index++;
+			}
+		} while (test.param_value);
+
+		kunit_print_ok_not_ok(&test, true, test_success,
+				      kunit_test_case_num(suite, test_case),
+				      test_case->name);
+	}
 
 	kunit_print_subtest_end(suite);

Arpitha Raghunandan Nov. 6, 2020, 4:16 p.m. UTC | #11

On 06/11/20 6:04 pm, Marco Elver wrote:
> On Fri, Nov 06, 2020 at 09:11AM +0100, Marco Elver wrote:
>> On Fri, 6 Nov 2020 at 06:54, Arpitha Raghunandan <[e-mail]> wrote:
> [...]
>>> I think this format of output should be fine for parameterized tests.
>>> But, this patch has the same issue as earlier. While, the tests run and
>>> this is the output that can be seen using dmesg, it still causes an issue on
>>> using the kunit tool. It gives a similar error:
>>>
>>> [11:07:38] [ERROR]  expected 7 test suites, but got -1
>>> [11:07:38] [ERROR] expected_suite_index -1, but got 2
>>> [11:07:38] [ERROR] got unexpected test suite: kunit-try-catch-test
>>> [ERROR] no tests run!
>>> [11:07:38] ============================================================
>>> [11:07:38] Testing complete. 0 tests run. 0 failed. 0 crashed.
>>>
>>
>> I'd suggest testing without these patches and diffing the output.
>> AFAIK we're not adding any new non-# output, so it might be a
>> pre-existing bug in some parsing code. Either that, or the parsing
>> code does not respect the # correctly?
> 
> Hmm, tools/testing/kunit/kunit_parser.py has
> 
> 	SUBTEST_DIAGNOSTIC = re.compile(r'^[\s]+# .*?: (.*)$')
> 
> , which seems to expect a ': ' in there. Not sure if this is required by
> TAP, but let's leave this alone for now.
> 

Oh okay.

> We can change the output to not trip this up, which might also be more a
> consistent diagnostic format vs. other diagnostics. See the revised
> patch below. With it the output is as follows:
> 
> | TAP version 14
> | 1..6
> |     # Subtest: ext4_inode_test
> |     1..1
> |     # inode_test_xtimestamp_decoding: param-0 ok
> |     # inode_test_xtimestamp_decoding: param-1 ok
> |     # inode_test_xtimestamp_decoding: param-2 ok
> |     # inode_test_xtimestamp_decoding: param-3 ok
> |     # inode_test_xtimestamp_decoding: param-4 ok
> |     # inode_test_xtimestamp_decoding: param-5 ok
> |     # inode_test_xtimestamp_decoding: param-6 ok
> |     # inode_test_xtimestamp_decoding: param-7 ok
> |     # inode_test_xtimestamp_decoding: param-8 ok
> |     # inode_test_xtimestamp_decoding: param-9 ok
> |     # inode_test_xtimestamp_decoding: param-10 ok
> |     # inode_test_xtimestamp_decoding: param-11 ok
> |     # inode_test_xtimestamp_decoding: param-12 ok
> |     # inode_test_xtimestamp_decoding: param-13 ok
> |     # inode_test_xtimestamp_decoding: param-14 ok
> |     # inode_test_xtimestamp_decoding: param-15 ok
> |     ok 1 - inode_test_xtimestamp_decoding
> | ok 1 - ext4_inode_test
> 
> And kunit-tool seems to be happy as well.
> 

Yes this works as expected. Thank you!
I will send this patch as v5.

> Thanks,
> -- Marco
> 
> ------ >8 ------
> 
> From 13a94d75d6b1b430e89fcff2cd76629b56b9d636 Mon Sep 17 00:00:00 2001
> From: Arpitha Raghunandan <[e-mail]>
> Date: Tue, 27 Oct 2020 23:16:30 +0530
> Subject: [PATCH] kunit: Support for Parameterized Testing
> 
> 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 <[e-mail]>
> Co-developed-by: Marco Elver <[e-mail]>
> Signed-off-by: Marco Elver <[e-mail]>
> ---
> 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 9197da792336..ae5488a37e48 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -107,6 +107,7 @@ struct kunit;
>   *
>   * @run_case: the function representing the actual test case.
>   * @name:     the name of the test case.
> + * @generate_params: the generator function for parameterized tests.
>   *
>   * A test case is a function with the signature,
>   * ``void (*)(struct kunit *)``
> @@ -141,6 +142,7 @@ struct kunit;
>  struct kunit_case {
>  	void (*run_case)(struct kunit *test);
>  	const char *name;
> +	const void* (*generate_params)(const void *prev);
>  
>  	/* private: internal use only. */
>  	bool success;
> @@ -163,6 +165,22 @@ static inline char *kunit_status_to_string(bool status)
>   */
>  #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
>  
> +/**
> + * KUNIT_CASE_PARAM - A helper for creation a parameterized &struct kunit_case
> + *
> + * @test_name: a reference to a test case function.
> + * @gen_params: a reference to a parameter generator function.
> + *
> + * The generator function ``const void* gen_params(const void *prev)`` is used
> + * to lazily generate a series of arbitrarily typed values that fit into a
> + * void*. The argument @prev is the previously returned value, which should be
> + * used to derive the next value; @prev is set to NULL on the initial generator
> + * call.  When no more values are available, the generator must return NULL.
> + */
> +#define KUNIT_CASE_PARAM(test_name, gen_params)			\
> +		{ .run_case = test_name, .name = #test_name,	\
> +		  .generate_params = gen_params }
> +
>  /**
>   * struct kunit_suite - describes a related collection of &struct kunit_case
>   *
> @@ -208,6 +226,10 @@ struct kunit {
>  	const char *name; /* Read only after initialization! */
>  	char *log; /* Points at case log after initialization */
>  	struct kunit_try_catch try_catch;
> +	/* param_value is the current parameter value for a test case. */
> +	const void *param_value;
> +	/* param_index stores the index of the parameter in parameterized tests. */
> +	int param_index;
>  	/*
>  	 * success starts as true, and may only be set to false during a
>  	 * test case; thus, it is safe to update this across multiple
> @@ -1742,4 +1764,18 @@ do {									       \
>  						fmt,			       \
>  						##__VA_ARGS__)
>  
> +/**
> + * KUNIT_ARRAY_PARAM() - Define test parameter generator from an array.
> + * @name:  prefix for the test parameter generator function.
> + * @array: array of test parameters.
> + *
> + * Define function @name_gen_params which uses @array to generate parameters.
> + */
> +#define KUNIT_ARRAY_PARAM(name, array)								\
> +	static const void *name##_gen_params(const void *prev)					\
> +	{											\
> +		typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);	\
> +		return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;			\
> +	}
> +
>  #endif /* _KUNIT_TEST_H */
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 750704abe89a..329fee9e0634 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -325,29 +325,25 @@ static void kunit_catch_run_case(void *data)
>   * occur in a test case and reports them as failures.
>   */
>  static void kunit_run_case_catch_errors(struct kunit_suite *suite,
> -					struct kunit_case *test_case)
> +					struct kunit_case *test_case,
> +					struct kunit *test)
>  {
>  	struct kunit_try_catch_context context;
>  	struct kunit_try_catch *try_catch;
> -	struct kunit test;
>  
> -	kunit_init_test(&test, test_case->name, test_case->log);
> -	try_catch = &test.try_catch;
> +	kunit_init_test(test, test_case->name, test_case->log);
> +	try_catch = &test->try_catch;
>  
>  	kunit_try_catch_init(try_catch,
> -			     &test,
> +			     test,
>  			     kunit_try_run_case,
>  			     kunit_catch_run_case);
> -	context.test = &test;
> +	context.test = test;
>  	context.suite = suite;
>  	context.test_case = test_case;
>  	kunit_try_catch_run(try_catch, &context);
>  
> -	test_case->success = test.success;
> -
> -	kunit_print_ok_not_ok(&test, true, test_case->success,
> -			      kunit_test_case_num(suite, test_case),
> -			      test_case->name);
> +	test_case->success = test->success;
>  }
>  
>  int kunit_run_tests(struct kunit_suite *suite)
> @@ -356,8 +352,32 @@ int kunit_run_tests(struct kunit_suite *suite)
>  
>  	kunit_print_subtest_start(suite);
>  
> -	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",
> +					  test_case->name, test.param_index,
> +					  kunit_status_to_string(test.success));
> +				test.param_value = test_case->generate_params(test.param_value);
> +				test.param_index++;
> +			}
> +		} while (test.param_value);
> +
> +		kunit_print_ok_not_ok(&test, true, test_success,
> +				      kunit_test_case_num(suite, test_case),
> +				      test_case->name);
> +	}
>  
>  	kunit_print_subtest_end(suite);
>  
>

Patch

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 9197da792336..ec2307ee9bb0 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -107,6 +107,13 @@  struct kunit;
  *
  * @run_case: the function representing the actual test case.
  * @name:     the name of the test case.
+ * @generate_params: the generator function for parameterized tests.
+ *
+ * The generator function is used to lazily generate a series of
+ * arbitrarily typed values that fit into a void*. The argument @prev
+ * is the previously returned value, which should be used to derive the
+ * next value; @prev is set to NULL on the initial generator call.
+ * When no more values are available, the generator must return NULL.
  *
  * A test case is a function with the signature,
  * ``void (*)(struct kunit *)``
@@ -141,6 +148,7 @@  struct kunit;
 struct kunit_case {
 	void (*run_case)(struct kunit *test);
 	const char *name;
+	void* (*generate_params)(void *prev);
 
 	/* private: internal use only. */
 	bool success;
@@ -162,6 +170,9 @@  static inline char *kunit_status_to_string(bool status)
  * &struct kunit_case for an example on how to use it.
  */
 #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }
+#define KUNIT_CASE_PARAM(test_name, gen_params)			\
+		{ .run_case = test_name, .name = #test_name,	\
+		  .generate_params = gen_params }
 
 /**
  * struct kunit_suite - describes a related collection of &struct kunit_case
@@ -208,6 +219,15 @@  struct kunit {
 	const char *name; /* Read only after initialization! */
 	char *log; /* Points at case log after initialization */
 	struct kunit_try_catch try_catch;
+	/* param_value points to test case parameters in parameterized tests */
+	void *param_value;
+	/*
+	 * param_index stores the index of the parameter in
+	 * parameterized tests. param_index + 1 is printed
+	 * to indicate the parameter that causes the test
+	 * to fail in case of test failure.
+	 */
+	int param_index;
 	/*
 	 * success starts as true, and may only be set to false during a
 	 * test case; thus, it is safe to update this across multiple
@@ -1742,4 +1762,18 @@  do {									       \
 						fmt,			       \
 						##__VA_ARGS__)
 
+/**
+ * KUNIT_ARRAY_PARAM() - Helper method for test parameter generators
+ * 			 required in parameterized tests.
+ * @name:  prefix of the name for the test parameter generator function.
+ *	   It will be suffixed by "_gen_params".
+ * @array: a user-supplied pointer to an array of test parameters.
+ */
+#define KUNIT_ARRAY_PARAM(name, array)								\
+	static void *name##_gen_params(void *prev)						\
+	{											\
+		typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array);	\
+		return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL;			\
+	}
+
 #endif /* _KUNIT_TEST_H */
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 750704abe89a..8ad908b61494 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -127,6 +127,12 @@  unsigned int kunit_test_case_num(struct kunit_suite *suite,
 }
 EXPORT_SYMBOL_GPL(kunit_test_case_num);
 
+static void kunit_print_failed_param(struct kunit *test)
+{
+	kunit_err(test, "\n\tTest failed at:\n\ttest case: %s\n\tparameter: %d\n",
+						test->name, test->param_index + 1);
+}
+
 static void kunit_print_string_stream(struct kunit *test,
 				      struct string_stream *stream)
 {
@@ -168,6 +174,8 @@  static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
 	assert->format(assert, stream);
 
 	kunit_print_string_stream(test, stream);
+	if (test->param_value)
+		kunit_print_failed_param(test);
 
 	WARN_ON(string_stream_destroy(stream));
 }
@@ -239,7 +247,18 @@  static void kunit_run_case_internal(struct kunit *test,
 		}
 	}
 
-	test_case->run_case(test);
+	if (!test_case->generate_params) {
+		test_case->run_case(test);
+	} else {
+		test->param_value = test_case->generate_params(NULL);
+		test->param_index = 0;
+
+		while (test->param_value) {
+			test_case->run_case(test);
+			test->param_value = test_case->generate_params(test->param_value);
+			test->param_index++;
+		}
+	}
 }
 
 static void kunit_case_internal_cleanup(struct kunit *test)