On Tue, Aug 5, 2025 at 11:17 AM Rae Moar <rm...@google.com> wrote:
>
> On Tue, Jul 29, 2025 at 3:37 PM Marie Zhussupova <marie...@google.com> wrote:
> >
> > Currently, KUnit parameterized tests lack a mechanism
> > to share resources across individual test invocations
> > because the same `struct kunit` instance is reused for
> > each test.
> >
> > This patch refactors kunit_run_tests() to provide each
> > parameterized test with its own `struct kunit` instance.
> > A new parent pointer is added to `struct kunit`, allowing
> > individual parameterized tests to reference a shared
> > parent kunit instance. Resources added to this parent
> > will then be accessible to all individual parameter
> > test executions.
> >
> > Signed-off-by: Marie Zhussupova <marie...@google.com>
>
> Hello!
>
> Thank you so much for sending out this series. I have wanted to see an
> update of our parameterized test framework for a while. I have a few
> comments below for this patch. But otherwise it is looking good.
>
> Reviewed-by: Rae Moar <rm...@google.com>
>
> Thanks!
> -Rae
>
> > ---
> > include/kunit/test.h | 12 ++++++++++--
> > lib/kunit/test.c | 32 +++++++++++++++++++-------------
> > 2 files changed, 29 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index 39c768f87dc9..a42d0c8cb985 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -268,14 +268,22 @@ 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.
> > *
>
> As David mentioned, I would also prefer that this provides a more
> general description of the @parent field here. Although this is
> currently only used for parameterized tests, it could have other use
> cases in the future.
>
Will edit this in v2.
> > * 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
> > - * indirectly via public functions; the one exception is @priv which can be
> > - * used by the test writer to store arbitrary data.
> > + * indirectly via public functions; the two exceptions are @priv and
> > @parent
> > + * which can be used by the test writer to store arbitrary data or data
> > that is
> > + * available to all parameter test executions, respectively.
>
> In addition, I would prefer that the call out to @parent here is also
> changed to a more general description of the @parent field. However,
> feel free to also include the description of the use case for the
> parameterized tests.
>
I will edit this in v2, as well.
> > */
> > struct kunit {
> > void *priv;
> > + /*
> > + * Reference to the parent struct kunit for storing shared resources
> > + * during parameterized testing.
> > + */
>
> I am more 50/50 on changing this description. Could change it just to:
> "Reference to the parent struct kunit for storing shared resources."
Thank you for the suggestion! The description would sound good.
>
> > + struct kunit *parent;
> >
> > /* private: internal use only. */
> > const char *name; /* Read only after initialization! */
> > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > index f3c6b11f12b8..4d6a39eb2c80 100644
> > --- a/lib/kunit/test.c
> > +++ b/lib/kunit/test.c
> > @@ -647,6 +647,7 @@ int kunit_run_tests(struct kunit_suite *suite)
> > struct kunit_case *test_case;
> > struct kunit_result_stats suite_stats = { 0 };
> > struct kunit_result_stats total_stats = { 0 };
> > + const void *curr_param;
> >
> > /* Taint the kernel so we know we've run tests. */
> > add_taint(TAINT_TEST, LOCKDEP_STILL_OK);
> > @@ -679,36 +680,39 @@ int kunit_run_tests(struct kunit_suite *suite)
> > } else {
> > /* Get initial param. */
> > param_desc[0] = '\0';
> > - test.param_value = test_case->generate_params(NULL,
> > param_desc);
> > + /* TODO: Make generate_params try-catch */
> > + curr_param = test_case->generate_params(NULL,
> > param_desc);
> > test_case->status = KUNIT_SKIPPED;
> > kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT
> > KUNIT_SUBTEST_INDENT
> > "KTAP version 1\n");
> > kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT
> > KUNIT_SUBTEST_INDENT
> > "# Subtest: %s", test_case->name);
> >
> > - while (test.param_value) {
> > - kunit_run_case_catch_errors(suite,
> > test_case, &test);
> > + while (curr_param) {
> > + struct kunit param_test = {
> > + .param_value = curr_param,
> > + .param_index = ++test.param_index,
> > + .parent = &test,
> > + };
> > + kunit_init_test(¶m_test,
> > test_case->name, test_case->log);
> > + kunit_run_case_catch_errors(suite,
> > test_case, ¶m_test);
> >
> > if (param_desc[0] == '\0') {
> > snprintf(param_desc,
> > sizeof(param_desc),
> > "param-%d",
> > test.param_index);
>
> This probably doesn't matter too much either way but should this be
> param_test.param_index instead? This would cover the case where the
> param_index is changed during the test run even though it shouldn't.
>
Thank you for catching this!
> > }
> >
> > - kunit_print_ok_not_ok(&test,
> > KUNIT_LEVEL_CASE_PARAM,
> > - test.status,
> > - test.param_index + 1,
> > + kunit_print_ok_not_ok(¶m_test,
> > KUNIT_LEVEL_CASE_PARAM,
> > + param_test.status,
> > +
> > param_test.param_index,
> > param_desc,
> > - test.status_comment);
> > +
> > param_test.status_comment);
> >
> > - kunit_update_stats(¶m_stats,
> > test.status);
> > + kunit_update_stats(¶m_stats,
> > param_test.status);
> >
> > /* Get next param. */
> > param_desc[0] = '\0';
> > - test.param_value =
> > test_case->generate_params(test.param_value, param_desc);
> > - test.param_index++;
> > - test.status = KUNIT_SUCCESS;
> > - test.status_comment[0] = '\0';
> > - test.priv = NULL;
> > + curr_param =
> > test_case->generate_params(curr_param, param_desc);
> > }
> > }
> >
> > @@ -723,6 +727,8 @@ 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 this kunit_cleanup into a try-catch. */
> > + kunit_cleanup(&test);
>
> I might be missing something here but why not do this cleanup before
> the printing stage and only if the test was a parent param test?
>
Thank you for catching this too, it should be only for the parent param test.
>
>
> > }
> >
> > if (suite->suite_exit)
> > --
> > 2.50.1.552.g942d659e1b-goog
> >
