On Tue, Aug 5, 2025 at 11:17 AM Rae Moar rmoar@google.com wrote:
On Tue, Jul 29, 2025 at 3:37 PM Marie Zhussupova marievic@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 marievic@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 rmoar@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