[PATCH] kunit: tool: Fix bug in parsing test plan

[PATCH] 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 bug to correctly parse test plan and add error if test plan is
missing.

Signed-off-by: Rae Moar <rmoar@google.com>
---
 tools/testing/kunit/kunit_parser.py    | 12 +++++++-----
 tools/testing/kunit/kunit_tool_test.py |  5 ++---
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 29fc27e8949b..5dcbc670e1dc 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -761,20 +761,22 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest:
 		test.name = "main"
 		ktap_line = parse_ktap_header(lines, test, printer)
 		test.log.extend(parse_diagnostic(lines))
-		parse_test_plan(lines, test)
+		plan_line = parse_test_plan(lines, test)
 		parent_test = True
 	else:
 		# If not the main test, attempt to parse a test header containing
 		# 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))
+		plan_line = 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)
+
+	if parent_test and not plan_line:
+			test.add_error(printer, 'missing test plan!')
+
 	expected_count = test.expected_count
 	subtests = []
 	test_num = 1
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 0bcb0cc002f8..e1e142c1a850 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -181,8 +181,7 @@ class KUnitParserTest(unittest.TestCase):
 			result = kunit_parser.parse_run_tests(
 				kunit_parser.extract_tap_lines(
 				file.readlines()), stdout)
-		# A missing test plan is not an error.
-		self.assertEqual(result.counts, kunit_parser.TestCounts(passed=10, errors=0))
+		self.assertEqual(result.counts, kunit_parser.TestCounts(passed=10, errors=2))
 		self.assertEqual(kunit_parser.TestStatus.SUCCESS, result.status)
 
 	def test_no_tests(self):
@@ -203,7 +202,7 @@ class KUnitParserTest(unittest.TestCase):
 		self.assertEqual(
 			kunit_parser.TestStatus.NO_TESTS,
 			result.subtests[0].subtests[0].status)
-		self.assertEqual(result.counts, kunit_parser.TestCounts(passed=1, errors=1))
+		self.assertEqual(result.counts, kunit_parser.TestCounts(passed=1, errors=2))
 
 
 	def test_no_kunit_output(self):

base-commit: 0619a4868fc1b32b07fb9ed6c69adc5e5cf4e4b2
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH] kunit: tool: Fix bug in parsing test plan

[David Gow]

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

On Thu, 6 Mar 2025 at 08:29, 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 bug to correctly parse test plan and add error if test plan is
> missing.
>
> Signed-off-by: Rae Moar <rmoar@google.com>
> ---

Thanks for looking into this: I don't think we want to unconditionally
error if there's no test plan, though. Pretty much no parameterised
tests include one -- it's not always possible to know how many tests
there'll be in advance -- so this triggers all of the time.

Maybe we can only include an error if we find a test plan line after
an existing result, or something?

-- David

>  tools/testing/kunit/kunit_parser.py    | 12 +++++++-----
>  tools/testing/kunit/kunit_tool_test.py |  5 ++---
>  2 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index 29fc27e8949b..5dcbc670e1dc 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -761,20 +761,22 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest:
>                 test.name = "main"
>                 ktap_line = parse_ktap_header(lines, test, printer)
>                 test.log.extend(parse_diagnostic(lines))
> -               parse_test_plan(lines, test)
> +               plan_line = parse_test_plan(lines, test)
>                 parent_test = True
>         else:
>                 # If not the main test, attempt to parse a test header containing
>                 # 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))
> +               plan_line = 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)
> +
> +       if parent_test and not plan_line:
> +                       test.add_error(printer, 'missing test plan!')
> +
>         expected_count = test.expected_count
>         subtests = []
>         test_num = 1
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 0bcb0cc002f8..e1e142c1a850 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -181,8 +181,7 @@ class KUnitParserTest(unittest.TestCase):
>                         result = kunit_parser.parse_run_tests(
>                                 kunit_parser.extract_tap_lines(
>                                 file.readlines()), stdout)
> -               # A missing test plan is not an error.
> -               self.assertEqual(result.counts, kunit_parser.TestCounts(passed=10, errors=0))
> +               self.assertEqual(result.counts, kunit_parser.TestCounts(passed=10, errors=2))
>                 self.assertEqual(kunit_parser.TestStatus.SUCCESS, result.status)
>
>         def test_no_tests(self):
> @@ -203,7 +202,7 @@ class KUnitParserTest(unittest.TestCase):
>                 self.assertEqual(
>                         kunit_parser.TestStatus.NO_TESTS,
>                         result.subtests[0].subtests[0].status)
> -               self.assertEqual(result.counts, kunit_parser.TestCounts(passed=1, errors=1))
> +               self.assertEqual(result.counts, kunit_parser.TestCounts(passed=1, errors=2))
>
>
>         def test_no_kunit_output(self):
>
> base-commit: 0619a4868fc1b32b07fb9ed6c69adc5e5cf4e4b2
> --
> 2.48.1.711.g2feabab25a-goog
>

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

Re: [PATCH] kunit: tool: Fix bug in parsing test plan

[Brendan Jackman]

On Thu, 6 Mar 2025 at 10:00, David Gow <davidgow@google.com> wrote:
>
> On Thu, 6 Mar 2025 at 08:29, 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 bug to correctly parse test plan and add error if test plan is
> > missing.
> >
> > Signed-off-by: Rae Moar <rmoar@google.com>

Thanks for taking a look at this Rae! I tried to take a look myself
but I could not really get a grip on the parsing logic in the time I
had.

> Thanks for looking into this: I don't think we want to unconditionally
> error if there's no test plan, though. Pretty much no parameterised
> tests include one -- it's not always possible to know how many tests
> there'll be in advance -- so this triggers all of the time.
>
> Maybe we can only include an error if we find a test plan line after
> an existing result, or something?

Since I reported this bug, I discovered that the example above is in
fact valid TAP:

> The plan [...] must appear once, whether at the beginning or end of the output.

From https://testanything.org/tap-version-13-specification.html

Re: [PATCH] kunit: tool: Fix bug in parsing test plan

[Rae Moar]

On Thu, Mar 6, 2025 at 7:26 AM Brendan Jackman <jackmanb@google.com> wrote:
>
> On Thu, 6 Mar 2025 at 10:00, David Gow <davidgow@google.com> wrote:
> >
> > On Thu, 6 Mar 2025 at 08:29, 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 bug to correctly parse test plan and add error if test plan is
> > > missing.
> > >
> > > Signed-off-by: Rae Moar <rmoar@google.com>
>
> Thanks for taking a look at this Rae! I tried to take a look myself
> but I could not really get a grip on the parsing logic in the time I
> had.
>
> > Thanks for looking into this: I don't think we want to unconditionally
> > error if there's no test plan, though. Pretty much no parameterised
> > tests include one -- it's not always possible to know how many tests
> > there'll be in advance -- so this triggers all of the time.
> >
> > Maybe we can only include an error if we find a test plan line after
> > an existing result, or something?
>
> Since I reported this bug, I discovered that the example above is in
> fact valid TAP:
>
> > The plan [...] must appear once, whether at the beginning or end of the output.
>
> From https://testanything.org/tap-version-13-specification.html

Hi!
This brings up an interesting question because the parser has been
mainly geared towards parsing KTAP
(https://docs.kernel.org/dev-tools/ktap.html) rather than TAP.
(Although we do try to have backwards compatibility with TAP v14
"Subtest" lines)

For example,

TAP version 13
1..1
  TAP version 13
  1..1
  ok 1 test_case
ok 1 test_suite

This would be accepted by the parser without error because it is valid
KTAP even though it is not valid TAP v13.

The scenario above that caused the infinite loop would be incorrect
KTAP (which requires the test plan to follow a version line) but
correct TAP v13. So do we accept it without error? Ideally, we would
parse based on the version given in the version line.

Just an interesting thought. Either way, I will remove the error for
now as our parameterized tests don't properly produce a test plan,
which causes errors.

Thanks!
-Rae

Re: [PATCH] kunit: tool: Fix bug in parsing test plan

[Brendan Jackman]

Hi Rae,

On Thu, Mar 06, 2025 at 11:02:13AM -0500, Rae Moar wrote:
> On Thu, Mar 6, 2025 at 7:26 AM Brendan Jackman <jackmanb@google.com> wrote:
> > Since I reported this bug, I discovered that the example above is in
> > fact valid TAP:
> >
> > > The plan [...] must appear once, whether at the beginning or end of the output.
> >
> > From https://testanything.org/tap-version-13-specification.html
> 
> Hi!
> This brings up an interesting question because the parser has been
> mainly geared towards parsing KTAP
> (https://docs.kernel.org/dev-tools/ktap.html) rather than TAP.
> (Although we do try to have backwards compatibility with TAP v14
> "Subtest" lines)
> 
> For example,
> 
> TAP version 13
> 1..1
>   TAP version 13
>   1..1
>   ok 1 test_case
> ok 1 test_suite
> 
> This would be accepted by the parser without error because it is valid
> KTAP even though it is not valid TAP v13.
> 
> The scenario above that caused the infinite loop would be incorrect
> KTAP (which requires the test plan to follow a version line) but
> correct TAP v13. So do we accept it without error? Ideally, we would
> parse based on the version given in the version line.
> 
> Just an interesting thought. Either way, I will remove the error for
> now as our parameterized tests don't properly produce a test plan,
> which causes errors.

OK yeah sounds good. Now I think about it, I should note that I am
abusing the KUnit scripts to parse the result of a kselftest run. I
just assumed that this would be supported but actually there's no
particular reason I should have assumed that :)

I would like to have general support in the tree for parsing test
output ([K]TAP it's not really a human-readable format IMO... even in
the best case like nice tidy KUnit tests I find the structure very
hard to read. And without something like 'kunit.py parse' I find it
extremely difficult to get a high-level gestalt of how a test run
went). But that doesn't mean kunit.py is responsible for the whole
kernel tree's output! Still, it would be nice to handle it to the
extent that's practical (and at least, with no infinite loops :D).

Re: [PATCH] kunit: tool: Fix bug in parsing test plan

[Rae Moar]

On Thu, Mar 6, 2025 at 4:00 AM David Gow <davidgow@google.com> wrote:
>
> On Thu, 6 Mar 2025 at 08:29, 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 bug to correctly parse test plan and add error if test plan is
> > missing.
> >
> > Signed-off-by: Rae Moar <rmoar@google.com>
> > ---
>
> Thanks for looking into this: I don't think we want to unconditionally
> error if there's no test plan, though. Pretty much no parameterised
> tests include one -- it's not always possible to know how many tests
> there'll be in advance -- so this triggers all of the time.
>
> Maybe we can only include an error if we find a test plan line after
> an existing result, or something?

Hi David!

I thought I'd include the error in the first version but I figured it
might not be accepted. Technically it is improper KTAP for the test
plan to be missing so the error would be correct but because it fires
on parameterized tests which is not ideal.

I wonder for parameterized tests if we could output a test plan:
"1..X" indicating an unknown number of tests or something similar. I'd
be happy to implement this. However, I am happy to remove the error
for the second version.

Thanks!
-Rae

>
> -- David
>
> >  tools/testing/kunit/kunit_parser.py    | 12 +++++++-----
> >  tools/testing/kunit/kunit_tool_test.py |  5 ++---
> >  2 files changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> > index 29fc27e8949b..5dcbc670e1dc 100644
> > --- a/tools/testing/kunit/kunit_parser.py
> > +++ b/tools/testing/kunit/kunit_parser.py
> > @@ -761,20 +761,22 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str], is_subtest:
> >                 test.name = "main"
> >                 ktap_line = parse_ktap_header(lines, test, printer)
> >                 test.log.extend(parse_diagnostic(lines))
> > -               parse_test_plan(lines, test)
> > +               plan_line = parse_test_plan(lines, test)
> >                 parent_test = True
> >         else:
> >                 # If not the main test, attempt to parse a test header containing
> >                 # 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))
> > +               plan_line = 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)
> > +
> > +       if parent_test and not plan_line:
> > +                       test.add_error(printer, 'missing test plan!')
> > +
> >         expected_count = test.expected_count
> >         subtests = []
> >         test_num = 1
> > diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> > index 0bcb0cc002f8..e1e142c1a850 100755
> > --- a/tools/testing/kunit/kunit_tool_test.py
> > +++ b/tools/testing/kunit/kunit_tool_test.py
> > @@ -181,8 +181,7 @@ class KUnitParserTest(unittest.TestCase):
> >                         result = kunit_parser.parse_run_tests(
> >                                 kunit_parser.extract_tap_lines(
> >                                 file.readlines()), stdout)
> > -               # A missing test plan is not an error.
> > -               self.assertEqual(result.counts, kunit_parser.TestCounts(passed=10, errors=0))
> > +               self.assertEqual(result.counts, kunit_parser.TestCounts(passed=10, errors=2))
> >                 self.assertEqual(kunit_parser.TestStatus.SUCCESS, result.status)
> >
> >         def test_no_tests(self):
> > @@ -203,7 +202,7 @@ class KUnitParserTest(unittest.TestCase):
> >                 self.assertEqual(
> >                         kunit_parser.TestStatus.NO_TESTS,
> >                         result.subtests[0].subtests[0].status)
> > -               self.assertEqual(result.counts, kunit_parser.TestCounts(passed=1, errors=1))
> > +               self.assertEqual(result.counts, kunit_parser.TestCounts(passed=1, errors=2))
> >
> >
> >         def test_no_kunit_output(self):
> >
> > base-commit: 0619a4868fc1b32b07fb9ed6c69adc5e5cf4e4b2
> > --
> > 2.48.1.711.g2feabab25a-goog
> >