On Sat, Nov 5, 2022 at 3:48 AM Rae Moar rmoar@google.com wrote:
Change KUnit test output to comply with KTAP version 1 specifications found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html.
- Use "KTAP version 1" instead of "TAP version 14" as test output header
- Remove '-' between test number and test name on test result lines
- Add KTAP version lines to each subtest header as well
Original output:
TAP version 14 1..1 # Subtest: kunit-test-suite 1..3 ok 1 - kunit_test_1 ok 2 - kunit_test_2 ok 3 - kunit_test_3 # kunit-test-suite: pass:3 fail:0 skip:0 total:3 # Totals: pass:3 fail:0 skip:0 total:3 ok 1 - kunit-test-suite
New output:
KTAP version 1 1..1 # Subtest: kunit-test-suite KTAP version 1 1..3 ok 1 kunit_test_1 ok 2 kunit_test_2 ok 3 kunit_test_3 # kunit-test-suite: pass:3 fail:0 skip:0 total:3 # Totals: pass:3 fail:0 skip:0 total:3 ok 1 kunit-test-suite
Signed-off-by: Rae Moar rmoar@google.com
Thanks very much for doing this. It's always been slightly embarrassing that we weren't perfectly following our own spec!
I think this series is good enough, but have a minor suggestion: could we swap the order of these two patches? I'd rather have the parser changes go in first.
I'd also be curious to see if this is likely to break anyone else's (K)TAP parsers.
+Isabella, Anders: do these changes break the IGT or LKFT TAP/KTAP parsing? From a quick look at [1] and [2], we're probably okay??
[1]: https://gitlab.freedesktop.org/isinyaaa/igt-gpu-tools/-/commit/1a84306425e97... [2]: https://github.com/Linaro/test-definitions/blob/master/automated/linux/kunit...
I also looked into the possibility of moving or removing the Subtest line, but upon further thought (see below), it's probably best to keep it as-is here for now. That should be the closest to the current spec, and we can possibly find a better way to provide the name in KTAPv2.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
lib/kunit/executor.c | 6 +++--- lib/kunit/test.c | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 9bbc422c284b..74982b83707c 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -166,7 +166,7 @@ static void kunit_exec_run_tests(struct suite_set *suite_set) { size_t num_suites = suite_set->end - suite_set->start;
pr_info("TAP version 14\n");
pr_info("KTAP version 1\n"); pr_info("1..%zu\n", num_suites); __kunit_test_suites_init(suite_set->start, num_suites);@@ -177,8 +177,8 @@ static void kunit_exec_list_tests(struct suite_set *suite_set) struct kunit_suite * const *suites; struct kunit_case *test_case;
/* Hack: print a tap header so kunit.py can find the start of KUnit output. */pr_info("TAP version 14\n");
/* Hack: print a ktap header so kunit.py can find the start of KUnit output. */pr_info("KTAP version 1\n"); for (suites = suite_set->start; suites < suite_set->end; suites++) kunit_suite_for_each_test_case((*suites), test_case) {diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 90640a43cf62..b541d59a05c3 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -151,6 +151,7 @@ static void kunit_print_suite_start(struct kunit_suite *suite) { kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s", suite->name);
kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "KTAP version 1\n");
Would it make sense to have the Subtest line _after_ the KTAP line here?
Given KTAP doesn't specify the "Subtest" line at all, I guess it doesn't matter.
A part of me feels that the "KTAP version 1" line should be the _first_ line of the subtest output, but that would introduce a line between it and the test plan, which goes against the spec.
We could also just get rid of the "Subtest" line, given it's not technically required, though having the test name before the results is really useful.
Having tried all three options while writing this email, I think it's probably best to leave this patch as is (with the Subtest line followed by the KTAP line), and discuss standardising something similar as part of the KTAP v2 spec.
kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "1..%zd", kunit_suite_num_test_cases(suite));} @@ -175,13 +176,13 @@ static void kunit_print_ok_not_ok(void *test_or_suite, * representation. */ if (suite)
pr_info("%s %zd - %s%s%s\n",
pr_info("%s %zd %s%s%s\n", kunit_status_to_ok_not_ok(status), test_number, description, directive_header, (status == KUNIT_SKIPPED) ? directive : ""); else kunit_log(KERN_INFO, test,
KUNIT_SUBTEST_INDENT "%s %zd - %s%s%s",
KUNIT_SUBTEST_INDENT "%s %zd %s%s%s", kunit_status_to_ok_not_ok(status), test_number, description, directive_header, (status == KUNIT_SKIPPED) ? directive : "");base-commit: 6fe1ad4a156095859721fef85073df3ed43081d4
2.38.1.431.g37b22c650d-goog