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

[Rae Moar]

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