Re: [PATCH v1 2/2] kunit: tool: parse KTAP compliant test output

Rae Moar
2022-11-15T20:46:27Z

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
> >