[PATCH v3 1/2] kunit: tool: Fix bug in parsing test plan

[PATCH v3 1/2] kunit: tool: Fix bug in parsing test plan

[Rae Moar]

A bug was identified where the KTAP below caused an infinite loop:

 TAP version 13
 ok 4 test_case
 1..4

The infinite loop was caused by the parser not parsing a test plan
if following a test result line.

Fix this bug by parsing test plan line to avoid the infinite loop.

Signed-off-by: Rae Moar <rmoar@google.com>
---
Changes since v2:
- None, adds test in second patch

 tools/testing/kunit/kunit_parser.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 29fc27e8949b..da53a709773a 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -759,7 +759,7 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str], 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, printer)
+		parse_ktap_header(lines, test, printer)
 		test.log.extend(parse_diagnostic(lines))
 		parse_test_plan(lines, test)
 		parent_test = True
@@ -768,13 +768,12 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest:
 		# the KTAP version line and/or subtest header line
 		ktap_line = parse_ktap_header(lines, test, printer)
 		subtest_line = parse_test_header(lines, test)
+		test.log.extend(parse_diagnostic(lines))
+		parse_test_plan(lines, test)
 		parent_test = (ktap_line or subtest_line)
 		if parent_test:
-			# If KTAP version line and/or subtest header is found, attempt
-			# to parse test plan and print test header
-			test.log.extend(parse_diagnostic(lines))
-			parse_test_plan(lines, test)
 			print_test_header(test, printer)
+
 	expected_count = test.expected_count
 	subtests = []
 	test_num = 1

base-commit: 0619a4868fc1b32b07fb9ed6c69adc5e5cf4e4b2
-- 
2.49.0.rc1.451.g8f38331e32-goog

[PATCH v3 2/2] kunit: tool: add test to check parsing late test plan

[Rae Moar]

Add test to check for the infinite loop caused by the inability
to parse a late test plan.

The test parses the following output:
 TAP version 13
 ok 4 test4
 1..4

Signed-off-by: Rae Moar <rmoar@google.com>
---
Changes since v2:
- Adds this patch to add a test for this behavior

 tools/testing/kunit/kunit_tool_test.py | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 0bcb0cc002f8..5ff4f6ffd873 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -363,6 +363,17 @@ class KUnitParserTest(unittest.TestCase):
 		self.print_mock.assert_any_call(StrContains('  Indented more.'))
 		self.noPrintCallContains('not ok 1 test1')
 
+	def test_parse_late_test_plan(self):
+		output = """
+		TAP version 13
+		ok 4 test4
+		1..4
+		"""
+		result = kunit_parser.parse_run_tests(output.splitlines(), stdout)
+		# Missing test results after test plan should alert a suspected test crash.
+		self.assertEqual(kunit_parser.TestStatus.TEST_CRASHED, result.status)
+		self.assertEqual(result.counts, kunit_parser.TestCounts(passed=1, crashed=1, errors=1))
+
 def line_stream_from_strs(strs: Iterable[str]) -> kunit_parser.LineStream:
 	return kunit_parser.LineStream(enumerate(strs, start=1))
 
-- 
2.49.0.rc1.451.g8f38331e32-goog

Re: [PATCH v3 1/2] kunit: tool: Fix bug in parsing test plan

[David Gow]

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

On Fri, 14 Mar 2025 at 03:27, Rae Moar <rmoar@google.com> wrote:
>
> A bug was identified where the KTAP below caused an infinite loop:
>
>  TAP version 13
>  ok 4 test_case
>  1..4
>
> The infinite loop was caused by the parser not parsing a test plan
> if following a test result line.
>
> Fix this bug by parsing test plan line to avoid the infinite loop.
>
> Signed-off-by: Rae Moar <rmoar@google.com>
> ---

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

Cheers,

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

Re: [PATCH v3 2/2] kunit: tool: add test to check parsing late test plan

[David Gow]

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

On Fri, 14 Mar 2025 at 03:27, Rae Moar <rmoar@google.com> wrote:
>
> Add test to check for the infinite loop caused by the inability
> to parse a late test plan.
>
> The test parses the following output:
>  TAP version 13
>  ok 4 test4
>  1..4
>
> Signed-off-by: Rae Moar <rmoar@google.com>
> ---

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

Cheers,
-- David

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

Re: [PATCH v3 1/2] kunit: tool: Fix bug in parsing test plan

[Brendan Jackman]

On Fri, 14 Mar 2025 at 06:37, David Gow <davidgow@google.com> wrote:
>
> On Fri, 14 Mar 2025 at 03:27, Rae Moar <rmoar@google.com> wrote:
> >
> > A bug was identified where the KTAP below caused an infinite loop:
> >
> >  TAP version 13
> >  ok 4 test_case
> >  1..4
> >
> > The infinite loop was caused by the parser not parsing a test plan
> > if following a test result line.
> >
> > Fix this bug by parsing test plan line to avoid the infinite loop.

Hi Rae,

With this change and this input:

https://gist.githubusercontent.com/bjackman/220265699f346e16161c6534b115019b/raw/a2e0e1aa75c0d8ab9814708b028ec78810a0471b/run_vmtests.sh.tap

The infinite loop is gone, but it's still hallucinating a [CRASHED] result:

[16:07:15] # SUMMARY: PASS=17 SKIP=0 FAIL=1
[16:07:15] [CRASHED]
...
[16:07:15] Testing complete. Ran 19 tests: passed: 17, failed: 1, crashed: 1

Re: [PATCH v3 1/2] kunit: tool: Fix bug in parsing test plan

[Rae Moar]

On Mon, Mar 17, 2025 at 12:13 PM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Fri, 14 Mar 2025 at 06:37, David Gow <davidgow@google.com> wrote:
> >
> > On Fri, 14 Mar 2025 at 03:27, Rae Moar <rmoar@google.com> wrote:
> > >
> > > A bug was identified where the KTAP below caused an infinite loop:
> > >
> > >  TAP version 13
> > >  ok 4 test_case
> > >  1..4
> > >
> > > The infinite loop was caused by the parser not parsing a test plan
> > > if following a test result line.
> > >
> > > Fix this bug by parsing test plan line to avoid the infinite loop.
>
> Hi Rae,
>
> With this change and this input:
>
> https://gist.githubusercontent.com/bjackman/220265699f346e16161c6534b115019b/raw/a2e0e1aa75c0d8ab9814708b028ec78810a0471b/run_vmtests.sh.tap
>
> The infinite loop is gone, but it's still hallucinating a [CRASHED] result:
>
> [16:07:15] # SUMMARY: PASS=17 SKIP=0 FAIL=1
> [16:07:15] [CRASHED]
> ...
> [16:07:15] Testing complete. Ran 19 tests: passed: 17, failed: 1, crashed: 1

Hi! Thanks for the response. This is an interesting problem. Should a
test plan at the bottom cause a crash because no tests were found
after? Again with KTAP, a crash would make sense. I feel this example
demonstrates why there is a need for a general parser that can parse
kselftest output as well as KUnit.  I'll see how difficult it would be
to change the parser to accommodate removing the crash in a new
version. Thanks!

-Rae

Re: [PATCH v3 1/2] kunit: tool: Fix bug in parsing test plan

[Brendan Jackman]

On Wed Mar 19, 2025 at 9:11 PM UTC, Rae Moar wrote:
> On Mon, Mar 17, 2025 at 12:13 PM Brendan Jackman <jackmanb@google.com> wrote:
> >
> > On Fri, 14 Mar 2025 at 06:37, David Gow <davidgow@google.com> wrote:
> > >
> > > On Fri, 14 Mar 2025 at 03:27, Rae Moar <rmoar@google.com> wrote:
> > > >
> > > > A bug was identified where the KTAP below caused an infinite loop:
> > > >
> > > >  TAP version 13
> > > >  ok 4 test_case
> > > >  1..4
> > > >
> > > > The infinite loop was caused by the parser not parsing a test plan
> > > > if following a test result line.
> > > >
> > > > Fix this bug by parsing test plan line to avoid the infinite loop.
> >
> > Hi Rae,
> >
> > With this change and this input:
> >
> > https://gist.githubusercontent.com/bjackman/220265699f346e16161c6534b115019b/raw/a2e0e1aa75c0d8ab9814708b028ec78810a0471b/run_vmtests.sh.tap
> >
> > The infinite loop is gone, but it's still hallucinating a [CRASHED] result:
> >
> > [16:07:15] # SUMMARY: PASS=17 SKIP=0 FAIL=1
> > [16:07:15] [CRASHED]
> > ...
> > [16:07:15] Testing complete. Ran 19 tests: passed: 17, failed: 1, crashed: 1
>
> Hi! Thanks for the response. This is an interesting problem. Should a
> test plan at the bottom cause a crash because no tests were found
> after? Again with KTAP, a crash would make sense. I feel this example
> demonstrates why there is a need for a general parser that can parse
> kselftest output as well as KUnit.  I'll see how difficult it would be
> to change the parser to accommodate removing the crash in a new
> version. Thanks!

Yeah, if this is difficult, it would be fine for the KUnit tool to
just say "sorry this is a KTAP parser not a general TAP parser"

With your new patch:

https://lore.kernel.org/linux-kernel/20250319223351.1517262-1-rmoar@google.com/

I get this:

[10:00:41] # SUMMARY: PASS=17 SKIP=0 FAIL=1
[10:00:41] [ERROR] Test: : No more test results!
[10:00:41] [NO TESTS RUN]

Which I think is basically fine.

We do want something that can parse this stuff properly but I tihnk
that's a separate little project.

Also, vanilla non-K TAP... is honestly a terrible format.  A flat
structure for the test results (i.e. no nesting) is a bit of a joke -
even for just run_vmtests.sh which has a pretty tractable amount of
tests, it's already a pain to mentally keep track of which results
come from where. Maybe if I want to be able to parse the output of
tests I should just switch them to using KTAP.