| Message ID | 20201026183523.82749-1-98.arpi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [v3,1/2] kunit: Support for Parameterized Testing | expand |
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
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!
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
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.
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
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
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!
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)