diff mbox series

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

Message ID 20201026183523.82749-1-98.arpi@gmail.com
State New
Headers show
Series [v3,1/2] kunit: Support for Parameterized Testing | expand

Commit Message

Arpitha Raghunandan Oct. 26, 2020, 6:35 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 <98.arpi@gmail.com>
Co-developed-by: Marco Elver <elver@google.com>
Signed-off-by: Marco Elver <elver@google.com>
---
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 | 32 ++++++++++++++++++++++++++++++++
 lib/kunit/test.c     | 20 +++++++++++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

Comments

Marco Elver Oct. 26, 2020, 11:14 p.m. UTC | #1
On Mon, 26 Oct 2020 at 19:36, Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>

> Implementation of support for parameterized testing in KUnit.

> This approach requires the creation of a test case using the

> KUNIT_CASE_PARAM macro that accepts a generator function as input.

> This generator function should return the next parameter given the

> previous parameter in parameterized tests. It also provides

> a macro to generate common-case generators.

>

> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>

> Co-developed-by: Marco Elver <elver@google.com>

> Signed-off-by: Marco Elver <elver@google.com>

> ---

> Changes v2->v3:

> - Modifictaion of generator macro and method


Great to see it worked as expected!

> Changes v1->v2:

> - Use of a generator method to access test case parameters

>

>  include/kunit/test.h | 32 ++++++++++++++++++++++++++++++++

>  lib/kunit/test.c     | 20 +++++++++++++++++++-

>  2 files changed, 51 insertions(+), 1 deletion(-)

>

> diff --git a/include/kunit/test.h b/include/kunit/test.h

> index a423fffefea0..16bf9f334e2c 100644

> --- a/include/kunit/test.h

> +++ b/include/kunit/test.h

> @@ -142,6 +142,12 @@ struct kunit_case {

>         void (*run_case)(struct kunit *test);

>         const char *name;

>

> +       /*

> +        * Pointer to test parameter generator function.

> +        * Used only for parameterized tests.


What I meant was to give a description of the protocol, so that if
somebody wanted, they could (without reading the implementation)
implement their own custom generator without the helper macro.

E.g. something like: "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."

> +        */

> +       void* (*generate_params)(void *prev);

> +

>         /* private: internal use only. */

>         bool success;

>         char *log;

> @@ -162,6 +168,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 +217,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_values points to test case parameters in parameterized tests */

> +       void *param_values;

> +       /*

> +        * current_param stores the index of the parameter in

> +        * the array of parameters in parameterized tests.

> +        * current_param + 1 is printed to indicate the parameter

> +        * that causes the test to fail in case of test failure.

> +        */

> +       int current_param;

>         /*

>          * 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 +1760,18 @@ do {                                                                            \

>                                                 fmt,                           \

>                                                 ##__VA_ARGS__)

>

> +/**

> + * KUNIT_PARAM_GENERATOR() - Helper method for test parameter generators

> + *                          required in parameterized tests.


This is only for arrays, which is why I suggested KUNIT_ARRAY_PARAM()
as the name.

A generator can very well be implemented without an array, so this
macro name is confusing. In future somebody might want to provide a
macro that takes a start + end value (and maybe a step value) to
generate a series of values. That generator could be named
KUNIT_RANGE_PARAM(name, start, end, step) and gives us a generator
that is also named name##_gen_params. (If you want to try implementing
that macro, I'd suggest doing it as a separate patch.)

And I don't think we need to put "GENERATOR" into the name of these
macros, because the generators are now the fundamental method with
which to get parameterized tests. We don't need to state the obvious,
in favor of some brevity.

> + * @name:  prefix of the name for the test parameter generator function.

> + * @prev: a pointer to the previous test parameter, NULL for first parameter.

> + * @array: a user-supplied pointer to an array of test parameters.

> + */

> +#define KUNIT_PARAM_GENERATOR(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 */


Thanks,
-- Marco
Arpitha Raghunandan Oct. 27, 2020, 5:14 a.m. UTC | #2
On 27/10/20 4:44 am, Marco Elver wrote:
> On Mon, 26 Oct 2020 at 19:36, Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>>
>> Implementation of support for parameterized testing in KUnit.
>> This approach requires the creation of a test case using the
>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
>> This generator function should return the next parameter given the
>> previous parameter in parameterized tests. It also provides
>> a macro to generate common-case generators.
>>
>> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
>> Co-developed-by: Marco Elver <elver@google.com>
>> Signed-off-by: Marco Elver <elver@google.com>
>> ---
>> Changes v2->v3:
>> - Modifictaion of generator macro and method
> 
> Great to see it worked as expected!
> 
>> Changes v1->v2:
>> - Use of a generator method to access test case parameters
>>
>>  include/kunit/test.h | 32 ++++++++++++++++++++++++++++++++
>>  lib/kunit/test.c     | 20 +++++++++++++++++++-
>>  2 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index a423fffefea0..16bf9f334e2c 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -142,6 +142,12 @@ struct kunit_case {
>>         void (*run_case)(struct kunit *test);
>>         const char *name;
>>
>> +       /*
>> +        * Pointer to test parameter generator function.
>> +        * Used only for parameterized tests.
> 
> What I meant was to give a description of the protocol, so that if
> somebody wanted, they could (without reading the implementation)
> implement their own custom generator without the helper macro.
> 
> E.g. something like: "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."
>

Oh okay. I am not sure if this is the best place to add documentation for this.
 
>> +        */
>> +       void* (*generate_params)(void *prev);
>> +
>>         /* private: internal use only. */
>>         bool success;
>>         char *log;
>> @@ -162,6 +168,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 +217,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_values points to test case parameters in parameterized tests */
>> +       void *param_values;
>> +       /*
>> +        * current_param stores the index of the parameter in
>> +        * the array of parameters in parameterized tests.
>> +        * current_param + 1 is printed to indicate the parameter
>> +        * that causes the test to fail in case of test failure.
>> +        */
>> +       int current_param;
>>         /*
>>          * 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 +1760,18 @@ do {                                                                            \
>>                                                 fmt,                           \
>>                                                 ##__VA_ARGS__)
>>
>> +/**
>> + * KUNIT_PARAM_GENERATOR() - Helper method for test parameter generators
>> + *                          required in parameterized tests.
> 
> This is only for arrays, which is why I suggested KUNIT_ARRAY_PARAM()
> as the name.
> 
> A generator can very well be implemented without an array, so this
> macro name is confusing. In future somebody might want to provide a
> macro that takes a start + end value (and maybe a step value) to
> generate a series of values. That generator could be named
> KUNIT_RANGE_PARAM(name, start, end, step) and gives us a generator
> that is also named name##_gen_params. (If you want to try implementing
> that macro, I'd suggest doing it as a separate patch.)
> 
> And I don't think we need to put "GENERATOR" into the name of these
> macros, because the generators are now the fundamental method with
> which to get parameterized tests. We don't need to state the obvious,
> in favor of some brevity.
>

Okay, makes sense. I will change it to KUNIT_ARRAY_PARAM() for the next version.
 
>> + * @name:  prefix of the name for the test parameter generator function.
>> + * @prev: a pointer to the previous test parameter, NULL for first parameter.
>> + * @array: a user-supplied pointer to an array of test parameters.
>> + */
>> +#define KUNIT_PARAM_GENERATOR(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 */
> 
> Thanks,
> -- Marco
> 

Thanks!
Marco Elver Oct. 27, 2020, 7:44 a.m. UTC | #3
On Tue, 27 Oct 2020 at 06:14, Arpitha Raghunandan <98.arpi@gmail.com> wrote:
[...]
> >> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >> index a423fffefea0..16bf9f334e2c 100644
> >> --- a/include/kunit/test.h
> >> +++ b/include/kunit/test.h
> >> @@ -142,6 +142,12 @@ struct kunit_case {
> >>         void (*run_case)(struct kunit *test);
> >>         const char *name;
> >>
> >> +       /*
> >> +        * Pointer to test parameter generator function.
> >> +        * Used only for parameterized tests.
> >
> > What I meant was to give a description of the protocol, so that if
> > somebody wanted, they could (without reading the implementation)
> > implement their own custom generator without the helper macro.
> >
> > E.g. something like: "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."
> >
>
> Oh okay. I am not sure if this is the best place to add documentation for this.

I think it doesn't hurt to add, but have a look at the comment above
this struct, which is already a kernel-doc comment. It probably makes
sense to move the comment there to describe the new variable.

Thanks,
-- Marco
Arpitha Raghunandan Oct. 27, 2020, 7:51 a.m. UTC | #4
On 27/10/20 1:14 pm, Marco Elver wrote:
> On Tue, 27 Oct 2020 at 06:14, Arpitha Raghunandan <98.arpi@gmail.com> wrote:
> [...]
>>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>>> index a423fffefea0..16bf9f334e2c 100644
>>>> --- a/include/kunit/test.h
>>>> +++ b/include/kunit/test.h
>>>> @@ -142,6 +142,12 @@ struct kunit_case {
>>>>         void (*run_case)(struct kunit *test);
>>>>         const char *name;
>>>>
>>>> +       /*
>>>> +        * Pointer to test parameter generator function.
>>>> +        * Used only for parameterized tests.
>>>
>>> What I meant was to give a description of the protocol, so that if
>>> somebody wanted, they could (without reading the implementation)
>>> implement their own custom generator without the helper macro.
>>>
>>> E.g. something like: "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."
>>>
>>
>> Oh okay. I am not sure if this is the best place to add documentation for this.
> 
> I think it doesn't hurt to add, but have a look at the comment above
> this struct, which is already a kernel-doc comment. It probably makes
> sense to move the comment there to describe the new variable.
>

Alright, I will move the comment there.
 
> Thanks,
> -- Marco
> 

Thanks.
Marco Elver Oct. 27, 2020, 7:55 a.m. UTC | #5
On Mon, 26 Oct 2020 at 19:36, Arpitha Raghunandan <98.arpi@gmail.com> wrote:
[...]
>          * 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 +1760,18 @@ do {                                                                            \
>                                                 fmt,                           \
>                                                 ##__VA_ARGS__)
>
> +/**
> + * KUNIT_PARAM_GENERATOR() - Helper method for test parameter generators
> + *                          required in parameterized tests.
> + * @name:  prefix of the name for the test parameter generator function.

This could mention that the generator function will be suffixed by
"_gen_params".

> + * @prev: a pointer to the previous test parameter, NULL for first parameter.
> + * @array: a user-supplied pointer to an array of test parameters.
> + */

I just noticed this: the interface of this macro does not include
"prev" (which is an argument of the generated function, but not
supplied to this macro; "prev" should hopefully be explained in the
other comment you're adding for the new struct field). So, the
kernel-doc comment here should only list the actual arguments of this
macro, which is only "name" and "array".

> +#define KUNIT_PARAM_GENERATOR(name, array)                                                     \
[...]

Thanks,
-- Marco
Marco Elver Oct. 27, 2020, 9:03 a.m. UTC | #6
I just tried to give this a spin on some of my tests and noticed some
more things (apologies for the multiple rounds of comments):

On Mon, 26 Oct 2020 at 19:36, Arpitha Raghunandan <98.arpi@gmail.com> wrote:
[...]
>  /**
>   * struct kunit_suite - describes a related collection of &struct kunit_case
> @@ -208,6 +217,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_values points to test case parameters in parameterized tests */
> +       void *param_values;

This should be singular, i.e. "param_value", since the generator only
generates 1 value for each test. Whether or not that value is a
pointer that points to more than 1 value or is an integer etc. is
entirely test-dependent.

> +       /*
> +        * current_param stores the index of the parameter in
> +        * the array of parameters in parameterized tests.
> +        * current_param + 1 is printed to indicate the parameter
> +        * that causes the test to fail in case of test failure.
> +        */
> +       int current_param;

I think, per your comment above, this should be named "param_index".
Also, I would suggest removing the mention of "array" in the comment,
because the parameters aren't dependent on use of an array.

>         /*
>          * 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 +1760,18 @@ do {                                                                            \
>                                                 fmt,                           \
>                                                 ##__VA_ARGS__)
>
> +/**
> + * KUNIT_PARAM_GENERATOR() - Helper method for test parameter generators
> + *                          required in parameterized tests.
> + * @name:  prefix of the name for the test parameter generator function.
> + * @prev: a pointer to the previous test parameter, NULL for first parameter.
> + * @array: a user-supplied pointer to an array of test parameters.
> + */
> +#define KUNIT_PARAM_GENERATOR(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..b70ab9b12f3b 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -127,6 +127,11 @@ 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 parameter: %d\n", test->current_param + 1);
> +}

Is this the only place where the param index is used? It might be
helpful to show the index together with the test-case name, otherwise
we get a series of test cases in the output which are all named the
same which can be confusing.

>  static void kunit_print_string_stream(struct kunit *test,
>                                       struct string_stream *stream)
>  {
> @@ -168,6 +173,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_values)
> +               kunit_print_failed_param(test);
>
>         WARN_ON(string_stream_destroy(stream));
>  }
> @@ -239,7 +246,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_values = test_case->generate_params(NULL);
> +               test->current_param = 0;
> +
> +               while (test->param_values) {
> +                       test_case->run_case(test);
> +                       test->param_values = test_case->generate_params(test->param_values);
> +                       test->current_param++;
> +               }
> +       }
>  }

Looking forward to v4. :-)

Thanks,
-- Marco
Arpitha Raghunandan Oct. 27, 2020, 2:39 p.m. UTC | #7
On 27/10/20 2:33 pm, Marco Elver wrote:
> I just tried to give this a spin on some of my tests and noticed some
> more things (apologies for the multiple rounds of comments):
> 
> On Mon, 26 Oct 2020 at 19:36, Arpitha Raghunandan <98.arpi@gmail.com> wrote:
> [...]
>>  /**
>>   * struct kunit_suite - describes a related collection of &struct kunit_case
>> @@ -208,6 +217,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_values points to test case parameters in parameterized tests */
>> +       void *param_values;
> 
> This should be singular, i.e. "param_value", since the generator only
> generates 1 value for each test. Whether or not that value is a
> pointer that points to more than 1 value or is an integer etc. is
> entirely test-dependent.
> 
>> +       /*
>> +        * current_param stores the index of the parameter in
>> +        * the array of parameters in parameterized tests.
>> +        * current_param + 1 is printed to indicate the parameter
>> +        * that causes the test to fail in case of test failure.
>> +        */
>> +       int current_param;
> 
> I think, per your comment above, this should be named "param_index".
> Also, I would suggest removing the mention of "array" in the comment,
> because the parameters aren't dependent on use of an array.
> 
>>         /*
>>          * 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 +1760,18 @@ do {                                                                            \
>>                                                 fmt,                           \
>>                                                 ##__VA_ARGS__)
>>
>> +/**
>> + * KUNIT_PARAM_GENERATOR() - Helper method for test parameter generators
>> + *                          required in parameterized tests.
>> + * @name:  prefix of the name for the test parameter generator function.
>> + * @prev: a pointer to the previous test parameter, NULL for first parameter.
>> + * @array: a user-supplied pointer to an array of test parameters.
>> + */
>> +#define KUNIT_PARAM_GENERATOR(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..b70ab9b12f3b 100644
>> --- a/lib/kunit/test.c
>> +++ b/lib/kunit/test.c
>> @@ -127,6 +127,11 @@ 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 parameter: %d\n", test->current_param + 1);
>> +}
> 
> Is this the only place where the param index is used? It might be
> helpful to show the index together with the test-case name, otherwise
> we get a series of test cases in the output which are all named the
> same which can be confusing.
> 

Yes, this is the only place param index is used.

>>  static void kunit_print_string_stream(struct kunit *test,
>>                                       struct string_stream *stream)
>>  {
>> @@ -168,6 +173,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_values)
>> +               kunit_print_failed_param(test);
>>
>>         WARN_ON(string_stream_destroy(stream));
>>  }
>> @@ -239,7 +246,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_values = test_case->generate_params(NULL);
>> +               test->current_param = 0;
>> +
>> +               while (test->param_values) {
>> +                       test_case->run_case(test);
>> +                       test->param_values = test_case->generate_params(test->param_values);
>> +                       test->current_param++;
>> +               }
>> +       }
>>  }
> 
> Looking forward to v4. :-)
> 
> Thanks,
> -- Marco
> 

I will make all the suggested changes.
Thanks!
Iurii Zaikin Oct. 27, 2020, 5:33 p.m. UTC | #8
>
> Modify fs/ext4/inode-test.c to use the parameterized testing
> feature of KUnit.
>
> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
> ---
> Changes v2->v3:
> - Marked hardcoded test data const
> - Modification based on latest implementation of KUnit parameterized testing
> Changes v1->v2:
> - Modification based on latest implementation of KUnit parameterized testing
>
>  fs/ext4/inode-test.c | 314 ++++++++++++++++++++++---------------------
>  1 file changed, 158 insertions(+), 156 deletions(-)
>
> diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
> index d62d802c9c12..3a449623b775 100644
> --- a/fs/ext4/inode-test.c
> +++ b/fs/ext4/inode-test.c
> @@ -80,6 +80,139 @@ struct timestamp_expectation {
>         bool lower_bound;
>  };
>
> +static const struct timestamp_expectation test_data[] = {
> +       {
> +               .test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE,
> +               .msb_set = true,
> +               .lower_bound = true,
> +               .extra_bits = 0,
> +               .expected = {.tv_sec = -0x80000000LL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = UPPER_BOUND_NEG_NO_EXTRA_BITS_CASE,
> +               .msb_set = true,
> +               .lower_bound = false,
> +               .extra_bits = 0,
> +               .expected = {.tv_sec = -1LL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = LOWER_BOUND_NONNEG_NO_EXTRA_BITS_CASE,
> +               .msb_set = false,
> +               .lower_bound = true,
> +               .extra_bits = 0,
> +               .expected = {0LL, 0L},
> +       },
> +
> +       {
> +               .test_case_name = UPPER_BOUND_NONNEG_NO_EXTRA_BITS_CASE,
> +               .msb_set = false,
> +               .lower_bound = false,
> +               .extra_bits = 0,
> +               .expected = {.tv_sec = 0x7fffffffLL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = LOWER_BOUND_NEG_LO_1_CASE,
> +               .msb_set = true,
> +               .lower_bound = true,
> +               .extra_bits = 1,
> +               .expected = {.tv_sec = 0x80000000LL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = UPPER_BOUND_NEG_LO_1_CASE,
> +               .msb_set = true,
> +               .lower_bound = false,
> +               .extra_bits = 1,
> +               .expected = {.tv_sec = 0xffffffffLL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = LOWER_BOUND_NONNEG_LO_1_CASE,
> +               .msb_set = false,
> +               .lower_bound = true,
> +               .extra_bits = 1,
> +               .expected = {.tv_sec = 0x100000000LL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = UPPER_BOUND_NONNEG_LO_1_CASE,
> +               .msb_set = false,
> +               .lower_bound = false,
> +               .extra_bits = 1,
> +               .expected = {.tv_sec = 0x17fffffffLL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = LOWER_BOUND_NEG_HI_1_CASE,
> +               .msb_set = true,
> +               .lower_bound = true,
> +               .extra_bits =  2,
> +               .expected = {.tv_sec = 0x180000000LL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = UPPER_BOUND_NEG_HI_1_CASE,
> +               .msb_set = true,
> +               .lower_bound = false,
> +               .extra_bits = 2,
> +               .expected = {.tv_sec = 0x1ffffffffLL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = LOWER_BOUND_NONNEG_HI_1_CASE,
> +               .msb_set = false,
> +               .lower_bound = true,
> +               .extra_bits = 2,
> +               .expected = {.tv_sec = 0x200000000LL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = UPPER_BOUND_NONNEG_HI_1_CASE,
> +               .msb_set = false,
> +               .lower_bound = false,
> +               .extra_bits = 2,
> +               .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = UPPER_BOUND_NONNEG_HI_1_NS_1_CASE,
> +               .msb_set = false,
> +               .lower_bound = false,
> +               .extra_bits = 6,
> +               .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 1L},
> +       },
> +
> +       {
> +               .test_case_name = LOWER_BOUND_NONNEG_HI_1_NS_MAX_CASE,
> +               .msb_set = false,
> +               .lower_bound = true,
> +               .extra_bits = 0xFFFFFFFF,
> +               .expected = {.tv_sec = 0x300000000LL,
> +                            .tv_nsec = MAX_NANOSECONDS},
> +       },
> +
> +       {
> +               .test_case_name = LOWER_BOUND_NONNEG_EXTRA_BITS_1_CASE,
> +               .msb_set = false,
> +               .lower_bound = true,
> +               .extra_bits = 3,
> +               .expected = {.tv_sec = 0x300000000LL, .tv_nsec = 0L},
> +       },
> +
> +       {
> +               .test_case_name = UPPER_BOUND_NONNEG_EXTRA_BITS_1_CASE,
> +               .msb_set = false,
> +               .lower_bound = false,
> +               .extra_bits = 3,
> +               .expected = {.tv_sec = 0x37fffffffLL, .tv_nsec = 0L},
> +       }
> +};
> +
> +KUNIT_PARAM_GENERATOR(ext4_inode, test_data);
> +
>  static time64_t get_32bit_time(const struct timestamp_expectation * const test)
>  {
>         if (test->msb_set) {
> @@ -101,166 +234,35 @@ static time64_t get_32bit_time(const struct timestamp_expectation * const test)
>   */
>  static void inode_test_xtimestamp_decoding(struct kunit *test)
>  {
> -       const struct timestamp_expectation test_data[] = {
> -               {
> -                       .test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE,
> -                       .msb_set = true,
> -                       .lower_bound = true,
> -                       .extra_bits = 0,
> -                       .expected = {.tv_sec = -0x80000000LL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = UPPER_BOUND_NEG_NO_EXTRA_BITS_CASE,
> -                       .msb_set = true,
> -                       .lower_bound = false,
> -                       .extra_bits = 0,
> -                       .expected = {.tv_sec = -1LL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = LOWER_BOUND_NONNEG_NO_EXTRA_BITS_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = true,
> -                       .extra_bits = 0,
> -                       .expected = {0LL, 0L},
> -               },
> -
> -               {
> -                       .test_case_name = UPPER_BOUND_NONNEG_NO_EXTRA_BITS_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = false,
> -                       .extra_bits = 0,
> -                       .expected = {.tv_sec = 0x7fffffffLL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = LOWER_BOUND_NEG_LO_1_CASE,
> -                       .msb_set = true,
> -                       .lower_bound = true,
> -                       .extra_bits = 1,
> -                       .expected = {.tv_sec = 0x80000000LL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = UPPER_BOUND_NEG_LO_1_CASE,
> -                       .msb_set = true,
> -                       .lower_bound = false,
> -                       .extra_bits = 1,
> -                       .expected = {.tv_sec = 0xffffffffLL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = LOWER_BOUND_NONNEG_LO_1_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = true,
> -                       .extra_bits = 1,
> -                       .expected = {.tv_sec = 0x100000000LL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = UPPER_BOUND_NONNEG_LO_1_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = false,
> -                       .extra_bits = 1,
> -                       .expected = {.tv_sec = 0x17fffffffLL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = LOWER_BOUND_NEG_HI_1_CASE,
> -                       .msb_set = true,
> -                       .lower_bound = true,
> -                       .extra_bits =  2,
> -                       .expected = {.tv_sec = 0x180000000LL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = UPPER_BOUND_NEG_HI_1_CASE,
> -                       .msb_set = true,
> -                       .lower_bound = false,
> -                       .extra_bits = 2,
> -                       .expected = {.tv_sec = 0x1ffffffffLL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = LOWER_BOUND_NONNEG_HI_1_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = true,
> -                       .extra_bits = 2,
> -                       .expected = {.tv_sec = 0x200000000LL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = UPPER_BOUND_NONNEG_HI_1_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = false,
> -                       .extra_bits = 2,
> -                       .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = UPPER_BOUND_NONNEG_HI_1_NS_1_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = false,
> -                       .extra_bits = 6,
> -                       .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 1L},
> -               },
> -
> -               {
> -                       .test_case_name = LOWER_BOUND_NONNEG_HI_1_NS_MAX_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = true,
> -                       .extra_bits = 0xFFFFFFFF,
> -                       .expected = {.tv_sec = 0x300000000LL,
> -                                    .tv_nsec = MAX_NANOSECONDS},
> -               },
> -
> -               {
> -                       .test_case_name = LOWER_BOUND_NONNEG_EXTRA_BITS_1_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = true,
> -                       .extra_bits = 3,
> -                       .expected = {.tv_sec = 0x300000000LL, .tv_nsec = 0L},
> -               },
> -
> -               {
> -                       .test_case_name = UPPER_BOUND_NONNEG_EXTRA_BITS_1_CASE,
> -                       .msb_set = false,
> -                       .lower_bound = false,
> -                       .extra_bits = 3,
> -                       .expected = {.tv_sec = 0x37fffffffLL, .tv_nsec = 0L},
> -               }
> -       };
> -
>         struct timespec64 timestamp;
> -       int i;
> -
> -       for (i = 0; i < ARRAY_SIZE(test_data); ++i) {
> -               timestamp.tv_sec = get_32bit_time(&test_data[i]);
> -               ext4_decode_extra_time(&timestamp,
> -                                      cpu_to_le32(test_data[i].extra_bits));
> -
> -               KUNIT_EXPECT_EQ_MSG(test,
> -                                   test_data[i].expected.tv_sec,
> -                                   timestamp.tv_sec,
> -                                   CASE_NAME_FORMAT,
> -                                   test_data[i].test_case_name,
> -                                   test_data[i].msb_set,
> -                                   test_data[i].lower_bound,
> -                                   test_data[i].extra_bits);
> -               KUNIT_EXPECT_EQ_MSG(test,
> -                                   test_data[i].expected.tv_nsec,
> -                                   timestamp.tv_nsec,
> -                                   CASE_NAME_FORMAT,
> -                                   test_data[i].test_case_name,
> -                                   test_data[i].msb_set,
> -                                   test_data[i].lower_bound,
> -                                   test_data[i].extra_bits);
> -       }
> +
> +       struct timestamp_expectation *test_param =
> +                       (struct timestamp_expectation *)(test->param_values);
> +
> +       timestamp.tv_sec = get_32bit_time(test_param);
> +       ext4_decode_extra_time(&timestamp,
> +                              cpu_to_le32(test_param->extra_bits));
> +
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           test_param->expected.tv_sec,
> +                           timestamp.tv_sec,
> +                           CASE_NAME_FORMAT,
> +                           test_param->test_case_name,
> +                           test_param->msb_set,
> +                           test_param->lower_bound,
> +                           test_param->extra_bits);
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           test_param->expected.tv_nsec,
> +                           timestamp.tv_nsec,
> +                           CASE_NAME_FORMAT,
> +                           test_param->test_case_name,
> +                           test_param->msb_set,
> +                           test_param->lower_bound,
> +                           test_param->extra_bits);
>  }
>
>  static struct kunit_case ext4_inode_test_cases[] = {
> -       KUNIT_CASE(inode_test_xtimestamp_decoding),
> +       KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding, ext4_inode_gen_params),
>         {}
>  };
>
> --
> 2.25.1
>

Reviewed-by: Iurii Zaikin <yzaikin@google.com>
diff mbox series

Patch

diff --git a/include/kunit/test.h b/include/kunit/test.h
index a423fffefea0..16bf9f334e2c 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -142,6 +142,12 @@  struct kunit_case {
 	void (*run_case)(struct kunit *test);
 	const char *name;
 
+	/*
+	 * Pointer to test parameter generator function.
+	 * Used only for parameterized tests.
+	 */
+	void* (*generate_params)(void *prev);
+
 	/* private: internal use only. */
 	bool success;
 	char *log;
@@ -162,6 +168,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 +217,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_values points to test case parameters in parameterized tests */
+	void *param_values;
+	/*
+	 * current_param stores the index of the parameter in
+	 * the array of parameters in parameterized tests.
+	 * current_param + 1 is printed to indicate the parameter
+	 * that causes the test to fail in case of test failure.
+	 */
+	int current_param;
 	/*
 	 * 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 +1760,18 @@  do {									       \
 						fmt,			       \
 						##__VA_ARGS__)
 
+/**
+ * KUNIT_PARAM_GENERATOR() - Helper method for test parameter generators
+ * 			     required in parameterized tests.
+ * @name:  prefix of the name for the test parameter generator function.
+ * @prev: a pointer to the previous test parameter, NULL for first parameter.
+ * @array: a user-supplied pointer to an array of test parameters.
+ */
+#define KUNIT_PARAM_GENERATOR(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..b70ab9b12f3b 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -127,6 +127,11 @@  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 parameter: %d\n", test->current_param + 1);
+}
+
 static void kunit_print_string_stream(struct kunit *test,
 				      struct string_stream *stream)
 {
@@ -168,6 +173,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_values)
+		kunit_print_failed_param(test);
 
 	WARN_ON(string_stream_destroy(stream));
 }
@@ -239,7 +246,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_values = test_case->generate_params(NULL);
+		test->current_param = 0;
+
+		while (test->param_values) {
+			test_case->run_case(test);
+			test->param_values = test_case->generate_params(test->param_values);
+			test->current_param++;
+		}
+	}
 }
 
 static void kunit_case_internal_cleanup(struct kunit *test)