linux-kernel - Re: [PATCH v4 09/15] kunit: tool: Parse skipped tests from kselftest.h

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: <CA+GJov7HN_HTVOygyF6SW12DeghW6BYc4KxEz8Ktub=QBqv0oQ@mail.gmail.com>
Date: Tue, 1 Jul 2025 17:22:07 -0400
From: Rae Moar <rmoar@...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>, David Gow <davidgow@...gle.com>, 
	Shuah Khan <shuah@...nel.org>, Jonathan Corbet <corbet@....net>, 
	Nicolas Schier <nicolas.schier@...ux.dev>, 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, workflows@...r.kernel.org
Subject: Re: [PATCH v4 09/15] kunit: tool: Parse skipped tests from kselftest.h

On Thu, Jun 26, 2025 at 2:10 AM Thomas Weißschuh
<thomas.weissschuh@...utronix.de> wrote:
>
> Skipped tests reported by kselftest.h use a different format than KTAP,
> there is no explicit test name. Normally the test name is part of the
> free-form string after the SKIP keyword:
>
>         ok 3 # SKIP test: some reason
>
> Extend the parser to handle those correctly. Use the free-form string as
> test name instead.

Hello!

I am happy to see that the kunit_parser will be used to parse
kselftest.h patches!

This patch looks pretty good to me! However, I do have one concern
where it does cause the parser to break.

Thanks!
-Rae

>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
> Reviewed-by: David Gow <davidgow@...gle.com>
> ---
>  tools/testing/kunit/kunit_parser.py                             | 8 +++++---
>  tools/testing/kunit/test_data/test_is_test_passed-kselftest.log | 3 ++-
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index 2478beb28fc3db825855ad46200340e884da7df1..4599d23c79b79f0e219d655c7053c8c3b34f8152 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -352,9 +352,9 @@ def parse_test_plan(lines: LineStream, test: Test) -> bool:
>         lines.pop()
>         return True
>
> -TEST_RESULT = re.compile(r'^\s*(ok|not ok) ([0-9]+) (- )?([^#]*)( # .*)?$')
> +TEST_RESULT = re.compile(r'^\s*(ok|not ok) ([0-9]+)? ?(- )?([^#]*)( # .*)?$')
>
> -TEST_RESULT_SKIP = re.compile(r'^\s*(ok|not ok) ([0-9]+) (- )?(.*) # SKIP(.*)$')
> +TEST_RESULT_SKIP = re.compile(r'^\s*(ok|not ok) ([0-9]+)? ?(- )?(.*) # SKIP ?(.*)$')

My concern with this patch is that this makes the test number optional
([0-9]+) -> ([0-9]+)? for both the test result regex matches. But if I
do pass in KTAP containing a line where the test number is missing, it
causes the parser to crash because other lines of the code rely on the
match with the test number.

I would prefer if we keep the test number as required. This is
integral to the KTAP specification and seems to be largely shared by
different frameworks test output.

>
>  def peek_test_name_match(lines: LineStream, test: Test) -> bool:
>         """
> @@ -379,6 +379,8 @@ def peek_test_name_match(lines: LineStream, test: Test) -> bool:
>         if not match:
>                 return False
>         name = match.group(4)
> +       if not name:
> +               return False
>         return name == test.name
>
>  def parse_test_result(lines: LineStream, test: Test,
> @@ -416,7 +418,7 @@ def parse_test_result(lines: LineStream, test: Test,
>
>         # Set name of test object
>         if skip_match:
> -               test.name = skip_match.group(4)
> +               test.name = skip_match.group(4) or skip_match.group(5)
>         else:
>                 test.name = match.group(4)
>
> diff --git a/tools/testing/kunit/test_data/test_is_test_passed-kselftest.log b/tools/testing/kunit/test_data/test_is_test_passed-kselftest.log
> index 65d3f27feaf22a3f47ed831c4c24f6f11c625a92..30d9ef18bcec177067288d5242771236f29b7d56 100644
> --- a/tools/testing/kunit/test_data/test_is_test_passed-kselftest.log
> +++ b/tools/testing/kunit/test_data/test_is_test_passed-kselftest.log
> @@ -1,5 +1,5 @@
>  TAP version 13
> -1..2
> +1..3
>  # selftests: membarrier: membarrier_test_single_thread
>  # TAP version 13
>  # 1..2
> @@ -12,3 +12,4 @@ ok 1 selftests: membarrier: membarrier_test_single_thread
>  # ok 1 sys_membarrier available
>  # ok 2 sys membarrier invalid command test: command = -1, flags = 0, errno = 22. Failed as expected
>  ok 2 selftests: membarrier: membarrier_test_multi_thread
> +ok 3 # SKIP selftests: membarrier: membarrier_test_multi_thread
>
> --
> 2.50.0
>