kunit: kunit_skip() should not overwrite KUNIT_FAIL()

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>
---
 include/kunit/test.h | 7 +++++++
 1 file changed, 7 insertions(+)


base-commit: 7dd4b804e08041ff56c88bdd8da742d14b17ed25

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-dlatypov%40google.com.