On Tue, Aug 5, 2025 at 11:18 AM Rae Moar <rm...@google.com> wrote: > > On Tue, Jul 29, 2025 at 3:37 PM Marie Zhussupova <marie...@google.com> wrote: > > > > Add `param_init` and `param_exit` function pointers to > > `struct kunit_case`. Users will be able to set them > > via the new `KUNIT_CASE_PARAM_WITH_INIT` macro. > > Hello! > > Very intrigued by this idea to add an init and exit function for > parameterized tests. In a way, this allows parameterized test series > to act more like suites. Either way I am happy to see more flexibility > being brought to the parameterized test framework. > > I have a few comments below that I would like to discuss before a > final review. But this patch is looking good. > > Thanks! > -Rae > > > > > These functions are invoked by kunit_run_tests() once before > > and once after the entire parameterized test series, respectively. > > This is a philosophical question but should we refer to a group of > parameterized tests as a parameterized test series or a parameterized > test suite? In the KTAP, the appearance is identical to a suite but in > the running of the tests it acts distinct to a test case or suite. > Curious on David's opinion here. >
Thank you for bringing this up! Using the wording of the patch that introduced the parameterized tests: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/lib/kunit?id=fadb08e7c7501ed42949e646c6865ba4ec5dd948, "parameterized test" will refer to a group of parameterized tests and "parameter run" will refer to a single parameter execution. I will also specify this terminology in the docs for v2. > > They will receive the parent kunit test instance, allowing users > > to register and manage shared resources. Resources added to this > > parent kunit test will be accessible to all individual parameterized > > tests, facilitating init and exit for shared state. > > > > Signed-off-by: Marie Zhussupova <marie...@google.com> > > --- > > include/kunit/test.h | 33 ++++++++++++++++++++++++++++++++- > > lib/kunit/test.c | 23 ++++++++++++++++++++++- > > 2 files changed, 54 insertions(+), 2 deletions(-) > > > > diff --git a/include/kunit/test.h b/include/kunit/test.h > > index a42d0c8cb985..d8dac7efd745 100644 > > --- a/include/kunit/test.h > > +++ b/include/kunit/test.h > > @@ -92,6 +92,8 @@ struct kunit_attributes { > > * @name: the name of the test case. > > * @generate_params: the generator function for parameterized tests. > > * @attr: the attributes associated with the test > > + * @param_init: The init function to run before parameterized tests. > > + * @param_exit: The exit function to run after parameterized tests. > > If we decide on a terminology for the parameterized test group, it > might be clearer to label these "The init function to run before > parameterized test [suite/series]." and same for the exit function. > Will update this in v2. > > * > > * A test case is a function with the signature, > > * ``void (*)(struct kunit *)`` > > @@ -129,6 +131,13 @@ struct kunit_case { > > const void* (*generate_params)(const void *prev, char *desc); > > struct kunit_attributes attr; > > > > + /* > > + * Optional user-defined functions: one to register shared > > resources once > > + * before the parameterized test series, and another to release > > them after. > > + */ > > + int (*param_init)(struct kunit *test); > > + void (*param_exit)(struct kunit *test); > > + > > /* private: internal use only. */ > > enum kunit_status status; > > char *module_name; > > @@ -218,6 +227,27 @@ static inline char *kunit_status_to_ok_not_ok(enum > > kunit_status status) > > .generate_params = gen_params, > > \ > > .attr = attributes, .module_name = KBUILD_MODNAME} > > > > +/** > > + * KUNIT_CASE_PARAM_WITH_INIT() - Define a parameterized KUnit test case > > with custom > > + * init and exit functions. > > + * @test_name: The function implementing the test case. > > + * @gen_params: The function to generate parameters for the test case. > > + * @init: The init function to run before parameterized tests. > > + * @exit: The exit function to run after parameterized tests. > > If we do change the description above of param_init/param_exit, it > might be nice to change it here too. > Will update this, as well. > > + * > > + * Provides the option to register init and exit functions that take in the > > + * parent of the parameterized tests and run once before and once after the > > + * parameterized test series. The init function can be used to add any > > resources > > + * to share between the parameterized tests or to pass parameter arrays. > > The > > + * exit function can be used to clean up any resources that are not > > managed by > > + * the test. > > + */ > > +#define KUNIT_CASE_PARAM_WITH_INIT(test_name, gen_params, init, exit) > > \ > > + { .run_case = test_name, .name = #test_name, > > \ > > + .generate_params = gen_params, > > \ > > + .param_init = init, .param_exit = exit, > > \ > > + .module_name = KBUILD_MODNAME} > > + > > /** > > * struct kunit_suite - describes a related collection of &struct > > kunit_case > > * > > @@ -269,7 +299,8 @@ struct kunit_suite_set { > > * @priv: for user to store arbitrary data. Commonly used to pass data > > * created in the init function (see &struct kunit_suite). > > * @parent: for user to store data that they want to shared across > > - * parameterized tests. > > + * parameterized tests. Typically, the data is provided in > > + * the param_init function (see &struct kunit_case). > > * > > * Used to store information about the current context under which the test > > * is running. Most of this data is private and should only be accessed > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > > index 4d6a39eb2c80..d80b5990d85d 100644 > > --- a/lib/kunit/test.c > > +++ b/lib/kunit/test.c > > @@ -641,6 +641,19 @@ static void kunit_accumulate_stats(struct > > kunit_result_stats *total, > > total->total += add.total; > > } > > > > +static void __kunit_init_parent_test(struct kunit_case *test_case, struct > > kunit *test) > > It would be nice to include "param" in this function name. Currently > it sounds more like you are initializing the @parent field of struct > kunit *test. That is a great suggestion, I will incorporate it in v2. > > > +{ > > + if (test_case->param_init) { > > + int err = test_case->param_init(test); > > + > > + if (err) { > > + kunit_err(test_case, KUNIT_SUBTEST_INDENT > > KUNIT_SUBTEST_INDENT > > + "# failed to initialize parent parameter > > test."); > > + test_case->status = KUNIT_FAILURE; > > + } > > + } > > +} > > + > > int kunit_run_tests(struct kunit_suite *suite) > > { > > char param_desc[KUNIT_PARAM_DESC_SIZE]; > > @@ -668,6 +681,8 @@ int kunit_run_tests(struct kunit_suite *suite) > > struct kunit_result_stats param_stats = { 0 }; > > > > kunit_init_test(&test, test_case->name, test_case->log); > > + __kunit_init_parent_test(test_case, &test); > > + > > Is it possible to move this so this function is only called when it is > a parameterized test? I see the check for KUNIT_FAILURE is useful but > I think I would still prefer this within the section for parameterized > tests. Yes, I will do that, unless we decide to go with the route to set generate_params to point to kunit_get_next_param_and_desc in the __kunit_init_parent_test function. > > > if (test_case->status == KUNIT_SKIPPED) { > > /* Test marked as skip */ > > test.status = KUNIT_SKIPPED; > > @@ -677,7 +692,7 @@ int kunit_run_tests(struct kunit_suite *suite) > > test_case->status = KUNIT_SKIPPED; > > kunit_run_case_catch_errors(suite, test_case, > > &test); > > kunit_update_stats(¶m_stats, test.status); > > - } else { > > + } else if (test_case->status != KUNIT_FAILURE) { > > /* Get initial param. */ > > param_desc[0] = '\0'; > > /* TODO: Make generate_params try-catch */ > > @@ -727,6 +742,12 @@ int kunit_run_tests(struct kunit_suite *suite) > > > > kunit_update_stats(&suite_stats, test_case->status); > > kunit_accumulate_stats(&total_stats, param_stats); > > + /* > > + * TODO: Put into a try catch. Since we don't need > > suite->exit > > + * for it we can't reuse kunit_try_run_cleanup for this yet. > > + */ > > + if (test_case->param_exit) > > + test_case->param_exit(&test); > > Also here I am not sure why this is done outside of the check for if > the test is parameterized? Either way this should definitely be done > before the test stats and ok/not ok line are printed because if there > is any log output during the param_exit function it is necessary to > print that before the status line to identify that that log > corresponds with that test. Thank you for catching this! Yes, it should be inside the check for if the test is parameterized. > > Also just curious why you chose to implement a function to perform the > param_init but not the param_exit? To be consistent with the existing style of "exit" functions in KUnit, test->param_exit returns void. Therefore, similar to how it's done for suite->suite_exit (https://elixir.bootlin.com/linux/v6.16/source/lib/kunit/test.c#L685), I didn't do extra error handling as the function itself doesn't indicate an error and therefore, didn't put it in a separate function. To do error handling for it, it would need to be in a try catch, then we could check if the cleanup timed out or if there was an internal error. > > > > > /* TODO: Put this kunit_cleanup into a try-catch. */ > > kunit_cleanup(&test); > > } > > -- > > 2.50.1.552.g942d659e1b-goog > >
