Change KUnit test output to comply with KTAP version 1 specifications found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html. 1) Use "KTAP version 1" instead of "TAP version 14" as test output header 2) Remove '-' between test number and test name on test result lines 2) 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 --- 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"); 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
Change the KUnit parser to be able to parse test output that complies with the KTAP version 1 specification format found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html. Ensure the parser is able to parse tests with the original KUnit test output format as well.
KUnit parser now accepts any of the following test output formats:
Original KUnit test output format:
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
KTAP version 1 test output format:
KTAP version 1 1..1 KTAP version 1 1..3 ok 1 kunit_test_1 ok 2 kunit_test_2 ok 3 kunit_test_3 ok 1 kunit-test-suite
New KUnit test output format (preferred for KUnit tests):
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 --- Note: this patch is based on the linux-kselftest/kunit branch. --- tools/testing/kunit/kunit_parser.py | 69 ++++++++++++------- tools/testing/kunit/kunit_tool_test.py | 8 +++ .../test_data/test_parse_ktap_output.log | 8 +++ 3 files changed, 60 insertions(+), 25 deletions(-) create mode 100644 tools/testing/kunit/test_data/test_parse_ktap_output.log
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py index a56c75a973b5..abb69f898263 100644 --- a/tools/testing/kunit/kunit_parser.py +++ b/tools/testing/kunit/kunit_parser.py @@ -441,6 +441,7 @@ def parse_diagnostic(lines: LineStream) -> List[str]: - '# Subtest: [test name]' - '[ok|not ok] [test number] [-] [test name] [optional skip directive]' + - 'KTAP version [version number]'
Parameters: lines - LineStream of KTAP output to parse @@ -449,8 +450,9 @@ def parse_diagnostic(lines: LineStream) -> List[str]: Log of diagnostic lines """ log = [] # type: List[str] - while lines and not TEST_RESULT.match(lines.peek()) and not \ - TEST_HEADER.match(lines.peek()): + non_diagnostic_lines = [TEST_RESULT, TEST_HEADER, KTAP_START] + while lines and not any(re.match(lines.peek()) + for re in non_diagnostic_lines): log.append(lines.pop()) return log
@@ -496,6 +498,12 @@ def print_test_header(test: Test) -> None: test - Test object representing current test being printed """ message = test.name + if message == "": + # KUnit tests print a Subtest header line that provides the name + # of the test suite. But the subtest header line isn't required + # by the KTAP spec, so use a placeholder name "Test suite" in that + # case. + message = "Test suite" if test.expected_count: if test.expected_count == 1: message += ' (1 subtest)' @@ -647,13 +655,13 @@ def bubble_up_test_results(test: Test) -> None: elif test.counts.get_status() == TestStatus.TEST_CRASHED: test.status = TestStatus.TEST_CRASHED
-def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: +def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest: bool) -> Test: """ Finds next test to parse in LineStream, creates new Test object, parses any subtests of the test, populates Test object with all information (status, name) about the test and the Test objects for any subtests, and then returns the Test object. The method accepts - three formats of tests: + four formats of tests:
Accepted test formats:
@@ -674,6 +682,16 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: [subtests] ok 1 name
+ - KTAP subtest header (in compliance with KTAP specification) + + Example: + + # May include subtest header line here + KTAP version 1 + 1..3 + [subtests] + ok 1 name + - Test result line
Example: @@ -685,6 +703,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: expected_num - expected test number for test to be parsed log - list of strings containing any preceding diagnostic lines corresponding to the current test + is_subtest - boolean indicating whether test is a subtest
Return: Test object populated with characteristics and any subtests @@ -692,21 +711,22 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: test = Test() test.log.extend(log) parent_test = False - main = parse_ktap_header(lines, test) - if main: - # If KTAP/TAP header is found, attempt to parse - # test plan + if not is_subtest: + # If parsing the main test, attempt to parse KTAP/TAP header + # and test plan test.name = "main" + parse_ktap_header(lines, test) parse_test_plan(lines, test) parent_test = True else: - # If KTAP/TAP header is not found, test must be subtest - # header or test result line so parse attempt to parser - # subtest header - parent_test = parse_test_header(lines, test) + # If test is a subtest, attempt to parse test suite header + # (either subtest line and/or KTAP/TAP version line) + subtest_line = parse_test_header(lines, test) + ktap_line = parse_ktap_header(lines, test) + parent_test = subtest_line or ktap_line if parent_test: - # If subtest header is found, attempt to parse - # test plan and print header + # If subtest header and/or KTAP/version line is found, attempt + # to parse test plan and print header parse_test_plan(lines, test) print_test_header(test) expected_count = test.expected_count @@ -721,7 +741,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: sub_log = parse_diagnostic(lines) sub_test = Test() if not lines or (peek_test_name_match(lines, test) and - not main): + is_subtest): if expected_count and test_num <= expected_count: # If parser reaches end of test before # parsing expected number of subtests, print @@ -735,20 +755,19 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: test.log.extend(sub_log) break else: - sub_test = parse_test(lines, test_num, sub_log) + sub_test = parse_test(lines, test_num, sub_log, True) subtests.append(sub_test) test_num += 1 test.subtests = subtests - if not main: + if is_subtest: # If not main test, look for test result line test.log.extend(parse_diagnostic(lines)) - if (parent_test and peek_test_name_match(lines, test)) or \ - not parent_test: - parse_test_result(lines, test, expected_num) - else: + if subtest_line and not peek_test_name_match(lines, test): test.add_error('missing subtest result line!') + else: + parse_test_result(lines, test, expected_num)
- # Check for there being no tests + # Check for there being no subtests within parent test if parent_test and len(subtests) == 0: # Don't override a bad status if this test had one reported. # Assumption: no subtests means CRASHED is from Test.__init__() @@ -758,11 +777,11 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test:
# Add statuses to TestCounts attribute in Test object bubble_up_test_results(test) - if parent_test and not main: + if parent_test and is_subtest: # If test has subtests and is not the main test object, print # footer. print_test_footer(test) - elif not main: + elif is_subtest: print_test_result(test) return test
@@ -785,7 +804,7 @@ def parse_run_tests(kernel_output: Iterable[str]) -> Test: test.add_error('could not find any KTAP output!') test.status = TestStatus.FAILURE_TO_PARSE_TESTS else: - test = parse_test(lines, 0, []) + test = parse_test(lines, 0, [], False) if test.status != TestStatus.NO_TESTS: test.status = test.counts.get_status() stdout.print_with_timestamp(DIVIDER) diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 90c65b072be9..7c2e2a45f330 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -312,6 +312,14 @@ class KUnitParserTest(unittest.TestCase): self.assertEqual(kunit_parser._summarize_failed_tests(result), 'Failures: all_failed_suite, some_failed_suite.test2')
+ def test_ktap_format(self): + ktap_log = test_data_path('test_parse_ktap_output.log') + with open(ktap_log) as file: + result = kunit_parser.parse_run_tests(file.readlines()) + self.assertEqual(result.counts, kunit_parser.TestCounts(passed=3)) + self.assertEqual('suite', result.subtests[0].name) + self.assertEqual('case_1', result.subtests[0].subtests[0].name) + self.assertEqual('case_2', result.subtests[0].subtests[1].name)
def line_stream_from_strs(strs: Iterable[str]) -> kunit_parser.LineStream: return kunit_parser.LineStream(enumerate(strs, start=1)) diff --git a/tools/testing/kunit/test_data/test_parse_ktap_output.log b/tools/testing/kunit/test_data/test_parse_ktap_output.log new file mode 100644 index 000000000000..ccdf244e5303 --- /dev/null +++ b/tools/testing/kunit/test_data/test_parse_ktap_output.log @@ -0,0 +1,8 @@ +KTAP version 1 +1..1 + KTAP version 1 + 1..3 + ok 1 case_1 + ok 2 case_2 + ok 3 case_3 +ok 1 suite
On Fri, Nov 4, 2022 at 12:48 PM Rae Moar rmoar@google.com wrote:
Change the KUnit parser to be able to parse test output that complies with the KTAP version 1 specification format found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html. Ensure the parser is able to parse tests with the original KUnit test output format as well.
KUnit parser now accepts any of the following test output formats:
Original KUnit test output format:
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
KTAP version 1 test output format:
KTAP version 1 1..1 KTAP version 1 1..3 ok 1 kunit_test_1 ok 2 kunit_test_2 ok 3 kunit_test_3 ok 1 kunit-test-suite
New KUnit test output format (preferred for KUnit tests):
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
Reviewed-by: Daniel Latypov dlatypov@google.com
Looks good to me. Some comments below, but nothing we have to address in this patch, IMO.
Note: this patch is based on the linux-kselftest/kunit branch.
tools/testing/kunit/kunit_parser.py | 69 ++++++++++++------- tools/testing/kunit/kunit_tool_test.py | 8 +++ .../test_data/test_parse_ktap_output.log | 8 +++ 3 files changed, 60 insertions(+), 25 deletions(-) create mode 100644 tools/testing/kunit/test_data/test_parse_ktap_output.log
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py index a56c75a973b5..abb69f898263 100644 --- a/tools/testing/kunit/kunit_parser.py +++ b/tools/testing/kunit/kunit_parser.py @@ -441,6 +441,7 @@ def parse_diagnostic(lines: LineStream) -> List[str]: - '# Subtest: [test name]' - '[ok|not ok] [test number] [-] [test name] [optional skip directive]'
- 'KTAP version [version number]' Parameters: lines - LineStream of KTAP output to parse
@@ -449,8 +450,9 @@ def parse_diagnostic(lines: LineStream) -> List[str]: Log of diagnostic lines """ log = [] # type: List[str]
while lines and not TEST_RESULT.match(lines.peek()) and not \
TEST_HEADER.match(lines.peek()):
non_diagnostic_lines = [TEST_RESULT, TEST_HEADER, KTAP_START]
while lines and not any(re.match(lines.peek())
for re in non_diagnostic_lines): log.append(lines.pop()) return log
@@ -496,6 +498,12 @@ def print_test_header(test: Test) -> None: test - Test object representing current test being printed """ message = test.name
if message == "":
# KUnit tests print a Subtest header line that provides the name
# of the test suite. But the subtest header line isn't required
# by the KTAP spec, so use a placeholder name "Test suite" in that
# case.
message = "Test suite"
(something we can address in a later change)
Hmm, consider the following input
KTAP version 1 1..1 KTAP version 1 1..1 KTAP version 1 1..1 ok 1 - subtest1 ok 1 - test1 ok 1 - suite
$ ./tools/testing/kunit/kunit.py parse < /tmp/example_nested_ktap ============================================================ ================== Test suite (1 subtest) ================== ================== Test suite (1 subtest) ================== [PASSED] subtest1 ====================== [PASSED] test1 ====================== ====================== [PASSED] suite ====================== ============================================================
I wonder if the duplicate "Test suite" line would be confusing. This also points to a slightly bigger problem that kunit_parser.py doesn't have a good way to format 3+ layers of tests atm.
I don't know if there's another placeholder name we can give that might be less confusing.
if test.expected_count: if test.expected_count == 1: message += ' (1 subtest)'
@@ -647,13 +655,13 @@ def bubble_up_test_results(test: Test) -> None: elif test.counts.get_status() == TestStatus.TEST_CRASHED: test.status = TestStatus.TEST_CRASHED
-def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: +def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest: bool) -> Test: """ Finds next test to parse in LineStream, creates new Test object, parses any subtests of the test, populates Test object with all information (status, name) about the test and the Test objects for any subtests, and then returns the Test object. The method accepts
three formats of tests:
four formats of tests: Accepted test formats:
@@ -674,6 +682,16 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: [subtests] ok 1 name
- KTAP subtest header (in compliance with KTAP specification)
Example:
# May include subtest header line here
KTAP version 1
1..3
[subtests]
ok 1 name
- Test result line Example:
@@ -685,6 +703,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: expected_num - expected test number for test to be parsed log - list of strings containing any preceding diagnostic lines corresponding to the current test
is_subtest - boolean indicating whether test is a subtest Return: Test object populated with characteristics and any subtests
@@ -692,21 +711,22 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: test = Test() test.log.extend(log) parent_test = False
main = parse_ktap_header(lines, test)
if main:
# If KTAP/TAP header is found, attempt to parse
# test plan
if not is_subtest:
# If parsing the main test, attempt to parse KTAP/TAP header
# and test plan test.name = "main"
parse_ktap_header(lines, test) parse_test_plan(lines, test) parent_test = True else:
# If KTAP/TAP header is not found, test must be subtest
# header or test result line so parse attempt to parser
# subtest header
parent_test = parse_test_header(lines, test)
# If test is a subtest, attempt to parse test suite header
# (either subtest line and/or KTAP/TAP version line)
subtest_line = parse_test_header(lines, test)
ktap_line = parse_ktap_header(lines, test)
parent_test = subtest_line or ktap_line if parent_test:
# If subtest header is found, attempt to parse
# test plan and print header
# If subtest header and/or KTAP/version line is found, attempt
# to parse test plan and print header parse_test_plan(lines, test) print_test_header(test) expected_count = test.expected_count
@@ -721,7 +741,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: sub_log = parse_diagnostic(lines) sub_test = Test() if not lines or (peek_test_name_match(lines, test) and
not main):
is_subtest): if expected_count and test_num <= expected_count: # If parser reaches end of test before # parsing expected number of subtests, print
@@ -735,20 +755,19 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: test.log.extend(sub_log) break else:
sub_test = parse_test(lines, test_num, sub_log)
sub_test = parse_test(lines, test_num, sub_log, True) subtests.append(sub_test) test_num += 1 test.subtests = subtests
if not main:
if is_subtest: # If not main test, look for test result line test.log.extend(parse_diagnostic(lines))
if (parent_test and peek_test_name_match(lines, test)) or \
not parent_test:
parse_test_result(lines, test, expected_num)
else:
if subtest_line and not peek_test_name_match(lines, test): test.add_error('missing subtest result line!')
else:
parse_test_result(lines, test, expected_num)
This change isn't a straightforward change of the logic like s/main/not is_subtest. But looking at it, it seems fine.
One example input would be
KTAP version 1 1..2 # Subtest: suite1 KTAP version 1 1..1 ok 1 test1 # ok 1 suite1 ok 2 suite2
We get output like this
$ ./tools/testing/kunit/kunit.py parse < /tmp/out [14:54:44] ============================================================ [14:54:44] ==================== suite1 (1 subtest) ==================== [14:54:44] [PASSED] test1 [14:54:44] [ERROR] Test: suite1: missing subtest result line! [14:54:44] # Subtest: suite1 [14:54:44] KTAP version 1 [14:54:44] 1..1 [14:54:44] # ok 1 suite1 [14:54:44] ===================== [CRASHED] suite1 ===================== [14:54:44] [PASSED] suite2 [14:54:44] ============================================================
So it handles it about as well as we could expect.
Note: kunit.py is indeed saying the kernel crashed even though there's "kernel output" after the missing line. But this is a pre-existing condition. It already doesn't check to see that the output is truncated before saying "CRASHED"
On Sat, Nov 5, 2022 at 3:48 AM Rae Moar rmoar@google.com wrote:
Change the KUnit parser to be able to parse test output that complies with the KTAP version 1 specification format found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html. Ensure the parser is able to parse tests with the original KUnit test output format as well.
KUnit parser now accepts any of the following test output formats:
Original KUnit test output format:
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
KTAP version 1 test output format:
KTAP version 1 1..1 KTAP version 1 1..3 ok 1 kunit_test_1 ok 2 kunit_test_2 ok 3 kunit_test_3 ok 1 kunit-test-suite
New KUnit test output format (preferred for KUnit tests):
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
Note: this patch is based on the linux-kselftest/kunit branch.
Looks good to me. Some minor thoughts: - As Daniel mentioned, can we think of a better placeholder name for tests without Subtest lines? One thought is to just leave it as the empty string? - Would it make sense to support the case where the "Subtest" line sits between the KTAP version line and the test plan as well. While that's not necessary (and does violate v1 of the KTAP spec), I suspect something similar would be useful in KTAP v2 for, e.g., individual module results. - As mentioned in patch 1, it'd be nice to swap the ordering of the two patches.
None of those are showstoppers, so if you disagree, we can probably accept them as-is, but they might make future changes easier.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
tools/testing/kunit/kunit_parser.py | 69 ++++++++++++------- tools/testing/kunit/kunit_tool_test.py | 8 +++ .../test_data/test_parse_ktap_output.log | 8 +++ 3 files changed, 60 insertions(+), 25 deletions(-) create mode 100644 tools/testing/kunit/test_data/test_parse_ktap_output.log
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py index a56c75a973b5..abb69f898263 100644 --- a/tools/testing/kunit/kunit_parser.py +++ b/tools/testing/kunit/kunit_parser.py @@ -441,6 +441,7 @@ def parse_diagnostic(lines: LineStream) -> List[str]: - '# Subtest: [test name]' - '[ok|not ok] [test number] [-] [test name] [optional skip directive]'
- 'KTAP version [version number]' Parameters: lines - LineStream of KTAP output to parse
@@ -449,8 +450,9 @@ def parse_diagnostic(lines: LineStream) -> List[str]: Log of diagnostic lines """ log = [] # type: List[str]
while lines and not TEST_RESULT.match(lines.peek()) and not \
TEST_HEADER.match(lines.peek()):
non_diagnostic_lines = [TEST_RESULT, TEST_HEADER, KTAP_START]
while lines and not any(re.match(lines.peek())
for re in non_diagnostic_lines): log.append(lines.pop()) return log
@@ -496,6 +498,12 @@ def print_test_header(test: Test) -> None: test - Test object representing current test being printed """ message = test.name
if message == "":
# KUnit tests print a Subtest header line that provides the name
# of the test suite. But the subtest header line isn't required
# by the KTAP spec, so use a placeholder name "Test suite" in that
# case.
message = "Test suite" if test.expected_count: if test.expected_count == 1: message += ' (1 subtest)'
@@ -647,13 +655,13 @@ def bubble_up_test_results(test: Test) -> None: elif test.counts.get_status() == TestStatus.TEST_CRASHED: test.status = TestStatus.TEST_CRASHED
-def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: +def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest: bool) -> Test: """ Finds next test to parse in LineStream, creates new Test object, parses any subtests of the test, populates Test object with all information (status, name) about the test and the Test objects for any subtests, and then returns the Test object. The method accepts
three formats of tests:
four formats of tests: Accepted test formats:
@@ -674,6 +682,16 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: [subtests] ok 1 name
- KTAP subtest header (in compliance with KTAP specification)
Example:
# May include subtest header line here
KTAP version 1
1..3
[subtests]
ok 1 name
- Test result line Example:
@@ -685,6 +703,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: expected_num - expected test number for test to be parsed log - list of strings containing any preceding diagnostic lines corresponding to the current test
is_subtest - boolean indicating whether test is a subtest Return: Test object populated with characteristics and any subtests
@@ -692,21 +711,22 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: test = Test() test.log.extend(log) parent_test = False
main = parse_ktap_header(lines, test)
if main:
# If KTAP/TAP header is found, attempt to parse
# test plan
if not is_subtest:
# If parsing the main test, attempt to parse KTAP/TAP header
# and test plan test.name = "main"
parse_ktap_header(lines, test) parse_test_plan(lines, test) parent_test = True else:
# If KTAP/TAP header is not found, test must be subtest
# header or test result line so parse attempt to parser
# subtest header
parent_test = parse_test_header(lines, test)
# If test is a subtest, attempt to parse test suite header
# (either subtest line and/or KTAP/TAP version line)
subtest_line = parse_test_header(lines, test)
ktap_line = parse_ktap_header(lines, test)
parent_test = subtest_line or ktap_line if parent_test:
# If subtest header is found, attempt to parse
# test plan and print header
# If subtest header and/or KTAP/version line is found, attempt
# to parse test plan and print header parse_test_plan(lines, test) print_test_header(test) expected_count = test.expected_count
@@ -721,7 +741,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: sub_log = parse_diagnostic(lines) sub_test = Test() if not lines or (peek_test_name_match(lines, test) and
not main):
is_subtest): if expected_count and test_num <= expected_count: # If parser reaches end of test before # parsing expected number of subtests, print
@@ -735,20 +755,19 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: test.log.extend(sub_log) break else:
sub_test = parse_test(lines, test_num, sub_log)
sub_test = parse_test(lines, test_num, sub_log, True) subtests.append(sub_test) test_num += 1 test.subtests = subtests
if not main:
if is_subtest: # If not main test, look for test result line test.log.extend(parse_diagnostic(lines))
if (parent_test and peek_test_name_match(lines, test)) or \
not parent_test:
parse_test_result(lines, test, expected_num)
else:
if subtest_line and not peek_test_name_match(lines, test): test.add_error('missing subtest result line!')
else:
parse_test_result(lines, test, expected_num)
# Check for there being no tests
# Check for there being no subtests within parent test if parent_test and len(subtests) == 0: # Don't override a bad status if this test had one reported. # Assumption: no subtests means CRASHED is from Test.__init__()
@@ -758,11 +777,11 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test:
# Add statuses to TestCounts attribute in Test object bubble_up_test_results(test)
if parent_test and not main:
if parent_test and is_subtest: # If test has subtests and is not the main test object, print # footer. print_test_footer(test)
elif not main:
elif is_subtest: print_test_result(test) return test
@@ -785,7 +804,7 @@ def parse_run_tests(kernel_output: Iterable[str]) -> Test: test.add_error('could not find any KTAP output!') test.status = TestStatus.FAILURE_TO_PARSE_TESTS else:
test = parse_test(lines, 0, [])
test = parse_test(lines, 0, [], False) if test.status != TestStatus.NO_TESTS: test.status = test.counts.get_status() stdout.print_with_timestamp(DIVIDER)
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 90c65b072be9..7c2e2a45f330 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -312,6 +312,14 @@ class KUnitParserTest(unittest.TestCase): self.assertEqual(kunit_parser._summarize_failed_tests(result), 'Failures: all_failed_suite, some_failed_suite.test2')
def test_ktap_format(self):
ktap_log = test_data_path('test_parse_ktap_output.log')
with open(ktap_log) as file:
result = kunit_parser.parse_run_tests(file.readlines())
self.assertEqual(result.counts, kunit_parser.TestCounts(passed=3))
self.assertEqual('suite', result.subtests[0].name)
self.assertEqual('case_1', result.subtests[0].subtests[0].name)
self.assertEqual('case_2', result.subtests[0].subtests[1].name)
def line_stream_from_strs(strs: Iterable[str]) -> kunit_parser.LineStream: return kunit_parser.LineStream(enumerate(strs, start=1)) diff --git a/tools/testing/kunit/test_data/test_parse_ktap_output.log b/tools/testing/kunit/test_data/test_parse_ktap_output.log new file mode 100644 index 000000000000..ccdf244e5303 --- /dev/null +++ b/tools/testing/kunit/test_data/test_parse_ktap_output.log @@ -0,0 +1,8 @@ +KTAP version 1 +1..1
- KTAP version 1
- 1..3
- ok 1 case_1
- ok 2 case_2
- ok 3 case_3
+ok 1 suite
2.38.1.431.g37b22c650d-goog
On Tue, Nov 15, 2022 at 2:27 AM David Gow davidgow@google.com wrote:
On Sat, Nov 5, 2022 at 3:48 AM Rae Moar rmoar@google.com wrote:
Change the KUnit parser to be able to parse test output that complies with the KTAP version 1 specification format found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html. Ensure the parser is able to parse tests with the original KUnit test output format as well.
KUnit parser now accepts any of the following test output formats:
Original KUnit test output format:
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
KTAP version 1 test output format:
KTAP version 1 1..1 KTAP version 1 1..3 ok 1 kunit_test_1 ok 2 kunit_test_2 ok 3 kunit_test_3 ok 1 kunit-test-suite
New KUnit test output format (preferred for KUnit tests):
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
Note: this patch is based on the linux-kselftest/kunit branch.
Looks good to me. Some minor thoughts:
- As Daniel mentioned, can we think of a better placeholder name for
tests without Subtest lines? One thought is to just leave it as the empty string?
I am definitely open to changing this placeholder name.
The ideas I thought of are: "Test suite", just "Test", or just an empty string. "Test" or empty string may be less confusing. What do people prefer?
- Would it make sense to support the case where the "Subtest" line
sits between the KTAP version line and the test plan as well. While that's not necessary (and does violate v1 of the KTAP spec), I suspect something similar would be useful in KTAP v2 for, e.g., individual module results.
Similar to the comments on the first patch, I personally think we could make those changes later in combination with the KTAP v2 development.
- As mentioned in patch 1, it'd be nice to swap the ordering of the two patches.
Yes, definitely a great idea. Will make a v2 with the patches swapped.
None of those are showstoppers, so if you disagree, we can probably accept them as-is, but they might make future changes easier.
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
tools/testing/kunit/kunit_parser.py | 69 ++++++++++++------- tools/testing/kunit/kunit_tool_test.py | 8 +++ .../test_data/test_parse_ktap_output.log | 8 +++ 3 files changed, 60 insertions(+), 25 deletions(-) create mode 100644 tools/testing/kunit/test_data/test_parse_ktap_output.log
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py index a56c75a973b5..abb69f898263 100644 --- a/tools/testing/kunit/kunit_parser.py +++ b/tools/testing/kunit/kunit_parser.py @@ -441,6 +441,7 @@ def parse_diagnostic(lines: LineStream) -> List[str]: - '# Subtest: [test name]' - '[ok|not ok] [test number] [-] [test name] [optional skip directive]'
- 'KTAP version [version number]' Parameters: lines - LineStream of KTAP output to parse
@@ -449,8 +450,9 @@ def parse_diagnostic(lines: LineStream) -> List[str]: Log of diagnostic lines """ log = [] # type: List[str]
while lines and not TEST_RESULT.match(lines.peek()) and not \
TEST_HEADER.match(lines.peek()):
non_diagnostic_lines = [TEST_RESULT, TEST_HEADER, KTAP_START]
while lines and not any(re.match(lines.peek())
for re in non_diagnostic_lines): log.append(lines.pop()) return log
@@ -496,6 +498,12 @@ def print_test_header(test: Test) -> None: test - Test object representing current test being printed """ message = test.name
if message == "":
# KUnit tests print a Subtest header line that provides the name
# of the test suite. But the subtest header line isn't required
# by the KTAP spec, so use a placeholder name "Test suite" in that
# case.
message = "Test suite" if test.expected_count: if test.expected_count == 1: message += ' (1 subtest)'
@@ -647,13 +655,13 @@ def bubble_up_test_results(test: Test) -> None: elif test.counts.get_status() == TestStatus.TEST_CRASHED: test.status = TestStatus.TEST_CRASHED
-def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: +def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest: bool) -> Test: """ Finds next test to parse in LineStream, creates new Test object, parses any subtests of the test, populates Test object with all information (status, name) about the test and the Test objects for any subtests, and then returns the Test object. The method accepts
three formats of tests:
four formats of tests: Accepted test formats:
@@ -674,6 +682,16 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: [subtests] ok 1 name
- KTAP subtest header (in compliance with KTAP specification)
Example:
# May include subtest header line here
KTAP version 1
1..3
[subtests]
ok 1 name
- Test result line Example:
@@ -685,6 +703,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: expected_num - expected test number for test to be parsed log - list of strings containing any preceding diagnostic lines corresponding to the current test
is_subtest - boolean indicating whether test is a subtest Return: Test object populated with characteristics and any subtests
@@ -692,21 +711,22 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: test = Test() test.log.extend(log) parent_test = False
main = parse_ktap_header(lines, test)
if main:
# If KTAP/TAP header is found, attempt to parse
# test plan
if not is_subtest:
# If parsing the main test, attempt to parse KTAP/TAP header
# and test plan test.name = "main"
parse_ktap_header(lines, test) parse_test_plan(lines, test) parent_test = True else:
# If KTAP/TAP header is not found, test must be subtest
# header or test result line so parse attempt to parser
# subtest header
parent_test = parse_test_header(lines, test)
# If test is a subtest, attempt to parse test suite header
# (either subtest line and/or KTAP/TAP version line)
subtest_line = parse_test_header(lines, test)
ktap_line = parse_ktap_header(lines, test)
parent_test = subtest_line or ktap_line if parent_test:
# If subtest header is found, attempt to parse
# test plan and print header
# If subtest header and/or KTAP/version line is found, attempt
# to parse test plan and print header parse_test_plan(lines, test) print_test_header(test) expected_count = test.expected_count
@@ -721,7 +741,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: sub_log = parse_diagnostic(lines) sub_test = Test() if not lines or (peek_test_name_match(lines, test) and
not main):
is_subtest): if expected_count and test_num <= expected_count: # If parser reaches end of test before # parsing expected number of subtests, print
@@ -735,20 +755,19 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: test.log.extend(sub_log) break else:
sub_test = parse_test(lines, test_num, sub_log)
sub_test = parse_test(lines, test_num, sub_log, True) subtests.append(sub_test) test_num += 1 test.subtests = subtests
if not main:
if is_subtest: # If not main test, look for test result line test.log.extend(parse_diagnostic(lines))
if (parent_test and peek_test_name_match(lines, test)) or \
not parent_test:
parse_test_result(lines, test, expected_num)
else:
if subtest_line and not peek_test_name_match(lines, test): test.add_error('missing subtest result line!')
else:
parse_test_result(lines, test, expected_num)
# Check for there being no tests
# Check for there being no subtests within parent test if parent_test and len(subtests) == 0: # Don't override a bad status if this test had one reported. # Assumption: no subtests means CRASHED is from Test.__init__()
@@ -758,11 +777,11 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test:
# Add statuses to TestCounts attribute in Test object bubble_up_test_results(test)
if parent_test and not main:
if parent_test and is_subtest: # If test has subtests and is not the main test object, print # footer. print_test_footer(test)
elif not main:
elif is_subtest: print_test_result(test) return test
@@ -785,7 +804,7 @@ def parse_run_tests(kernel_output: Iterable[str]) -> Test: test.add_error('could not find any KTAP output!') test.status = TestStatus.FAILURE_TO_PARSE_TESTS else:
test = parse_test(lines, 0, [])
test = parse_test(lines, 0, [], False) if test.status != TestStatus.NO_TESTS: test.status = test.counts.get_status() stdout.print_with_timestamp(DIVIDER)
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 90c65b072be9..7c2e2a45f330 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -312,6 +312,14 @@ class KUnitParserTest(unittest.TestCase): self.assertEqual(kunit_parser._summarize_failed_tests(result), 'Failures: all_failed_suite, some_failed_suite.test2')
def test_ktap_format(self):
ktap_log = test_data_path('test_parse_ktap_output.log')
with open(ktap_log) as file:
result = kunit_parser.parse_run_tests(file.readlines())
self.assertEqual(result.counts, kunit_parser.TestCounts(passed=3))
self.assertEqual('suite', result.subtests[0].name)
self.assertEqual('case_1', result.subtests[0].subtests[0].name)
self.assertEqual('case_2', result.subtests[0].subtests[1].name)
def line_stream_from_strs(strs: Iterable[str]) -> kunit_parser.LineStream: return kunit_parser.LineStream(enumerate(strs, start=1)) diff --git a/tools/testing/kunit/test_data/test_parse_ktap_output.log b/tools/testing/kunit/test_data/test_parse_ktap_output.log new file mode 100644 index 000000000000..ccdf244e5303 --- /dev/null +++ b/tools/testing/kunit/test_data/test_parse_ktap_output.log @@ -0,0 +1,8 @@ +KTAP version 1 +1..1
- KTAP version 1
- 1..3
- ok 1 case_1
- ok 2 case_2
- ok 3 case_3
+ok 1 suite
2.38.1.431.g37b22c650d-goog
On Tue, Nov 15, 2022 at 12:46 PM Rae Moar rmoar@google.com wrote:
- As Daniel mentioned, can we think of a better placeholder name for
tests without Subtest lines? One thought is to just leave it as the empty string?
I am definitely open to changing this placeholder name.
The ideas I thought of are: "Test suite", just "Test", or just an empty string. "Test" or empty string may be less confusing. What do people prefer?
I'd prefer the empty string.
So it would show up as something like ===== (1 subtests) ===== [PASSED] case1 ====== suite1 ======
Note: we'll just have to make sure to avoid a leading space (e.g. we're currently doing message += f' (1 subtest)' )
Daniel
On Tue, Nov 15, 2022 at 5:02 PM Daniel Latypov dlatypov@google.com wrote:
On Tue, Nov 15, 2022 at 12:46 PM Rae Moar rmoar@google.com wrote:
- As Daniel mentioned, can we think of a better placeholder name for
tests without Subtest lines? One thought is to just leave it as the empty string?
I am definitely open to changing this placeholder name.
The ideas I thought of are: "Test suite", just "Test", or just an empty string. "Test" or empty string may be less confusing. What do people prefer?
I'd prefer the empty string.
So it would show up as something like ===== (1 subtests) ===== [PASSED] case1 ====== suite1 ======
Note: we'll just have to make sure to avoid a leading space (e.g. we're currently doing message += f' (1 subtest)' )
Daniel
This was discussed off the mailing list and seems like there was agreement that the empty string would be the best. Just wanted to update here. Will be changing this in v2.
On Fri, Nov 4, 2022 at 12:48 PM 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
Reviewed-by: Daniel Latypov dlatypov@google.com
I had been worried initially that this needed kunit_parser.py changes to work. But it doesn't, this change can go in now with minimal side effects.
More details below: This code treats the new "KTAP version 1" subheaders as random kernel output, so it puts it into the `test.log`
E.g. if I change to `not ok 1 kunit_test_1`, it'll print KTAP version 1 1..3 not ok 1 kunit_test_1
After the next patch, it just prints not ok 1 kunit_test_1
This bit of extra output on failure is something we can live with, esp. since it gets addressed by the next patch.
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
On Tue, Nov 15, 2022 at 2:27 AM David Gow davidgow@google.com wrote:
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.
Hi David! Yes, I agree. I think it does make more sense to provide KTAP compatibility with the parser before changing the test output. This would also help to solve the issue that Daniel brought up on this patch about the "KTAP version 1" line and test plan being stored in the test.log as random kernel output. I will swap the patches in the v2 of this patch series.
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??
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.
I also struggle to decide how the "Subtest" line should be handled. Since the current KTAP v1 spec does not provide a way to declare the name of a test with subtests prior to the results, I think it is important to continue to include this Subtest line because it provides that functionality. Additionally, the line is not expected to be very disruptive for other parsers because it is read as a diagnostic line.
The location of the "Subtest" line before the KTAP version line is potentially not the most optimal but it seems to be the best choice while ensuring compatibility with the current KTAP v1 spec. I recommend that we discuss a standardized replacement for this "Subtest" line in 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
On Tue, Nov 15, 2022 at 12:18 PM Rae Moar rmoar@google.com wrote:
Yes, I agree. I think it does make more sense to provide KTAP compatibility with the parser before changing the test output. This would also help to solve the issue that Daniel brought up on this patch about the "KTAP version 1" line and test plan being stored in the test.log as random kernel output. I will swap the patches in the v2 of this patch series.
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??
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.
I also struggle to decide how the "Subtest" line should be handled. Since the current KTAP v1 spec does not provide a way to declare the name of a test with subtests prior to the results, I think it is important to continue to include this Subtest line because it provides that functionality. Additionally, the line is not expected to be very disruptive for other parsers because it is read as a diagnostic line.
Yeah, since it's going to be ignored as a diagnostic line, I think we're largely free to put it where we want?
I'm actually leaning towards making things more uniform e.g.
KTAP version 1 # Subtest: optionally set for the top-level test! 1..2 KTAP version 1 # Subtest: suite1 1..1 ok 1 - test1 ok 1 -suite1 // etc.
Then we can simplify the parser by not differentiating (as much) between the top-level test and subtests. This also simplifies parsing multiple KTAP documents (e.g. for supporting modules, etc.)
We'll probably talk about this offline soon, but I wanted to put this out there.
Daniel
The location of the "Subtest" line before the KTAP version line is potentially not the most optimal but it seems to be the best choice while ensuring compatibility with the current KTAP v1 spec. I recommend that we discuss a standardized replacement for this "Subtest" line in the KTAP v2 spec.
On Tue, Nov 15, 2022 at 5:41 PM Daniel Latypov dlatypov@google.com wrote:
On Tue, Nov 15, 2022 at 12:18 PM Rae Moar rmoar@google.com wrote:
Yes, I agree. I think it does make more sense to provide KTAP compatibility with the parser before changing the test output. This would also help to solve the issue that Daniel brought up on this patch about the "KTAP version 1" line and test plan being stored in the test.log as random kernel output. I will swap the patches in the v2 of this patch series.
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??
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.
I also struggle to decide how the "Subtest" line should be handled. Since the current KTAP v1 spec does not provide a way to declare the name of a test with subtests prior to the results, I think it is important to continue to include this Subtest line because it provides that functionality. Additionally, the line is not expected to be very disruptive for other parsers because it is read as a diagnostic line.
Yeah, since it's going to be ignored as a diagnostic line, I think we're largely free to put it where we want?
I'm actually leaning towards making things more uniform e.g.
KTAP version 1 # Subtest: optionally set for the top-level test! 1..2 KTAP version 1 # Subtest: suite1 1..1 ok 1 - test1 ok 1 -suite1 // etc.
Then we can simplify the parser by not differentiating (as much) between the top-level test and subtests. This also simplifies parsing multiple KTAP documents (e.g. for supporting modules, etc.)
We'll probably talk about this offline soon, but I wanted to put this out there.
Daniel
This was partially discussed off the mailing list. I am still a bit wary to change the output to this format because it is not strictly compliant with the KTAP v1 spec which requires the test plan to be the line directly below the version line. However, I do think that this location makes much more sense for the "# Subtest" line and since this is a diagnostic line and likely won't break any existing parsers, I will be changing to this format in v2 as well as adjusting the parser to allow for this format instead.
The optional top-level "# Subtest" line was also discussed off the mailing list. It was generally agreed that this could be useful in the future but not as an addition for this patch. However, the parser should be altered such that if the line is present, the parser will not crash but also not actively print the provided test name.
As discussed previously in this patch series, a test name line could be helpful to propose for KTAP v2 as a replacement to this "# Subtest" line, which is present in the TAP version 14 spec.
Rae
The location of the "Subtest" line before the KTAP version line is potentially not the most optimal but it seems to be the best choice while ensuring compatibility with the current KTAP v1 spec. I recommend that we discuss a standardized replacement for this "Subtest" line in the KTAP v2 spec.
linux-kselftest-mirror@lists.linaro.org