| Message ID | 20201010145357.60886-1-98.arpi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [1/2] kunit: Support for Parameterized Testing | expand |
On Sat, 10 Oct 2020 at 16:54, Arpitha Raghunandan <98.arpi@gmail.com> wrote: > Implementation of support for parameterized testing in KUnit. > > Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com> > --- > include/kunit/test.h | 29 +++++++++++++++++++++++++++++ > lib/kunit/test.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 72 insertions(+), 1 deletion(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index 59f3144f009a..4740d66269b4 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -140,10 +140,14 @@ struct kunit; > struct kunit_case { > void (*run_case)(struct kunit *test); > const char *name; > + void* (*get_params)(void); > + int max_parameters_count; > + int parameter_size; > > /* private: internal use only. */ > bool success; > char *log; > + bool parameterized; Why do you need this bool? Doesn't get_params being non-NULL tell you if the test case is parameterized? > }; > > static inline char *kunit_status_to_string(bool status) > @@ -162,6 +166,11 @@ static inline char *kunit_status_to_string(bool status) > */ > #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } > > +#define KUNIT_CASE_PARAM(test_name, getparams, count, size) \ > + { .run_case = test_name, .name = #test_name, \ > + .parameterized = true, .get_params = (void* (*)(void))getparams, \ > + .max_parameters_count = count, .parameter_size = size } > + I think this interface is overly complex. For one, if the only purpose of the getparams function is to return a pointer to some array, then there are only few cases where I see getparams being a function could be useful. Instead, could we make the getparams function behave like a generator? Because then you do not need count, nor size. Its function signature would be: void* (*generate_params)(void* prev_param); The protocol would be: - The first call to generate_params is passed prev_param of NULL, and returns a pointer to the first parameter P[0]. - Every nth successive call to generate_params is passed the previous parameter P[n-1]. - When no more parameters are available, generate_params returns NULL. - (generate_params should otherwise be stateless, but this is only relevant if concurrent calls are expected.) > /** > * struct kunit_suite - describes a related collection of &struct kunit_case > * > @@ -206,6 +215,23 @@ struct kunit { > /* private: internal use only. */ > const char *name; /* Read only after initialization! */ > char *log; /* Points at case log after initialization */ > + bool parameterized; /* True for parameterized tests */ > + /* param_values stores the test parameters > + * for parameterized tests. > + */ > + void *param_values; > + /* max_parameters_count indicates maximum number of > + * parameters for parameterized tests. > + */ > + int max_parameters_count; > + /* iterator_count is used by the iterator method > + * for parameterized tests. > + */ > + int iterator_count; > + /* parameter_size indicates size of a single test case > + * for parameterized tests. > + */ > + int parameter_size; All of this would become much simpler if you used the generator approach. Likely only 1 field would be required, which is the current param. > struct kunit_try_catch try_catch; > /* > * success starts as true, and may only be set to false during a > @@ -225,6 +251,7 @@ struct kunit { > }; > > void kunit_init_test(struct kunit *test, const char *name, char *log); > +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case); > > int kunit_run_tests(struct kunit_suite *suite); > > @@ -237,6 +264,8 @@ int __kunit_test_suites_init(struct kunit_suite **suites); > > void __kunit_test_suites_exit(struct kunit_suite **suites); > > +void *get_test_case_parameters(struct kunit *test); > + > /** > * kunit_test_suites() - used to register one or more &struct kunit_suite > * with KUnit. > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index c36037200310..ab9e13c81d4a 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -142,6 +142,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->iterator_count); > +} > + > static void kunit_print_string_stream(struct kunit *test, > struct string_stream *stream) > { > @@ -182,6 +187,9 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) > > assert->format(assert, stream); > > + if (test->parameterized) > + kunit_print_failed_param(test); > + > kunit_print_string_stream(test, stream); > > WARN_ON(string_stream_destroy(stream)); > @@ -236,6 +244,18 @@ void kunit_init_test(struct kunit *test, const char *name, char *log) > } > EXPORT_SYMBOL_GPL(kunit_init_test); > > +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case) > +{ > + spin_lock_init(&test->lock); > + INIT_LIST_HEAD(&test->resources); > + test->parameterized = true; > + test->param_values = (void *)(test_case->get_params()); > + test->max_parameters_count = test_case->max_parameters_count; > + test->parameter_size = test_case->parameter_size; > + test->iterator_count = 0; > +} > +EXPORT_SYMBOL_GPL(kunit_init_param_test); > + > /* > * Initializes and runs test case. Does not clean up or do post validations. > */ > @@ -254,7 +274,14 @@ static void kunit_run_case_internal(struct kunit *test, > } > } > > - test_case->run_case(test); > + if (!test->parameterized) { > + test_case->run_case(test); > + } else { > + int i; > + > + for (i = 0; i < test->max_parameters_count; i++) > + test_case->run_case(test); With a generator approach, here you'd call generate_params. Most likely, you'll need to stash its result somewhere, e.g. test->param, so it can be retrieved by the test case. > + } > } > > static void kunit_case_internal_cleanup(struct kunit *test) > @@ -343,6 +370,8 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite, > struct kunit test; > > kunit_init_test(&test, test_case->name, test_case->log); > + if (test_case->parameterized) > + kunit_init_param_test(&test, test_case); > try_catch = &test.try_catch; > > kunit_try_catch_init(try_catch, > @@ -407,6 +436,19 @@ void __kunit_test_suites_exit(struct kunit_suite **suites) > } > EXPORT_SYMBOL_GPL(__kunit_test_suites_exit); > > +/* > + * Iterator method for the parameterized test cases > + */ > +void *get_test_case_parameters(struct kunit *test) > +{ > + int index = test->iterator_count * test->parameter_size; > + > + if (test->iterator_count != test->max_parameters_count) > + test->iterator_count++; This is quite confusing, because if get_test_case_parameters is called multiple times within the same test case, we'll iterate through all the test case params in the same test case? I think this function should not have side-effects (like normal getters). But if you use the generator approach, you'll likely not need this function anyway. > + return (test->param_values + index); Braces not needed. > +} > +EXPORT_SYMBOL_GPL(get_test_case_parameters); > + > /* > * Used for static resources and when a kunit_resource * has been created by > * kunit_alloc_resource(). When an init function is supplied, @data is passed > -- > 2.25.1 >
On 12/10/20 4:30 pm, Marco Elver wrote: > On Sat, 10 Oct 2020 at 16:54, Arpitha Raghunandan <98.arpi@gmail.com> wrote: >> Implementation of support for parameterized testing in KUnit. >> >> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com> >> --- >> include/kunit/test.h | 29 +++++++++++++++++++++++++++++ >> lib/kunit/test.c | 44 +++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 72 insertions(+), 1 deletion(-) >> >> diff --git a/include/kunit/test.h b/include/kunit/test.h >> index 59f3144f009a..4740d66269b4 100644 >> --- a/include/kunit/test.h >> +++ b/include/kunit/test.h >> @@ -140,10 +140,14 @@ struct kunit; >> struct kunit_case { >> void (*run_case)(struct kunit *test); >> const char *name; >> + void* (*get_params)(void); >> + int max_parameters_count; >> + int parameter_size; >> >> /* private: internal use only. */ >> bool success; >> char *log; >> + bool parameterized; > > Why do you need this bool? Doesn't get_params being non-NULL tell you > if the test case is parameterized? >Yeah, this will. >> }; >> >> static inline char *kunit_status_to_string(bool status) >> @@ -162,6 +166,11 @@ static inline char *kunit_status_to_string(bool status) >> */ >> #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } >> >> +#define KUNIT_CASE_PARAM(test_name, getparams, count, size) \ >> + { .run_case = test_name, .name = #test_name, \ >> + .parameterized = true, .get_params = (void* (*)(void))getparams, \ >> + .max_parameters_count = count, .parameter_size = size } >> + > > I think this interface is overly complex. For one, if the only purpose > of the getparams function is to return a pointer to some array, then > there are only few cases where I see getparams being a function could > be useful. > > Instead, could we make the getparams function behave like a generator? > Because then you do not need count, nor size. Its function signature > would be: > > void* (*generate_params)(void* prev_param); > > The protocol would be: > > - The first call to generate_params is passed prev_param of NULL, and > returns a pointer to the first parameter P[0]. > > - Every nth successive call to generate_params is passed the previous > parameter P[n-1]. > > - When no more parameters are available, generate_params returns NULL. > > - (generate_params should otherwise be stateless, but this is only > relevant if concurrent calls are expected.) > > >> /** >> * struct kunit_suite - describes a related collection of &struct kunit_case >> * >> @@ -206,6 +215,23 @@ struct kunit { >> /* private: internal use only. */ >> const char *name; /* Read only after initialization! */ >> char *log; /* Points at case log after initialization */ >> + bool parameterized; /* True for parameterized tests */ >> + /* param_values stores the test parameters >> + * for parameterized tests. >> + */ >> + void *param_values; >> + /* max_parameters_count indicates maximum number of >> + * parameters for parameterized tests. >> + */ >> + int max_parameters_count; >> + /* iterator_count is used by the iterator method >> + * for parameterized tests. >> + */ >> + int iterator_count; >> + /* parameter_size indicates size of a single test case >> + * for parameterized tests. >> + */ >> + int parameter_size; > > All of this would become much simpler if you used the generator > approach. Likely only 1 field would be required, which is the current > param. > >> struct kunit_try_catch try_catch; >> /* >> * success starts as true, and may only be set to false during a >> @@ -225,6 +251,7 @@ struct kunit { >> }; >> >> void kunit_init_test(struct kunit *test, const char *name, char *log); >> +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case); >> >> int kunit_run_tests(struct kunit_suite *suite); >> >> @@ -237,6 +264,8 @@ int __kunit_test_suites_init(struct kunit_suite **suites); >> >> void __kunit_test_suites_exit(struct kunit_suite **suites); >> >> +void *get_test_case_parameters(struct kunit *test); >> + >> /** >> * kunit_test_suites() - used to register one or more &struct kunit_suite >> * with KUnit. >> diff --git a/lib/kunit/test.c b/lib/kunit/test.c >> index c36037200310..ab9e13c81d4a 100644 >> --- a/lib/kunit/test.c >> +++ b/lib/kunit/test.c >> @@ -142,6 +142,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->iterator_count); >> +} >> + >> static void kunit_print_string_stream(struct kunit *test, >> struct string_stream *stream) >> { >> @@ -182,6 +187,9 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) >> >> assert->format(assert, stream); >> >> + if (test->parameterized) >> + kunit_print_failed_param(test); >> + >> kunit_print_string_stream(test, stream); >> >> WARN_ON(string_stream_destroy(stream)); >> @@ -236,6 +244,18 @@ void kunit_init_test(struct kunit *test, const char *name, char *log) >> } >> EXPORT_SYMBOL_GPL(kunit_init_test); >> >> +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case) >> +{ >> + spin_lock_init(&test->lock); >> + INIT_LIST_HEAD(&test->resources); >> + test->parameterized = true; >> + test->param_values = (void *)(test_case->get_params()); >> + test->max_parameters_count = test_case->max_parameters_count; >> + test->parameter_size = test_case->parameter_size; >> + test->iterator_count = 0; >> +} >> +EXPORT_SYMBOL_GPL(kunit_init_param_test); >> + >> /* >> * Initializes and runs test case. Does not clean up or do post validations. >> */ >> @@ -254,7 +274,14 @@ static void kunit_run_case_internal(struct kunit *test, >> } >> } >> >> - test_case->run_case(test); >> + if (!test->parameterized) { >> + test_case->run_case(test); >> + } else { >> + int i; >> + >> + for (i = 0; i < test->max_parameters_count; i++) >> + test_case->run_case(test); > > With a generator approach, here you'd call generate_params. Most > likely, you'll need to stash its result somewhere, e.g. test->param, > so it can be retrieved by the test case. > >> + } >> } >> >> static void kunit_case_internal_cleanup(struct kunit *test) >> @@ -343,6 +370,8 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite, >> struct kunit test; >> >> kunit_init_test(&test, test_case->name, test_case->log); >> + if (test_case->parameterized) >> + kunit_init_param_test(&test, test_case); >> try_catch = &test.try_catch; >> >> kunit_try_catch_init(try_catch, >> @@ -407,6 +436,19 @@ void __kunit_test_suites_exit(struct kunit_suite **suites) >> } >> EXPORT_SYMBOL_GPL(__kunit_test_suites_exit); >> >> +/* >> + * Iterator method for the parameterized test cases >> + */ >> +void *get_test_case_parameters(struct kunit *test) >> +{ >> + int index = test->iterator_count * test->parameter_size; >> + >> + if (test->iterator_count != test->max_parameters_count) >> + test->iterator_count++; > > This is quite confusing, because if get_test_case_parameters is called > multiple times within the same test case, we'll iterate through all > the test case params in the same test case? I think this function > should not have side-effects (like normal getters). > > But if you use the generator approach, you'll likely not need this > function anyway. > The generator approach sounds good. I will work on it for the next version. >> + return (test->param_values + index); > > Braces not needed. > I will fix this. >> +} >> +EXPORT_SYMBOL_GPL(get_test_case_parameters); >> + >> /* >> * Used for static resources and when a kunit_resource * has been created by >> * kunit_alloc_resource(). When an init function is supplied, @data is passed >> -- >> 2.25.1 >>
diff --git a/include/kunit/test.h b/include/kunit/test.h index 59f3144f009a..4740d66269b4 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -140,10 +140,14 @@ struct kunit; struct kunit_case { void (*run_case)(struct kunit *test); const char *name; + void* (*get_params)(void); + int max_parameters_count; + int parameter_size; /* private: internal use only. */ bool success; char *log; + bool parameterized; }; static inline char *kunit_status_to_string(bool status) @@ -162,6 +166,11 @@ static inline char *kunit_status_to_string(bool status) */ #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } +#define KUNIT_CASE_PARAM(test_name, getparams, count, size) \ + { .run_case = test_name, .name = #test_name, \ + .parameterized = true, .get_params = (void* (*)(void))getparams, \ + .max_parameters_count = count, .parameter_size = size } + /** * struct kunit_suite - describes a related collection of &struct kunit_case * @@ -206,6 +215,23 @@ struct kunit { /* private: internal use only. */ const char *name; /* Read only after initialization! */ char *log; /* Points at case log after initialization */ + bool parameterized; /* True for parameterized tests */ + /* param_values stores the test parameters + * for parameterized tests. + */ + void *param_values; + /* max_parameters_count indicates maximum number of + * parameters for parameterized tests. + */ + int max_parameters_count; + /* iterator_count is used by the iterator method + * for parameterized tests. + */ + int iterator_count; + /* parameter_size indicates size of a single test case + * for parameterized tests. + */ + int parameter_size; struct kunit_try_catch try_catch; /* * success starts as true, and may only be set to false during a @@ -225,6 +251,7 @@ struct kunit { }; void kunit_init_test(struct kunit *test, const char *name, char *log); +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case); int kunit_run_tests(struct kunit_suite *suite); @@ -237,6 +264,8 @@ int __kunit_test_suites_init(struct kunit_suite **suites); void __kunit_test_suites_exit(struct kunit_suite **suites); +void *get_test_case_parameters(struct kunit *test); + /** * kunit_test_suites() - used to register one or more &struct kunit_suite * with KUnit. diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c36037200310..ab9e13c81d4a 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -142,6 +142,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->iterator_count); +} + static void kunit_print_string_stream(struct kunit *test, struct string_stream *stream) { @@ -182,6 +187,9 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) assert->format(assert, stream); + if (test->parameterized) + kunit_print_failed_param(test); + kunit_print_string_stream(test, stream); WARN_ON(string_stream_destroy(stream)); @@ -236,6 +244,18 @@ void kunit_init_test(struct kunit *test, const char *name, char *log) } EXPORT_SYMBOL_GPL(kunit_init_test); +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case) +{ + spin_lock_init(&test->lock); + INIT_LIST_HEAD(&test->resources); + test->parameterized = true; + test->param_values = (void *)(test_case->get_params()); + test->max_parameters_count = test_case->max_parameters_count; + test->parameter_size = test_case->parameter_size; + test->iterator_count = 0; +} +EXPORT_SYMBOL_GPL(kunit_init_param_test); + /* * Initializes and runs test case. Does not clean up or do post validations. */ @@ -254,7 +274,14 @@ static void kunit_run_case_internal(struct kunit *test, } } - test_case->run_case(test); + if (!test->parameterized) { + test_case->run_case(test); + } else { + int i; + + for (i = 0; i < test->max_parameters_count; i++) + test_case->run_case(test); + } } static void kunit_case_internal_cleanup(struct kunit *test) @@ -343,6 +370,8 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite, struct kunit test; kunit_init_test(&test, test_case->name, test_case->log); + if (test_case->parameterized) + kunit_init_param_test(&test, test_case); try_catch = &test.try_catch; kunit_try_catch_init(try_catch, @@ -407,6 +436,19 @@ void __kunit_test_suites_exit(struct kunit_suite **suites) } EXPORT_SYMBOL_GPL(__kunit_test_suites_exit); +/* + * Iterator method for the parameterized test cases + */ +void *get_test_case_parameters(struct kunit *test) +{ + int index = test->iterator_count * test->parameter_size; + + if (test->iterator_count != test->max_parameters_count) + test->iterator_count++; + return (test->param_values + index); +} +EXPORT_SYMBOL_GPL(get_test_case_parameters); + /* * Used for static resources and when a kunit_resource * has been created by * kunit_alloc_resource(). When an init function is supplied, @data is passed
Implementation of support for parameterized testing in KUnit. Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com> --- include/kunit/test.h | 29 +++++++++++++++++++++++++++++ lib/kunit/test.c | 44 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletion(-)