Hi David, Thanks for looking into this patch. My responses inline below:
David Gow <[email protected]> writes: <snip> > > Thanks — this is great! There are a few cases it's not handling > properly, though, particularly with respect to the debugfs support. > > I think we need to: > - Reset suite->status to KUNIT_SUCCESS in kunit_init_suite(), so that a > suite which is re-run via debugfs isn't automatically skipped again. > - Fix the result handling in debugfs to handle skipped suites. Agree, and I have addressed this in v2 of the patch using the code-snippets you have shared. > - (Optional) Maybe we could get rid of kunit_suite::suite_init_err now > that there's a specific status value. That'd have to be done carefully > to preserve all of the semantics, though. Yes fully agree that kunit_suite::suite_init_err will be redundant after introduction of kunit_suite::status. I have started looking into this and will post a different patch{series} for this. > > Here's a quick (and slightly hacky) patch to fix the first couple of issues: > --- > diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c > index 9c326f1837bd..23d34bfdba95 100644 > --- a/lib/kunit/debugfs.c > +++ b/lib/kunit/debugfs.c > @@ -58,6 +58,22 @@ static void debugfs_print_result(struct seq_file > *seq, struct string_stream *log > spin_unlock(&log->lock); > } > > +/* Print the result line for a suite. */ > +static void debugfs_print_ok_not_ok(struct seq_file *seq, > + enum kunit_status status, > + size_t test_number, > + const char *description, > + const char *directive) > +{ > + const char *directive_header = (status == KUNIT_SKIPPED) ? " # SKIP " > : ""; > + const char *directive_body = (status == KUNIT_SKIPPED) ? directive : ""; > + > + seq_printf(seq, "%s %zd %s%s%s\n", > + kunit_status_to_ok_not_ok(status), > + test_number, description, directive_header, > + directive_body); > +} > + > /* > * /sys/kernel/debug/kunit/<testsuite>/results shows all results for > testsuite. > */ > @@ -77,17 +93,17 @@ static int debugfs_print_results(struct seq_file > *seq, void *v) > seq_puts(seq, "1..1\n"); > > /* Print suite header because it is not stored in the test logs. */ > - seq_puts(seq, KUNIT_SUBTEST_INDENT "KTAP version 1\n"); > - seq_printf(seq, KUNIT_SUBTEST_INDENT "# Subtest: %s\n", suite->name); > - seq_printf(seq, KUNIT_SUBTEST_INDENT "1..%zd\n", > kunit_suite_num_test_cases(suite)); > - > - kunit_suite_for_each_test_case(suite, test_case) > - debugfs_print_result(seq, test_case->log); > + if (suite->status != KUNIT_SKIPPED) { > + seq_puts(seq, KUNIT_SUBTEST_INDENT "KTAP version 1\n"); > + seq_printf(seq, KUNIT_SUBTEST_INDENT "# Subtest: %s\n", > suite->name); > + seq_printf(seq, KUNIT_SUBTEST_INDENT "1..%zd\n", > kunit_suite_num_test_cases(suite)); > > + kunit_suite_for_each_test_case(suite, test_case) > + debugfs_print_result(seq, test_case->log); > + } > debugfs_print_result(seq, suite->log); > > - seq_printf(seq, "%s %d %s\n", > - kunit_status_to_ok_not_ok(success), 1, suite->name); > + debugfs_print_ok_not_ok(seq, success, 1, suite->name, > suite->status_comment); > return 0; > } Thanks, have modified these proposed changes a bit for v2 > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index c0ae45a22b2c..2ff145796450 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -798,9 +798,6 @@ int kunit_run_tests(struct kunit_suite *suite) > /* Taint the kernel so we know we've run tests. */ > add_taint(TAINT_TEST, LOCKDEP_STILL_OK); > > - if (suite->status == KUNIT_SKIPPED) > - goto suite_end; > - > if (suite->suite_init) { > suite->suite_init_err = suite->suite_init(suite); > if (suite->suite_init_err) { > @@ -836,6 +833,7 @@ static void kunit_init_suite(struct kunit_suite *suite) > kunit_debugfs_create_suite(suite); > suite->status_comment[0] = '\0'; > suite->suite_init_err = 0; > + suite->status = KUNIT_SUCCESS; Thanks, taken this change for v2. > > if (suite->log) > string_stream_clear(suite->log); > --- > > >> include/kunit/test.h | 1 + >> lib/kunit/test.c | 11 +++++++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/include/kunit/test.h b/include/kunit/test.h >> index ce0573e196ce..395221d623f7 100644 >> --- a/include/kunit/test.h >> +++ b/include/kunit/test.h >> @@ -285,6 +285,7 @@ struct kunit_suite { >> struct string_stream *log; >> int suite_init_err; >> bool is_init; >> + enum kunit_status status; >> }; >> >> /* Stores an array of suites, end points one past the end */ >> diff --git a/lib/kunit/test.c b/lib/kunit/test.c >> index 99773e000e1b..989acc770265 100644 >> --- a/lib/kunit/test.c >> +++ b/lib/kunit/test.c >> @@ -214,6 +214,9 @@ enum kunit_status kunit_suite_has_succeeded(struct >> kunit_suite *suite) >> const struct kunit_case *test_case; >> enum kunit_status status = KUNIT_SKIPPED; >> >> + if (suite->status == KUNIT_SKIPPED) >> + return KUNIT_SKIPPED; >> + >> if (suite->suite_init_err) >> return KUNIT_FAILURE; >> >> @@ -795,12 +798,20 @@ int kunit_run_tests(struct kunit_suite *suite) >> /* Taint the kernel so we know we've run tests. */ >> add_taint(TAINT_TEST, LOCKDEP_STILL_OK); >> >> + if (suite->status == KUNIT_SKIPPED) >> + goto suite_end; >> + > > Do we want this? If a test is run more than once, we probably want to > re-run suite_init so that we can tell if we should still skip it. This condition should prevent a 'kunit_suite' from running if its marked skipped before suite_init() is called. > > While I don't think it's likely that a test which was previously skipped > will suddenly become available, it's not impossible with, e.g., CPU hotplug. Agree, I have updated kunit_init_suite() to set 'kunit_suite.status' == KUNIT_SUCCESS. This should force the suite_init() to be calledback again when the suite is re-run. -- Cheers ~ Vaibhav
