| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 22 Nov 2022 17:06:00 -0500
From: Rae Moar <rmoar@...gle.com>
To: Daniel Latypov <dlatypov@...gle.com>
Cc: brendanhiggins@...gle.com, davidgow@...gle.com,
skhan@...uxfoundation.org, mauro.chehab@...ux.intel.com,
kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org, isabbasso@...eup.net,
anders.roxell@...aro.org
Subject: Re: [PATCH v2 1/2] kunit: tool: parse KTAP compliant test output
On Mon, Nov 21, 2022 at 3:51 PM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> On Mon, Nov 21, 2022 at 10:48 AM Rae Moar <rmoar@...gle.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 (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@...gle.com>
> > Reviewed-by: Daniel Latypov <dlatypov@...gle.com>
>
> 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@...gle.com>
> > ---
> >
> > Changes since v1:
> > https://lore.kernel.org/all/20221104194705.3245738-2-rmoar@google.com/
> > - 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
Powered by blists - more mailing lists