[PATCH] kunit: kunit_skip() should not overwrite KUNIT_FAIL()

[Daniel Latypov]

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 <dlat...@google.com>
---
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

[David Gow]

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 <davi...@google.com>

Cheers,
-- David

> --
> 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+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230113220718.2901010-1-dlatypov%40google.com.