On Sat, 14 Jan 2023 at 06:07, 'Daniel Latypov' via KUnit Development
<kunit-dev@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> ---
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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230113220718.2901010-1-dlatypov%40google.com.
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
