On Wed, Oct 27, 2021 at 11:42 PM David Gow <davidgow@xxxxxxxxxx> wrote: > > It's possible that a parameterised test could end up with zero > parameters. At the moment, the test function will nevertheless be called > with NULL as the parameter. Instead, don't try to run the test code, and > just mark the test as SKIPped. > > Reported-by: Daniel Latypov <dlatypov@xxxxxxxxxx> > Signed-off-by: David Gow <davidgow@xxxxxxxxxx> Reviewed-by: Daniel Latypov <dlatypov@xxxxxxxxxx> There's a small bug in this patch noted below, we just need to move the "# Subtest" change into the child patch. If/when we make that change, I have an optional suggestion about flipping the if/else branch. But other than that, this looks good to me. > --- > > Changes since v2: > https://lore.kernel.org/linux-kselftest/20211027013702.2039566-3-davidgow@xxxxxxxxxx/ > - Rework to not share the loop between the parameterised and > non-parameterised test cases. > - Suggested by Daniel Latypov. > - Avoids using a magic non-zero pointer value. > > lib/kunit/test.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 3bd741e50a2d..dfe1127aacfd 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -508,12 +508,12 @@ int kunit_run_tests(struct kunit_suite *suite) > /* Get initial param. */ > param_desc[0] = '\0'; > test.param_value = test_case->generate_params(NULL, param_desc); > - } > + kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT > + "# Subtest: %s", test_case->name); It looks like this change accidentally made its way into this patch as opposed to the child. This commit itself gives me a traffic light (red/yellow/green statuses): [ERROR] Test inode_test_xtimestamp_decoding: 0 tests run! ====== [NO TESTS RUN] inode_test_xtimestamp_decoding ======= ================= [PASSED] ext4_inode_test ================= ============================================================ The problem is the output becomes this: # Subtest: ext4_inode_test 1..1 # Subtest: inode_test_xtimestamp_decoding # inode_test_xtimestamp_decoding: ok 1 - 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits ... > > - do { > - kunit_run_case_catch_errors(suite, test_case, &test); > + while (test.param_value) { > + kunit_run_case_catch_errors(suite, test_case, &test); > > - if (test_case->generate_params) { > if (param_desc[0] == '\0') { > snprintf(param_desc, sizeof(param_desc), > "param-%d", test.param_index); > @@ -530,11 +530,15 @@ int kunit_run_tests(struct kunit_suite *suite) > param_desc[0] = '\0'; > test.param_value = test_case->generate_params(test.param_value, param_desc); > test.param_index++; > - } > > + kunit_update_stats(¶m_stats, test.status); > + } > + } else { I have a very slight preference for having the order of these branches swapped. i.e. if (!test_case->generate_params) { /* Non-parameterised test */ } else { ... } I prefer this because I think it's more readable for a few reasons: * I like having the "normal" branch come first. This is likely the code path a reader would care more about. * I prefer having the shorter branch come first. It makes it easier to read it through and see "oh, so this branch is just that one but with XYZ" > + /* Non-parameterised test. */ > + kunit_run_case_catch_errors(suite, test_case, &test); > kunit_update_stats(¶m_stats, test.status); > + } > > - } while (test.param_value); > > kunit_print_test_stats(&test, param_stats); > > -- > 2.33.0.1079.g6e70778dc9-goog >
