On Mon, Nov 21, 2022 at 3:51 PM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote: > > On Mon, Nov 21, 2022 at 10:48 AM Rae Moar <rmoar@xxxxxxxxxx> 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 (changes made in the next patch of > > this series): > > > > KTAP version 1 > > 1..1 > > KTAP version 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 > > > > Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx> > > Reviewed-by: Daniel Latypov <dlatypov@xxxxxxxxxx> > > Still looks good to me overall. > As noted offline, this sadly has a conflict with another recent patch, > so it won't apply to the kunit branch right now. > That's my fault: > https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/commit/?h=kunit&id=2a883a9f5c1f1c7bb9d9116da68e2ef2faeae5b8 > > I found a few optional nits down below that we could also address in > the rebased v3. > > > Reviewed-by: David Gow <davidgow@xxxxxxxxxx> > > --- > > > > Changes since v1: > > https://lore.kernel.org/all/20221104194705.3245738-2-rmoar@xxxxxxxxxx/ > > - Switch order of patches to make changes to the parser before making > > changes to the test output > > - Change placeholder label for test header from “Test suite” to empty > > string > > - Change parser to approve the new KTAP version line in the subtest header > > to be before the subtest header line rather than after. > > Thanks, as noted on the child patch, I think this will make our lives > easier in the future, even if it technically violates the v1 spec > (which requires the test plan right after the KTAP header IIUC). > > Given the wording > Diagnostic lines can be anywhere in the test output. > I assume most implementations would likely ignore unexpected lines > beginning with "# " already. > > > - Note: Considered changing parser to allow for the top-level of testing > > to have a '# Subtest' line as discussed in v1 but this breaks the > > missing test plan test. So I think it would be best to add this ability > > at a later time or after top-level test name and result lines are > > discussed for KTAP v2. > > Makes sense to me. > > > message = test.name > > + if message != "": > > + # Add a leading space before the subtest counts only if a test name > > + # is provided using a "# Subtest" header line. > > + message += " " > > if test.expected_count: > > if test.expected_count == 1: > > - message += ' (1 subtest)' > > + message += '(1 subtest)' > > Thanks, I like this output a lot better than having "Test suite" as a > placeholder name. > Tested this out by tweaking some kunit output locally and I get > > [12:39:11] ======================= (4 subtests) ======================= > [12:39:11] [PASSED] parse_filter_test > [12:39:11] [PASSED] filter_suites_test > [12:39:11] [PASSED] filter_suites_test_glob_test > [12:39:11] [PASSED] filter_suites_to_empty_test > [12:39:11] =============== [PASSED] kunit_executor_test =============== > Yeah I agree, this looks much better. > > else: > > - message += f' ({test.expected_count} subtests)' > > + message += f'({test.expected_count} subtests)' > > stdout.print_with_timestamp(format_test_divider(message, len(message))) > > > > def print_log(log: Iterable[str]) -> None: > > @@ -647,7 +653,7 @@ 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 > > @@ -665,15 +671,32 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test: > > 1..4 > > [subtests] > > > > - - Subtest header line > > + - Subtest header (must include either the KTAP version line or > > + "# Subtest" header line) > > > > - Example: > > + Example (preferred format with both KTAP version line and > > + "# Subtest" line): > > + > > + KTAP version 1 > > + # Subtest: name > > + 1..3 > > + [subtests] > > + ok 1 name > > + > > + Example (only "# Subtest" line): > > > > # Subtest: name > > 1..3 > > [subtests] > > ok 1 name > > > > + Example (only KTAP version line, compliant with KTAP v1 spec): > > + > > + KTAP version 1 > > + 1..3 > > + [subtests] > > + ok 1 name > > + > > - Test result line > > > > Example: > > @@ -685,28 +708,29 @@ 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 > > """ > > 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 > > + if not is_subtest: > > + # If parsing the main/top-level test, parse KTAP version line and > > # test plan > > test.name = "main" > > + ktap_line = 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 not the main test, attempt to parse a test header contatining > > typo: contatin => contain Oops, I will change this. > > > + # the KTAP version line and/or subtest header line > > + ktap_line = parse_ktap_header(lines, test) > > + subtest_line = parse_test_header(lines, test) > > + parent_test = (ktap_line or subtest_line) > > LGTM (this is where we changed to parse the KTAP header before " # Subtest"). > > Optional: do we want to extend kunit_tool_test.py to validate this logic too? > E.g. given input like > > KTAP version 1 > 1..1 > KTAP version 1 > # Subtest: suite > 1..1 > ok 1 - test > ok 1 - subtest > > we could assert that the parsed output contains "suite (1 subtest)" > > i.e. > self.print_mock.assert_any_call(StrContains('suite (1 subtest)')) > I like this addition. I will add this test to kunit_tool_test.py. > Daniel
