Consider this invocation
$ ./tools/testing/kunit/kunit.py parse <<EOF
TAP version 14
1..2
ok 1 - suite
# Subtest: no_tests_suite
# catastrophic error!
not ok 1 - no_tests_suite
EOF
It will have a 0 exit code even though there's a "not ok".
Consider this one:
$ ./tools/testing/kunit/kunit.py parse <<EOF
TAP version 14
1..2
ok 1 - suite
not ok 1 - no_tests_suite
EOF
It will a non-zero exit code.
Why?
We have this line in the kunit_parser.py
> parent_test = parse_test_header(lines, test)
where we have special handling when we see "# Subtest" and we ignore the
explicit reported "not ok 1" status!
Also, NO_TESTS at a suite-level only results in a non-zero status code
where then there's only one suite atm.
This change is the minimal one to make sure we don't overwrite it.
Signed-off-by: Daniel Latypov <[email protected]>
---
tools/testing/kunit/kunit_parser.py | 7 +++++--
.../test_data/test_is_test_passed-no_tests_no_plan.log | 2 +-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 7a0faf527a98..45c2c5837281 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -775,8 +775,11 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test:
# Check for there being no tests
if parent_test and len(subtests) == 0:
- test.status = TestStatus.NO_TESTS
- test.add_error('0 tests run!')
+ # Don't override a bad status if this test had one reported.
+ # Assumption: no subtests means CRASHED is from Test.__init__()
+ if test.status in (TestStatus.TEST_CRASHED, TestStatus.SUCCESS):
+ test.status = TestStatus.NO_TESTS
+ test.add_error('0 tests run!')
# Add statuses to TestCounts attribute in Test object
bubble_up_test_results(test)
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-no_tests_no_plan.log b/tools/testing/kunit/test_data/test_is_test_passed-no_tests_no_plan.log
index dd873c981108..4f81876ee6f1 100644
--- a/tools/testing/kunit/test_data/test_is_test_passed-no_tests_no_plan.log
+++ b/tools/testing/kunit/test_data/test_is_test_passed-no_tests_no_plan.log
@@ -3,5 +3,5 @@ TAP version 14
# Subtest: suite
1..1
# Subtest: case
- ok 1 - case # SKIP
+ ok 1 - case
ok 1 - suite
--
2.36.0.rc2.479.g8af0fa9b8e-goog
On Wed, Apr 27, 2022 at 1:33 AM Daniel Latypov <[email protected]> wrote:
>
> Consider this invocation
> $ ./tools/testing/kunit/kunit.py parse <<EOF
> TAP version 14
> 1..2
> ok 1 - suite
> # Subtest: no_tests_suite
> # catastrophic error!
> not ok 1 - no_tests_suite
> EOF
>
> It will have a 0 exit code even though there's a "not ok".
>
> Consider this one:
> $ ./tools/testing/kunit/kunit.py parse <<EOF
> TAP version 14
> 1..2
> ok 1 - suite
> not ok 1 - no_tests_suite
> EOF
>
> It will a non-zero exit code.
>
> Why?
> We have this line in the kunit_parser.py
> > parent_test = parse_test_header(lines, test)
> where we have special handling when we see "# Subtest" and we ignore the
> explicit reported "not ok 1" status!
>
> Also, NO_TESTS at a suite-level only results in a non-zero status code
> where then there's only one suite atm.
>
> This change is the minimal one to make sure we don't overwrite it.
>
> Signed-off-by: Daniel Latypov <[email protected]>
> ---
This seems sensible to me, though it doesn't change a lot in practice
given that the in-kernel KUnit will mark empty suites as skipped:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/kunit/test.c#n180
(And I think that is probably the correct thing for it to do, as "no
tests run" seems closer to skipped than to passed or failed, which are
the other options KTAP gives us.)
That being said, it's definitely true that we don't want to override a
"not ok" needlessly in kunit_tool, so this patch is definite
improvement.
Reviewed-by: David Gow <[email protected]>
-- David
> tools/testing/kunit/kunit_parser.py | 7 +++++--
> .../test_data/test_is_test_passed-no_tests_no_plan.log | 2 +-
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index 7a0faf527a98..45c2c5837281 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -775,8 +775,11 @@ def parse_test(lines: LineStream, expected_num: int, log: List[str]) -> Test:
>
> # Check for there being no tests
> if parent_test and len(subtests) == 0:
> - test.status = TestStatus.NO_TESTS
> - test.add_error('0 tests run!')
> + # Don't override a bad status if this test had one reported.
> + # Assumption: no subtests means CRASHED is from Test.__init__()
> + if test.status in (TestStatus.TEST_CRASHED, TestStatus.SUCCESS):
> + test.status = TestStatus.NO_TESTS
> + test.add_error('0 tests run!')
>
> # Add statuses to TestCounts attribute in Test object
> bubble_up_test_results(test)
> diff --git a/tools/testing/kunit/test_data/test_is_test_passed-no_tests_no_plan.log b/tools/testing/kunit/test_data/test_is_test_passed-no_tests_no_plan.log
> index dd873c981108..4f81876ee6f1 100644
> --- a/tools/testing/kunit/test_data/test_is_test_passed-no_tests_no_plan.log
> +++ b/tools/testing/kunit/test_data/test_is_test_passed-no_tests_no_plan.log
> @@ -3,5 +3,5 @@ TAP version 14
> # Subtest: suite
> 1..1
> # Subtest: case
> - ok 1 - case # SKIP
> + ok 1 - case
> ok 1 - suite
> --
> 2.36.0.rc2.479.g8af0fa9b8e-goog
>
On Tue, Apr 26, 2022 at 1:33 PM Daniel Latypov <[email protected]> wrote:
>
> Consider this invocation
> $ ./tools/testing/kunit/kunit.py parse <<EOF
> TAP version 14
> 1..2
> ok 1 - suite
> # Subtest: no_tests_suite
> # catastrophic error!
> not ok 1 - no_tests_suite
> EOF
>
> It will have a 0 exit code even though there's a "not ok".
>
> Consider this one:
> $ ./tools/testing/kunit/kunit.py parse <<EOF
> TAP version 14
> 1..2
> ok 1 - suite
> not ok 1 - no_tests_suite
> EOF
>
> It will a non-zero exit code.
>
> Why?
> We have this line in the kunit_parser.py
> > parent_test = parse_test_header(lines, test)
> where we have special handling when we see "# Subtest" and we ignore the
> explicit reported "not ok 1" status!
>
> Also, NO_TESTS at a suite-level only results in a non-zero status code
> where then there's only one suite atm.
>
> This change is the minimal one to make sure we don't overwrite it.
>
> Signed-off-by: Daniel Latypov <[email protected]>
Reviewed-by: Brendan Higgins <[email protected]>