On Wed, Apr 27, 2022 at 1:33 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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 <dlatypov@xxxxxxxxxx> > --- 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 <davidgow@xxxxxxxxxx> -- 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 >
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
