On Sat, 14 Jan 2023 at 06:07, 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
Currently, kunit_skip() and kunit_mark_skipped() will overwrite the current test's status even if it was already marked FAILED.
E.g. a test that just contains this KUNIT_FAIL(test, "FAIL REASON"); kunit_skip(test, "SKIP REASON"); will be marked "SKIPPED" in the end.
Now, tests like the above don't and shouldn't exist. But what happens if non-test code (e.g. KASAN) calls kunit_fail_current_test()?
E.g. if we have if (do_some_invalid_memory_accesses()) kunit_skip("); then the KASAN failures will get masked!
This patch: make it so kunit_mark_skipped() does not modify the status if it's already set to something (either already to SKIPPED or FAILURE).
Before this change, the KTAP output would look like # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:23 FAIL REASON ok 1 example_simple_test # SKIP SKIP REASON
After this change: # example_simple_test: EXPECTATION FAILED at lib/kunit/kunit-example-test.c:23 FAIL REASON # example_simple_test: status already changed, not marking skipped: SKIP REASON not ok 1 example_simple_test
Signed-off-by: Daniel Latypov dlatypov@google.com
Thanks very much: this makes much more sense than the old behaviour.
My only suggestion is that we add a test to verify this behaviour to the kunit_status suite, such as: diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c index 4df0335d0d06..fa114785b01e 100644 --- a/lib/kunit/kunit-test.c +++ b/lib/kunit/kunit-test.c @@ -510,9 +510,33 @@ static void kunit_status_mark_skipped_test(struct kunit *test) KUNIT_EXPECT_STREQ(test, fake.status_comment, "Accepts format string: YES"); }
+static void kunit_status_skip_after_fail_test(struct kunit *test) +{ + struct kunit fake; + + kunit_init_test(&fake, "fake test", NULL); + + /* Test starts off SUCCESS. */ + KUNIT_EXPECT_EQ(test, fake.status, KUNIT_SUCCESS); + + /* Fail the test. */ + kunit_set_failure(&fake); + KUNIT_EXPECT_EQ(test, fake.status, KUNIT_FAILURE); + + /* Now mark it as skipped. */ + kunit_mark_skipped(&fake, "Skip message"); + + /* The test has still failed. */ + KUNIT_EXPECT_EQ(test, fake.status, KUNIT_FAILURE); + + /* We shouldn't use the skip reason as a status comment. */ + KUNIT_EXPECT_STREQ(test, fake.status_comment, ""); +} + static struct kunit_case kunit_status_test_cases[] = { KUNIT_CASE(kunit_status_set_failure_test), KUNIT_CASE(kunit_status_mark_skipped_test), + KUNIT_CASE(kunit_status_skip_after_fail_test), {} };
--
Otherwise, this looks great!
Reviewed-by: David Gow davidgow@google.com
Cheers, -- David
include/kunit/test.h | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/include/kunit/test.h b/include/kunit/test.h index 87ea90576b50..39936463dde5 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -386,11 +386,18 @@ void __printf(2, 3) kunit_log_append(char *log, const char *fmt, ...);
- Marks the test as skipped. @fmt is given output as the test status
- comment, typically the reason the test was skipped.
*/
- This has no effect if the test has already been marked skipped or failed.
- Test execution continues after kunit_mark_skipped() is called.
#define kunit_mark_skipped(test_or_suite, fmt, ...) \ do { \
if (READ_ONCE((test_or_suite)->status) != KUNIT_SUCCESS) {\kunit_warn(test_or_suite, "status already " \"changed, not marking skipped: " fmt,\##__VA_ARGS__); \break; \} \ WRITE_ONCE((test_or_suite)->status, KUNIT_SKIPPED); \ scnprintf((test_or_suite)->status_comment, \ KUNIT_STATUS_COMMENT_SIZE, \base-commit: 7dd4b804e08041ff56c88bdd8da742d14b17ed25
2.39.0.314.g84b9a713c41-goog
-- You received this message because you are subscribed to the Google Groups "KUnit Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230113220718.2901010-1-dlatypo....