* [PATCH] kunit: kunit_skip() should not overwrite KUNIT_FAIL()
@ 2023-01-13 22:07 Daniel Latypov
2023-01-14 5:51 ` David Gow
0 siblings, 1 reply; 2+ messages in thread
From: Daniel Latypov @ 2023-01-13 22:07 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: rmoar, linux-kernel, kunit-dev, linux-kselftest, skhan, 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 <dlatypov@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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] kunit: kunit_skip() should not overwrite KUNIT_FAIL()
2023-01-13 22:07 [PATCH] kunit: kunit_skip() should not overwrite KUNIT_FAIL() Daniel Latypov
@ 2023-01-14 5:51 ` David Gow
0 siblings, 0 replies; 2+ messages in thread
From: David Gow @ 2023-01-14 5:51 UTC (permalink / raw)
To: Daniel Latypov
Cc: brendanhiggins, rmoar, linux-kernel, kunit-dev, linux-kselftest, skhan
[-- Attachment #1: Type: text/plain, Size: 4771 bytes --]
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.
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-01-14 5:52 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-13 22:07 [PATCH] kunit: kunit_skip() should not overwrite KUNIT_FAIL() Daniel Latypov
2023-01-14 5:51 ` David Gow
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.