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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSkG3dY3THo5DQOvWj1xX8XFFnnk7CXvVE2opQJZg4nwXg@mail.gmail.com>
Date: Fri, 20 Jun 2025 17:37:44 +0800
From: David Gow <davidgow@...gle.com>
To: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, 
	Andrew Morton <akpm@...ux-foundation.org>, Willy Tarreau <w@....eu>, 
	Thomas Weißschuh <linux@...ssschuh.net>, 
	Brendan Higgins <brendan.higgins@...ux.dev>, Rae Moar <rmoar@...gle.com>, 
	Shuah Khan <shuah@...nel.org>, Jonathan Corbet <corbet@....net>, 
	Nicolas Schier <nicolas.schier@...ux.dev>, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Alexandre Ghiti <alex@...ti.fr>, Christophe Leroy <christophe.leroy@...roup.eu>, 
	linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com, 
	linux-doc@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	workflows@...r.kernel.org
Subject: Re: [PATCH v3 09/16] kunit: tool: Don't overwrite test status based
 on subtest counts

On Wed, 11 Jun 2025 at 15:38, Thomas Weißschuh
<thomas.weissschuh@...utronix.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@...utronix.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@...gle.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
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5281 bytes)