Re: [PATCH v3 09/16] kunit: tool: Don't overwrite test status based on subtest counts

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 4155 bytes --]

On Wed, 11 Jun 2025 at 15:38, Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> If a subtest itself reports success, but the outer testcase fails,
> the whole testcase should be reported as a failure.
> However the status is recalculated based on the test counts,
> overwriting the outer test result.
> Synthesize a failed test in this case to make sure the failure is not
> swallowed.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---

Hmm... this is definitely a nasty edge-case. I don't completely like
this solution, but none of the other options seem drastically better.

I think the more obvious options are either to _always_ count tests
alongside their subtests, or to _never_ do so, but acknowledge that
"test failed, but failure count is 0" is a valid option. But neither
of those are especially satisfying, either greatly inflating test
counts, or creating obvious contradictions.

So I'm tentatively in favour of this, but if anyone has a nicer way of
doing it, I'm all ears.

The implementation looks good. If we can add the explicit checks for
the sub(sub)test results as mentioned in the previous patch, that'd be
even better.

Reviewed-by: David Gow <davidgow@google.com>

>  tools/testing/kunit/kunit_parser.py                                  | 5 +++++
>  tools/testing/kunit/kunit_tool_test.py                               | 2 +-
>  tools/testing/kunit/test_data/test_is_test_passed-failure-nested.log | 3 +++
>  3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index c176487356e6c94882046b19ea696d750905b8d5..2478beb28fc3db825855ad46200340e884da7df1 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -686,6 +686,11 @@ def bubble_up_test_results(test: Test) -> None:
>                 counts.add_status(status)
>         elif test.counts.get_status() == TestStatus.TEST_CRASHED:
>                 test.status = TestStatus.TEST_CRASHED
> +       if not test.ok_status():
> +               for t in subtests:
> +                       if not t.ok_status():
> +                               counts.add_status(t.status)
> +                               break
>
>  def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest: bool, printer: Printer) -> Test:
>         """
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 691cde9b030f7729128490c1bdb42ccee1967ad6..c25f52650837e83325b06bddd2aa665fd29f91d9 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -170,7 +170,7 @@ class KUnitParserTest(unittest.TestCase):
>                 with open(nested_log) as file:
>                         result = kunit_parser.parse_run_tests(file.readlines(), stdout)
>                 self.assertEqual(kunit_parser.TestStatus.FAILURE, result.status)
> -               self.assertEqual(result.counts.failed, 2)
> +               self.assertEqual(result.counts.failed, 3)
>                 self.assertEqual(kunit_parser.TestStatus.FAILURE, result.subtests[0].status)

Could we add:
self.assertEqual(kunit_parser.TestStatus.SUCCESS,
result.subtests[0].subtests[0].status)

>                 self.assertEqual(kunit_parser.TestStatus.FAILURE, result.subtests[1].status)

and

self.assertEqual(kunit_parser.TestStatus.FAILURE,
result.subtests[1].subtests[0].status)


>
> diff --git a/tools/testing/kunit/test_data/test_is_test_passed-failure-nested.log b/tools/testing/kunit/test_data/test_is_test_passed-failure-nested.log
> index 835816e0a07715a514f5f5afab1b6250037feaf4..cd9033c464792e6294905a5676346684182874ad 100644
> --- a/tools/testing/kunit/test_data/test_is_test_passed-failure-nested.log
> +++ b/tools/testing/kunit/test_data/test_is_test_passed-failure-nested.log
> @@ -1,5 +1,8 @@
>  KTAP version 1
>  1..2
> +    KTAP version 1
> +    1..1
> +        ok 1 test 1
>  not ok 1 subtest 1
>      KTAP version 1
>      1..1
>
> --
> 2.49.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5281 bytes --]