| Message ID | 20240213154416.422739-4-kuba@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: kselftest_harness: support using xfail | expand |
On Tue, Feb 13, 2024 at 07:44:15AM -0800, Jakub Kicinski wrote: > Selftest summary includes XFAIL but there's no way to use > it from within the harness. Support it in a similar way to skip. > > Currently tests report skip for things they expect to fail > e.g. when given combination of parameters is known to be unsupported. > This is confusing because in an ideal environment and fully featured > kernel no tests should be skipped. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> This is great -- I've been wanting an XFAIL for a while for harness users but I didn't have the imagination to find a way to make it work. :) Acked-by: Kees Cook <keescook@chromium.org>
On Tue, Feb 13, 2024 at 07:44 AM -08, Jakub Kicinski wrote: > Selftest summary includes XFAIL but there's no way to use > it from within the harness. Support it in a similar way to skip. > > Currently tests report skip for things they expect to fail > e.g. when given combination of parameters is known to be unsupported. > This is confusing because in an ideal environment and fully featured > kernel no tests should be skipped. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > tools/testing/selftests/kselftest_harness.h | 37 +++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h > index 618b41eac749..561a817117f9 100644 > --- a/tools/testing/selftests/kselftest_harness.h > +++ b/tools/testing/selftests/kselftest_harness.h > @@ -141,6 +141,33 @@ > statement; \ > } while (0) > > +/** > + * XFAIL() > + * > + * @statement: statement to run after reporting XFAIL > + * @fmt: format string > + * @...: optional arguments > + * > + * .. code-block:: c > + * > + * XFAIL(statement, fmt, ...); > + * > + * This forces a "pass" after reporting why something is expected to fail, > + * and runs "statement", which is usually "return" or "goto skip". > + */ > +#define XFAIL(statement, fmt, ...) do { \ > + snprintf(_metadata->results->reason, \ > + sizeof(_metadata->results->reason), fmt, ##__VA_ARGS__); \ > + if (TH_LOG_ENABLED) { \ > + fprintf(TH_LOG_STREAM, "# XFAIL %s\n", \ > + _metadata->results->reason); \ > + } \ > + _metadata->passed = 1; \ > + _metadata->xfail = 1; \ > + _metadata->trigger = 0; \ > + statement; \ > +} while (0) > + > /** > * TEST() - Defines the test function and creates the registration > * stub > @@ -834,6 +861,7 @@ struct __test_metadata { > int termsig; > int passed; > int skip; /* did SKIP get used? */ > + int xfail; /* did XFAIL get used? */ > int trigger; /* extra handler after the evaluation */ > int timeout; /* seconds to wait for test timeout */ > bool timed_out; /* did this test timeout instead of exiting? */ > @@ -941,6 +969,9 @@ void __wait_for_test(struct __test_metadata *t) > /* SKIP */ > t->passed = 1; > t->skip = 1; > + } else if (WEXITSTATUS(status) == KSFT_XFAIL) { > + t->passed = 1; > + t->xfail = 1; > } else if (t->termsig != -1) { > t->passed = 0; > fprintf(TH_LOG_STREAM, > @@ -1112,6 +1143,7 @@ void __run_test(struct __fixture_metadata *f, > /* reset test struct */ > t->passed = 1; > t->skip = 0; > + t->xfail = 0; > t->trigger = 0; > t->no_print = 0; > memset(t->results->reason, 0, sizeof(t->results->reason)); > @@ -1133,6 +1165,8 @@ void __run_test(struct __fixture_metadata *f, > t->fn(t, variant); > if (t->skip) > _exit(KSFT_SKIP); > + if (t->xfail) > + _exit(KSFT_XFAIL); > if (t->passed) > _exit(KSFT_PASS); > /* Something else happened. */ > @@ -1146,6 +1180,9 @@ void __run_test(struct __fixture_metadata *f, > if (t->skip) > ksft_test_result_skip("%s\n", t->results->reason[0] ? > t->results->reason : "unknown"); > + else if (t->xfail) > + ksft_test_result_xfail("%s\n", t->results->reason[0] ? > + t->results->reason : "unknown"); > else > ksft_test_result(t->passed, "%s%s%s.%s\n", > f->name, variant->name[0] ? "." : "", variant->name, t->name); On second thought, if I can suggest a follow up change so this: ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT ... becomes this ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT You see, we parse test results if they are in TAP format. Lack of test name for xfail'ed and skip'ed tests makes it difficult to report in CI which subtest was it. Happy to contribute it, once this series gets applied. A quick 'n dirty change could look like below. Open to better ideas. ---8<--- diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h index a781e6311810..b73985df9cb9 100644 --- a/tools/testing/selftests/kselftest.h +++ b/tools/testing/selftests/kselftest.h @@ -211,7 +211,8 @@ static inline __printf(1, 2) void ksft_test_result_fail(const char *msg, ...) ksft_test_result_fail(fmt, ##__VA_ARGS__);\ } while (0) -static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...) +static inline __printf(2, 3) void ksft_test_result_xfail(const char *test_name, + const char *msg, ...) { int saved_errno = errno; va_list args; @@ -219,7 +220,7 @@ static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...) ksft_cnt.ksft_xfail++; va_start(args, msg); - printf("ok %u # XFAIL ", ksft_test_num()); + printf("ok %u %s # XFAIL ", ksft_test_num(), test_name); errno = saved_errno; vprintf(msg, args); va_end(args); diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 561a817117f9..2db647f98abc 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -56,6 +56,7 @@ #include <asm/types.h> #include <ctype.h> #include <errno.h> +#include <limits.h> #include <stdbool.h> #include <stdint.h> #include <stdio.h> @@ -1140,6 +1141,8 @@ void __run_test(struct __fixture_metadata *f, struct __fixture_variant_metadata *variant, struct __test_metadata *t) { + char test_name[LINE_MAX]; + /* reset test struct */ t->passed = 1; t->skip = 0; @@ -1149,8 +1152,9 @@ void __run_test(struct __fixture_metadata *f, memset(t->results->reason, 0, sizeof(t->results->reason)); t->results->step = 1; - ksft_print_msg(" RUN %s%s%s.%s ...\n", - f->name, variant->name[0] ? "." : "", variant->name, t->name); + snprintf(test_name, sizeof(test_name), "%s%s%s.%s", + f->name, variant->name[0] ? "." : "", variant->name, t->name); + ksft_print_msg(" RUN %s ...\n", test_name); /* Make sure output buffers are flushed before fork */ fflush(stdout); @@ -1174,18 +1178,16 @@ void __run_test(struct __fixture_metadata *f, } else { __wait_for_test(t); } - ksft_print_msg(" %4s %s%s%s.%s\n", t->passed ? "OK" : "FAIL", - f->name, variant->name[0] ? "." : "", variant->name, t->name); + ksft_print_msg(" %4s %s\n", t->passed ? "OK" : "FAIL", test_name); if (t->skip) ksft_test_result_skip("%s\n", t->results->reason[0] ? t->results->reason : "unknown"); else if (t->xfail) - ksft_test_result_xfail("%s\n", t->results->reason[0] ? + ksft_test_result_xfail(test_name, "%s\n", t->results->reason[0] ? t->results->reason : "unknown"); else - ksft_test_result(t->passed, "%s%s%s.%s\n", - f->name, variant->name[0] ? "." : "", variant->name, t->name); + ksft_test_result(t->passed, "%s\n", test_name); } static int test_harness_run(int argc, char **argv) diff --git a/tools/testing/selftests/mm/mremap_test.c b/tools/testing/selftests/mm/mremap_test.c index 2f8b991f78cb..0abab3b32c88 100644 --- a/tools/testing/selftests/mm/mremap_test.c +++ b/tools/testing/selftests/mm/mremap_test.c @@ -575,8 +575,7 @@ static void run_mremap_test_case(struct test test_case, int *failures, if (remap_time < 0) { if (test_case.expect_failure) - ksft_test_result_xfail("%s\n\tExpected mremap failure\n", - test_case.name); + ksft_test_result_xfail(test_case.name, "\n\tExpected mremap failure\n"); else { ksft_test_result_fail("%s\n", test_case.name); *failures += 1; diff --git a/tools/testing/selftests/net/tcp_ao/key-management.c b/tools/testing/selftests/net/tcp_ao/key-management.c index 24e62120b792..928f067513da 100644 --- a/tools/testing/selftests/net/tcp_ao/key-management.c +++ b/tools/testing/selftests/net/tcp_ao/key-management.c @@ -123,8 +123,8 @@ static void try_delete_key(char *tst_name, int sk, uint8_t sndid, uint8_t rcvid, return; } if (err && fault(FIXME)) { - test_xfail("%s: failed to delete the key %u:%u %d", - tst_name, sndid, rcvid, err); + test_xfail(tst_name, "failed to delete the key %u:%u %d", + sndid, rcvid, err); return; } if (!err) { @@ -283,8 +283,7 @@ static void assert_no_current_rnext(const char *tst_msg, int sk) errno = 0; if (ao_info.set_current || ao_info.set_rnext) { - test_xfail("%s: the socket has current/rnext keys: %d:%d", - tst_msg, + test_xfail(tst_msg, "the socket has current/rnext keys: %d:%d", (ao_info.set_current) ? ao_info.current_key : -1, (ao_info.set_rnext) ? ao_info.rnext : -1); } else { diff --git a/tools/testing/selftests/net/tcp_ao/lib/aolib.h b/tools/testing/selftests/net/tcp_ao/lib/aolib.h index fbc7f6111815..0d6f33b51758 100644 --- a/tools/testing/selftests/net/tcp_ao/lib/aolib.h +++ b/tools/testing/selftests/net/tcp_ao/lib/aolib.h @@ -59,8 +59,8 @@ static inline void __test_print(void (*fn)(const char *), const char *fmt, ...) __test_print(__test_ok, fmt "\n", ##__VA_ARGS__) #define test_skip(fmt, ...) \ __test_print(__test_skip, fmt "\n", ##__VA_ARGS__) -#define test_xfail(fmt, ...) \ - __test_print(__test_xfail, fmt "\n", ##__VA_ARGS__) +#define test_xfail(test_name, fmt, ...) \ + __test_print(__test_xfail, test_name, fmt "\n", ##__VA_ARGS__) #define test_fail(fmt, ...) \ do { \
On Wed, Feb 14, 2024 at 08:40 PM +01, Jakub Sitnicki wrote: [...] > On second thought, if I can suggest a follow up change so this: > > ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > ... becomes this > > ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > You see, we parse test results if they are in TAP format. Lack of test > name for xfail'ed and skip'ed tests makes it difficult to report in CI > which subtest was it. Happy to contribute it, once this series gets > applied. Should have said "harder", not "difficult". That was an overstatement. Test name can be extracted from diagnostic lines preceeding the status. # RUN ip_local_port_range.ip4_stcp.late_bind ... # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT # OK ip_local_port_range.ip4_stcp.late_bind ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT It just makes the TAP parser easier if the test name is included on the status line. That would be the motivation here. Let me know what you think. [...]
On Wed, 14 Feb 2024 22:46:46 +0100 Jakub Sitnicki wrote: > > On second thought, if I can suggest a follow up change so this: > > > > ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > > > ... becomes this > > > > ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > > > You see, we parse test results if they are in TAP format. Lack of test > > name for xfail'ed and skip'ed tests makes it difficult to report in CI > > which subtest was it. Happy to contribute it, once this series gets > > applied. > > Should have said "harder", not "difficult". That was an overstatement. > > Test name can be extracted from diagnostic lines preceeding the status. > > # RUN ip_local_port_range.ip4_stcp.late_bind ... > # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > # OK ip_local_port_range.ip4_stcp.late_bind > ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > It just makes the TAP parser easier if the test name is included on the > status line. That would be the motivation here. Let me know what you > think. Good catch, I just copied what we do for skip and completely missed this. As you said we'd report: ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT and I think that's sort of closer to valid TAP than to valid KTAP which always mentions test/test_case_name: https://docs.kernel.org/dev-tools/ktap.html We currently do the same thing for SKIP, e.g.: # RUN ip_local_port_range.ip4_stcp.late_bind ... # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT # OK ip_local_port_range.ip4_stcp.late_bind ok 17 # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT I'm not sure if we can realistically do surgery on the existing print helpers to add the test_name, because: $ git grep 'ksft_test_result_*' | wc -l 915 That'd be a cruel patch to send. But I do agree that adding the test_name to the prototype is a good move, to avoid others making the same mistake. Should we introduce a new set of helpers which take the extra arg and call them ksft_test_report_*() instead of ksft_test_result_*() ? Maybe we're overthinking and a local fix in the harness is enough. Kees, WDYT?
On Wed, Feb 14, 2024 at 04:25:14PM -0800, Jakub Kicinski wrote: > On Wed, 14 Feb 2024 22:46:46 +0100 Jakub Sitnicki wrote: > > > On second thought, if I can suggest a follow up change so this: > > > > > > ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > > > > > ... becomes this > > > > > > ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > > > > > You see, we parse test results if they are in TAP format. Lack of test > > > name for xfail'ed and skip'ed tests makes it difficult to report in CI > > > which subtest was it. Happy to contribute it, once this series gets > > > applied. > > > > Should have said "harder", not "difficult". That was an overstatement. > > > > Test name can be extracted from diagnostic lines preceeding the status. > > > > # RUN ip_local_port_range.ip4_stcp.late_bind ... > > # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > # OK ip_local_port_range.ip4_stcp.late_bind > > ok 17 ip_local_port_range.ip4_stcp.late_bind # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > > > It just makes the TAP parser easier if the test name is included on the > > status line. That would be the motivation here. Let me know what you > > think. > > Good catch, I just copied what we do for skip and completely missed > this. As you said we'd report: > > ok 17 # XFAIL SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > and I think that's sort of closer to valid TAP than to valid KTAP > which always mentions test/test_case_name: > > https://docs.kernel.org/dev-tools/ktap.html > > We currently do the same thing for SKIP, e.g.: > > # RUN ip_local_port_range.ip4_stcp.late_bind ... > # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > # OK ip_local_port_range.ip4_stcp.late_bind > ok 17 # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT > > I'm not sure if we can realistically do surgery on the existing print > helpers to add the test_name, because: > > $ git grep 'ksft_test_result_*' | wc -l > 915 > > That'd be a cruel patch to send. > > But I do agree that adding the test_name to the prototype is a good > move, to avoid others making the same mistake. Should we introduce > a new set of helpers which take the extra arg and call them > ksft_test_report_*() instead of ksft_test_result_*() ? > > Maybe we're overthinking and a local fix in the harness is enough. > > Kees, WDYT? Yeah, let's separate this fix-up from the addition of the XFAIL logic.
On Tue, Feb 13, 2024 at 07:44:15AM -0800, Jakub Kicinski wrote: > [...] > +/** > + * XFAIL() > + * > + * @statement: statement to run after reporting XFAIL > + * @fmt: format string > + * @...: optional arguments > + * > + * .. code-block:: c > + * > + * XFAIL(statement, fmt, ...); > + * > + * This forces a "pass" after reporting why something is expected to fail, > + * and runs "statement", which is usually "return" or "goto skip". > + */ > +#define XFAIL(statement, fmt, ...) do { \ > + snprintf(_metadata->results->reason, \ > + sizeof(_metadata->results->reason), fmt, ##__VA_ARGS__); \ > + if (TH_LOG_ENABLED) { \ > + fprintf(TH_LOG_STREAM, "# XFAIL %s\n", \ Oh! I just noticed this while testing changes to use XFAIL, there is an alignment issue: one too many spaces after "XFAIL" above, which leads to misaligned output. fprintf(TH_LOG_STREAM, "# XFAIL %s\n", \ fprintf(TH_LOG_STREAM, "# SKIP %s\n", \ Compare the position of "%s" above...
On Thu, 15 Feb 2024 14:06:58 -0800 Kees Cook wrote: > Oh! I just noticed this while testing changes to use XFAIL, there is an > alignment issue: one too many spaces after "XFAIL" above, which leads > to misaligned output. > > fprintf(TH_LOG_STREAM, "# XFAIL %s\n", \ > fprintf(TH_LOG_STREAM, "# SKIP %s\n", \ > > Compare the position of "%s" above... Ah, sadness. Thanks for catching. Let me fix, look this over again, and perhaps add the new helpers Jakub S suggested.
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 618b41eac749..561a817117f9 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -141,6 +141,33 @@ statement; \ } while (0) +/** + * XFAIL() + * + * @statement: statement to run after reporting XFAIL + * @fmt: format string + * @...: optional arguments + * + * .. code-block:: c + * + * XFAIL(statement, fmt, ...); + * + * This forces a "pass" after reporting why something is expected to fail, + * and runs "statement", which is usually "return" or "goto skip". + */ +#define XFAIL(statement, fmt, ...) do { \ + snprintf(_metadata->results->reason, \ + sizeof(_metadata->results->reason), fmt, ##__VA_ARGS__); \ + if (TH_LOG_ENABLED) { \ + fprintf(TH_LOG_STREAM, "# XFAIL %s\n", \ + _metadata->results->reason); \ + } \ + _metadata->passed = 1; \ + _metadata->xfail = 1; \ + _metadata->trigger = 0; \ + statement; \ +} while (0) + /** * TEST() - Defines the test function and creates the registration * stub @@ -834,6 +861,7 @@ struct __test_metadata { int termsig; int passed; int skip; /* did SKIP get used? */ + int xfail; /* did XFAIL get used? */ int trigger; /* extra handler after the evaluation */ int timeout; /* seconds to wait for test timeout */ bool timed_out; /* did this test timeout instead of exiting? */ @@ -941,6 +969,9 @@ void __wait_for_test(struct __test_metadata *t) /* SKIP */ t->passed = 1; t->skip = 1; + } else if (WEXITSTATUS(status) == KSFT_XFAIL) { + t->passed = 1; + t->xfail = 1; } else if (t->termsig != -1) { t->passed = 0; fprintf(TH_LOG_STREAM, @@ -1112,6 +1143,7 @@ void __run_test(struct __fixture_metadata *f, /* reset test struct */ t->passed = 1; t->skip = 0; + t->xfail = 0; t->trigger = 0; t->no_print = 0; memset(t->results->reason, 0, sizeof(t->results->reason)); @@ -1133,6 +1165,8 @@ void __run_test(struct __fixture_metadata *f, t->fn(t, variant); if (t->skip) _exit(KSFT_SKIP); + if (t->xfail) + _exit(KSFT_XFAIL); if (t->passed) _exit(KSFT_PASS); /* Something else happened. */ @@ -1146,6 +1180,9 @@ void __run_test(struct __fixture_metadata *f, if (t->skip) ksft_test_result_skip("%s\n", t->results->reason[0] ? t->results->reason : "unknown"); + else if (t->xfail) + ksft_test_result_xfail("%s\n", t->results->reason[0] ? + t->results->reason : "unknown"); else ksft_test_result(t->passed, "%s%s%s.%s\n", f->name, variant->name[0] ? "." : "", variant->name, t->name);
Selftest summary includes XFAIL but there's no way to use it from within the harness. Support it in a similar way to skip. Currently tests report skip for things they expect to fail e.g. when given combination of parameters is known to be unsupported. This is confusing because in an ideal environment and fully featured kernel no tests should be skipped. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- tools/testing/selftests/kselftest_harness.h | 37 +++++++++++++++++++++ 1 file changed, 37 insertions(+)