Add a common result printing helper and always include test name in the result line. Previously when SKIP or XPASS would happen we printed:
ok 1 # SKIP unknown
without the test name. Now we'll print:
ok 1 global.no_pad # SKIP unknown
This appears to be more inline with: https://docs.kernel.org/dev-tools/ktap.html and makes parsing results easier.
First 3 patches rearrange kselftest_harness to use exit code as an enum rather than separate passed/skip/xfail members.
Rest of the series builds a ksft_test_result_code() helper.
This series is on top of: https://lore.kernel.org/all/20240216002619.1999225-1-kuba@kernel.org/
Jakub Kicinski (7): selftests: kselftest_harness: generate test name once selftests: kselftest_harness: save full exit code in metadata selftests: kselftest_harness: use exit code to store skip and xfail selftests: kselftest: add ksft_test_result_code(), handling all exit codes selftests: kselftest_harness: print test name for SKIP and XFAIL selftests: kselftest_harness: let ksft_test_result_code() handle line termination selftests: kselftest_harness: let PASS / FAIL provide diagnostic
tools/testing/selftests/kselftest.h | 45 ++++++++++ tools/testing/selftests/kselftest_harness.h | 96 ++++++++++----------- tools/testing/selftests/net/tls.c | 2 +- 3 files changed, 91 insertions(+), 52 deletions(-)
Since we added variant support generating full test case name takes 4 string arguments. We're about to need it in another two places. Stop the duplication and print once into a temporary buffer.
Suggested-by: Jakub Sitnicki jakub@cloudflare.com Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest_harness.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 04177813930b..b271cb721b81 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,10 @@ 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,8 +1179,8 @@ 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] ? @@ -1184,8 +1189,7 @@ void __run_test(struct __fixture_metadata *f, 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); + ksft_test_result(t->passed, "%s\n", test_name); }
static int test_harness_run(int argc, char **argv)
On Thu, Feb 15, 2024 at 04:41:16PM -0800, Jakub Kicinski wrote:
Since we added variant support generating full test case name takes 4 string arguments. We're about to need it in another two places. Stop the duplication and print once into a temporary buffer.
Suggested-by: Jakub Sitnicki jakub@cloudflare.com Signed-off-by: Jakub Kicinski kuba@kernel.org
Clean refactoring -- makes this much more readable.
Acked-by: Kees Cook keescook@chromium.org
Instead of tracking passed = 0/1 rename the field to exit_code and invert the values so that they match the KSFT_* exit codes. This will allow us to fold SKIP / XFAIL into the same value.
Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest_harness.h | 52 ++++++++++++--------- tools/testing/selftests/net/tls.c | 2 +- 2 files changed, 30 insertions(+), 24 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index b271cb721b81..70366864ffd9 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -136,7 +136,7 @@ fprintf(TH_LOG_STREAM, "# SKIP %s\n", \ _metadata->results->reason); \ } \ - _metadata->passed = 1; \ + _metadata->exit_code = KSFT_PASS; \ _metadata->skip = 1; \ _metadata->trigger = 0; \ statement; \ @@ -163,7 +163,7 @@ fprintf(TH_LOG_STREAM, "# XFAIL %s\n", \ _metadata->results->reason); \ } \ - _metadata->passed = 1; \ + _metadata->exit_code = KSFT_PASS; \ _metadata->xfail = 1; \ _metadata->trigger = 0; \ statement; \ @@ -416,7 +416,7 @@ if (setjmp(_metadata->env) == 0) { \ fixture_name##_setup(_metadata, &self, variant->data); \ /* Let setup failure terminate early. */ \ - if (!_metadata->passed || _metadata->skip) \ + if (!__test_passed(_metadata) || _metadata->skip) \ return; \ _metadata->setup_completed = true; \ fixture_name##_##test_name(_metadata, &self, variant->data); \ @@ -723,7 +723,7 @@ __bail(_assert, _metadata))
#define __INC_STEP(_metadata) \ - if (_metadata->passed) \ + if (__test_passed(_metadata)) \ _metadata->results->step++;
#define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1)) @@ -769,7 +769,7 @@ break; \ } \ } \ - _metadata->passed = 0; \ + _metadata->exit_code = KSFT_FAIL; \ /* Ensure the optional handler is triggered */ \ _metadata->trigger = 1; \ } \ @@ -781,7 +781,7 @@ if (_assert) __INC_STEP(_metadata); \ if (!(strcmp(__exp, __seen) _t 0)) { \ __TH_LOG("Expected '%s' %s '%s'.", __exp, #_t, __seen); \ - _metadata->passed = 0; \ + _metadata->exit_code = KSFT_FAIL; \ _metadata->trigger = 1; \ } \ } while (0); OPTIONAL_HANDLER(_assert) @@ -860,7 +860,7 @@ struct __test_metadata { pid_t pid; /* pid of test when being run */ struct __fixture_metadata *fixture; int termsig; - int passed; + int exit_code; int skip; /* did SKIP get used? */ int xfail; /* did XFAIL get used? */ int trigger; /* extra handler after the evaluation */ @@ -874,6 +874,12 @@ struct __test_metadata { struct __test_metadata *prev, *next; };
+static inline bool __test_passed(struct __test_metadata *metadata) +{ + return metadata->exit_code != KSFT_FAIL && + metadata->exit_code <= KSFT_SKIP; +} + /* * Since constructors are called in reverse order, reverse the test * list so tests are run in source declaration order. @@ -941,7 +947,7 @@ void __wait_for_test(struct __test_metadata *t) int status;
if (sigaction(SIGALRM, &action, &saved_action)) { - t->passed = 0; + t->exit_code = KSFT_FAIL; fprintf(TH_LOG_STREAM, "# %s: unable to install SIGALRM handler\n", t->name); @@ -953,7 +959,7 @@ void __wait_for_test(struct __test_metadata *t) waitpid(t->pid, &status, 0); alarm(0); if (sigaction(SIGALRM, &saved_action, NULL)) { - t->passed = 0; + t->exit_code = KSFT_FAIL; fprintf(TH_LOG_STREAM, "# %s: unable to uninstall SIGALRM handler\n", t->name); @@ -962,19 +968,19 @@ void __wait_for_test(struct __test_metadata *t) __active_test = NULL;
if (t->timed_out) { - t->passed = 0; + t->exit_code = KSFT_FAIL; fprintf(TH_LOG_STREAM, "# %s: Test terminated by timeout\n", t->name); } else if (WIFEXITED(status)) { if (WEXITSTATUS(status) == KSFT_SKIP) { /* SKIP */ - t->passed = 1; + t->exit_code = KSFT_PASS; t->skip = 1; } else if (WEXITSTATUS(status) == KSFT_XFAIL) { - t->passed = 1; + t->exit_code = KSFT_PASS; t->xfail = 1; } else if (t->termsig != -1) { - t->passed = 0; + t->exit_code = KSFT_FAIL; fprintf(TH_LOG_STREAM, "# %s: Test exited normally instead of by signal (code: %d)\n", t->name, @@ -983,11 +989,11 @@ void __wait_for_test(struct __test_metadata *t) switch (WEXITSTATUS(status)) { /* Success */ case KSFT_PASS: - t->passed = 1; + t->exit_code = KSFT_PASS; break; /* Other failure, assume step report. */ default: - t->passed = 0; + t->exit_code = KSFT_FAIL; fprintf(TH_LOG_STREAM, "# %s: Test failed at step #%d\n", t->name, @@ -995,13 +1001,13 @@ void __wait_for_test(struct __test_metadata *t) } } } else if (WIFSIGNALED(status)) { - t->passed = 0; + t->exit_code = KSFT_FAIL; if (WTERMSIG(status) == SIGABRT) { fprintf(TH_LOG_STREAM, "# %s: Test terminated by assertion\n", t->name); } else if (WTERMSIG(status) == t->termsig) { - t->passed = 1; + t->exit_code = KSFT_PASS; } else { fprintf(TH_LOG_STREAM, "# %s: Test terminated unexpectedly by signal %d\n", @@ -1144,7 +1150,7 @@ void __run_test(struct __fixture_metadata *f, char test_name[LINE_MAX];
/* reset test struct */ - t->passed = 1; + t->exit_code = KSFT_PASS; t->skip = 0; t->xfail = 0; t->trigger = 0; @@ -1164,7 +1170,7 @@ void __run_test(struct __fixture_metadata *f, t->pid = fork(); if (t->pid < 0) { ksft_print_msg("ERROR SPAWNING TEST CHILD\n"); - t->passed = 0; + t->exit_code = KSFT_FAIL; } else if (t->pid == 0) { setpgrp(); t->fn(t, variant); @@ -1172,7 +1178,7 @@ void __run_test(struct __fixture_metadata *f, _exit(KSFT_SKIP); if (t->xfail) _exit(KSFT_XFAIL); - if (t->passed) + if (__test_passed(t)) _exit(KSFT_PASS); /* Something else happened. */ _exit(KSFT_FAIL); @@ -1180,7 +1186,7 @@ void __run_test(struct __fixture_metadata *f, __wait_for_test(t); } ksft_print_msg(" %4s %s\n", - t->passed ? "OK" : "FAIL", test_name); + __test_passed(t) ? "OK" : "FAIL", test_name);
if (t->skip) ksft_test_result_skip("%s\n", t->results->reason[0] ? @@ -1189,7 +1195,7 @@ void __run_test(struct __fixture_metadata *f, ksft_test_result_xfail("%s\n", t->results->reason[0] ? t->results->reason : "unknown"); else - ksft_test_result(t->passed, "%s\n", test_name); + ksft_test_result(__test_passed(t), "%s\n", test_name); }
static int test_harness_run(int argc, char **argv) @@ -1237,7 +1243,7 @@ static int test_harness_run(int argc, char **argv) t->results = results; __run_test(f, v, t); t->results = NULL; - if (t->passed) + if (__test_passed(t)) pass_count++; else ret = 1; diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c index 49c84602707f..046d1ccedcf3 100644 --- a/tools/testing/selftests/net/tls.c +++ b/tools/testing/selftests/net/tls.c @@ -1882,7 +1882,7 @@ TEST_F(tls_err, poll_partial_rec_async) pfd.events = POLLIN; EXPECT_EQ(poll(&pfd, 1, 20), 1);
- exit(!_metadata->passed); + exit(!__test_passed(_metadata)); } }
We always use skip / xfail with combination of exit_code being 0 (KSFT_PASS). This are just basic KSFT / KTAP semantics. Store the right KSFT_* code in exit_code directly.
Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest_harness.h | 35 ++++++--------------- 1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 70366864ffd9..6923cd7060fc 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -136,8 +136,7 @@ fprintf(TH_LOG_STREAM, "# SKIP %s\n", \ _metadata->results->reason); \ } \ - _metadata->exit_code = KSFT_PASS; \ - _metadata->skip = 1; \ + _metadata->exit_code = KSFT_SKIP; \ _metadata->trigger = 0; \ statement; \ } while (0) @@ -163,8 +162,7 @@ fprintf(TH_LOG_STREAM, "# XFAIL %s\n", \ _metadata->results->reason); \ } \ - _metadata->exit_code = KSFT_PASS; \ - _metadata->xfail = 1; \ + _metadata->exit_code = KSFT_XFAIL; \ _metadata->trigger = 0; \ statement; \ } while (0) @@ -416,7 +414,7 @@ if (setjmp(_metadata->env) == 0) { \ fixture_name##_setup(_metadata, &self, variant->data); \ /* Let setup failure terminate early. */ \ - if (!__test_passed(_metadata) || _metadata->skip) \ + if (_metadata->exit_code) \ return; \ _metadata->setup_completed = true; \ fixture_name##_##test_name(_metadata, &self, variant->data); \ @@ -861,8 +859,6 @@ struct __test_metadata { struct __fixture_metadata *fixture; int termsig; int exit_code; - 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? */ @@ -972,13 +968,9 @@ void __wait_for_test(struct __test_metadata *t) fprintf(TH_LOG_STREAM, "# %s: Test terminated by timeout\n", t->name); } else if (WIFEXITED(status)) { - if (WEXITSTATUS(status) == KSFT_SKIP) { - /* SKIP */ - t->exit_code = KSFT_PASS; - t->skip = 1; - } else if (WEXITSTATUS(status) == KSFT_XFAIL) { - t->exit_code = KSFT_PASS; - t->xfail = 1; + if (WEXITSTATUS(status) == KSFT_SKIP || + WEXITSTATUS(status) == KSFT_XFAIL) { + t->exit_code = WEXITSTATUS(status); } else if (t->termsig != -1) { t->exit_code = KSFT_FAIL; fprintf(TH_LOG_STREAM, @@ -1151,8 +1143,6 @@ void __run_test(struct __fixture_metadata *f,
/* reset test struct */ t->exit_code = KSFT_PASS; - t->skip = 0; - t->xfail = 0; t->trigger = 0; t->no_print = 0; memset(t->results->reason, 0, sizeof(t->results->reason)); @@ -1174,24 +1164,17 @@ void __run_test(struct __fixture_metadata *f, } else if (t->pid == 0) { setpgrp(); t->fn(t, variant); - if (t->skip) - _exit(KSFT_SKIP); - if (t->xfail) - _exit(KSFT_XFAIL); - if (__test_passed(t)) - _exit(KSFT_PASS); - /* Something else happened. */ - _exit(KSFT_FAIL); + _exit(t->exit_code); } else { __wait_for_test(t); } ksft_print_msg(" %4s %s\n", __test_passed(t) ? "OK" : "FAIL", test_name);
- if (t->skip) + if (t->exit_code == KSFT_SKIP) ksft_test_result_skip("%s\n", t->results->reason[0] ? t->results->reason : "unknown"); - else if (t->xfail) + else if (t->exit_code == KSFT_XFAIL) ksft_test_result_xfail("%s\n", t->results->reason[0] ? t->results->reason : "unknown"); else
For generic test harness code it's more useful to deal with exit codes directly, rather than having to switch on them and call the right ksft_test_result_*() helper. Add such function to kselftest.h.
Note that "directive" and "diagnostic" are what ktap docs call those parts of the message.
Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest.h | 39 +++++++++++++++++++++ tools/testing/selftests/kselftest_harness.h | 14 ++++---- 2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h index a781e6311810..12ad7f8dfe3a 100644 --- a/tools/testing/selftests/kselftest.h +++ b/tools/testing/selftests/kselftest.h @@ -25,6 +25,7 @@ * ksft_test_result_skip(fmt, ...); * ksft_test_result_xfail(fmt, ...); * ksft_test_result_error(fmt, ...); + * ksft_test_result_code(exit_code, test_name, fmt, ...); * * When all tests are finished, clean up and exit the program with one of: * @@ -254,6 +255,44 @@ static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...) va_end(args); }
+static inline __printf(2, 3) +void ksft_test_result_code(int exit_code, const char *msg, ...) +{ + const char *tap_code = "ok"; + const char *directive = ""; + int saved_errno = errno; + va_list args; + + switch (exit_code) { + case KSFT_PASS: + ksft_cnt.ksft_pass++; + break; + case KSFT_XFAIL: + directive = " # XFAIL "; + ksft_cnt.ksft_xfail++; + break; + case KSFT_XPASS: + directive = " # XPASS "; + ksft_cnt.ksft_xpass++; + break; + case KSFT_SKIP: + directive = " # SKIP "; + ksft_cnt.ksft_xskip++; + break; + case KSFT_FAIL: + default: + tap_code = "not ok"; + ksft_cnt.ksft_fail++; + break; + } + + va_start(args, msg); + printf("%s %u%s", tap_code, ksft_test_num(), directive); + errno = saved_errno; + vprintf(msg, args); + va_end(args); +} + static inline int ksft_exit_pass(void) { ksft_print_cnts(); diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 6923cd7060fc..79a3fec0cefa 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -1140,6 +1140,7 @@ void __run_test(struct __fixture_metadata *f, struct __test_metadata *t) { char test_name[LINE_MAX]; + const char *diagnostic;
/* reset test struct */ t->exit_code = KSFT_PASS; @@ -1171,12 +1172,13 @@ void __run_test(struct __fixture_metadata *f, ksft_print_msg(" %4s %s\n", __test_passed(t) ? "OK" : "FAIL", test_name);
- if (t->exit_code == KSFT_SKIP) - ksft_test_result_skip("%s\n", t->results->reason[0] ? - t->results->reason : "unknown"); - else if (t->exit_code == KSFT_XFAIL) - ksft_test_result_xfail("%s\n", t->results->reason[0] ? - t->results->reason : "unknown"); + if (t->results->reason[0]) + diagnostic = t->results->reason; + else + diagnostic = "unknown"; + + if (t->exit_code == KSFT_SKIP || t->exit_code == KSFT_XFAIL) + ksft_test_result_code(t->exit_code, "%s\n", diagnostic); else ksft_test_result(__test_passed(t), "%s\n", test_name); }
Jakub points out that for parsers it's rather useful to always have the test name on the result line. Currently if we SKIP or XFAIL, we will print:
ok 17 # SKIP SCTP doesn't support IP_BIND_ADDRESS_NO_PORT
^ no test name
Always print the test name. KTAP format seems to allow or even call for it, per: https://docs.kernel.org/dev-tools/ktap.html
Suggested-by: Jakub Sitnicki jakub@cloudflare.com Link: https://lore.kernel.org/all/87jzn6lnou.fsf@cloudflare.com/ Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest.h | 7 ++++--- tools/testing/selftests/kselftest_harness.h | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h index 12ad7f8dfe3a..25e29626566e 100644 --- a/tools/testing/selftests/kselftest.h +++ b/tools/testing/selftests/kselftest.h @@ -255,8 +255,9 @@ static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...) va_end(args); }
-static inline __printf(2, 3) -void ksft_test_result_code(int exit_code, const char *msg, ...) +static inline __printf(3, 4) +void ksft_test_result_code(int exit_code, const char *test_name, + const char *msg, ...) { const char *tap_code = "ok"; const char *directive = ""; @@ -287,7 +288,7 @@ void ksft_test_result_code(int exit_code, const char *msg, ...) }
va_start(args, msg); - printf("%s %u%s", tap_code, ksft_test_num(), directive); + printf("%s %u %s%s", tap_code, ksft_test_num(), test_name, directive); 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 79a3fec0cefa..beae50fd5ac3 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -1178,7 +1178,8 @@ void __run_test(struct __fixture_metadata *f, diagnostic = "unknown";
if (t->exit_code == KSFT_SKIP || t->exit_code == KSFT_XFAIL) - ksft_test_result_code(t->exit_code, "%s\n", diagnostic); + ksft_test_result_code(t->exit_code, test_name, + "%s\n", diagnostic); else ksft_test_result(__test_passed(t), "%s\n", test_name); }
According to the spec we should always print a # if we add a diagnostic message. Having the caller pass in the new line as part of diagnostic message makes handling this a bit counter-intuitive.
Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest.h | 5 +++++ tools/testing/selftests/kselftest_harness.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h index 25e29626566e..541bf192e30e 100644 --- a/tools/testing/selftests/kselftest.h +++ b/tools/testing/selftests/kselftest.h @@ -287,10 +287,15 @@ void ksft_test_result_code(int exit_code, const char *test_name, break; }
+ /* Docs seem to call for double space if directive is absent */ + if (!directive[0] && msg[0]) + directive = " # "; + va_start(args, msg); printf("%s %u %s%s", tap_code, ksft_test_num(), test_name, directive); errno = saved_errno; vprintf(msg, args); + printf("\n"); va_end(args); }
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index beae50fd5ac3..5fca75e180b8 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -1179,7 +1179,7 @@ void __run_test(struct __fixture_metadata *f,
if (t->exit_code == KSFT_SKIP || t->exit_code == KSFT_XFAIL) ksft_test_result_code(t->exit_code, test_name, - "%s\n", diagnostic); + "%s", diagnostic); else ksft_test_result(__test_passed(t), "%s\n", test_name); }
Switch to printing KTAP line for PASS / FAIL with ksft_test_result_code(), this gives us the ability to report diagnostic messages for free.
Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest_harness.h | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 5fca75e180b8..202f599c1462 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -1174,14 +1174,12 @@ void __run_test(struct __fixture_metadata *f,
if (t->results->reason[0]) diagnostic = t->results->reason; + else if (t->exit_code == KSFT_PASS || t->exit_code == KSFT_FAIL) + diagnostic = ""; else diagnostic = "unknown";
- if (t->exit_code == KSFT_SKIP || t->exit_code == KSFT_XFAIL) - ksft_test_result_code(t->exit_code, test_name, - "%s", diagnostic); - else - ksft_test_result(__test_passed(t), "%s\n", test_name); + ksft_test_result_code(t->exit_code, test_name, "%s", diagnostic); }
static int test_harness_run(int argc, char **argv)
On Thu, Feb 15, 2024 at 04:41:15PM -0800, Jakub Kicinski wrote:
First 3 patches rearrange kselftest_harness to use exit code as an enum rather than separate passed/skip/xfail members.
One thought I was having here while porting other stuff to use XFAIL was that in the strictest sense, XFAIL isn't like SKIP, which can be used to avoid running a test entirely. XFAIL is about the expected outcome, which means that if we're going to support XFAIL correctly, we need to distinguish when a test was marked XFAIL but it _didn't_ fail.
The implicit expectation is that a test outcome should be "pass". If something is marked "xfail", we're saying a successful test is that it fails. If it _passes_ instead of failing, this is unexpected and should be reported as well. (i.e. an XPASS -- unexpected pass)
I think if we mix intent with result code, we're going to lose the ability to make this distinction in the future. (Right now the harness doesn't do it either -- it treats XFAIL as a special SKIP.)
On Fri, 16 Feb 2024 13:32:12 -0800 Kees Cook wrote:
On Thu, Feb 15, 2024 at 04:41:15PM -0800, Jakub Kicinski wrote:
First 3 patches rearrange kselftest_harness to use exit code as an enum rather than separate passed/skip/xfail members.
One thought I was having here while porting other stuff to use XFAIL was that in the strictest sense, XFAIL isn't like SKIP, which can be used to avoid running a test entirely. XFAIL is about the expected outcome, which means that if we're going to support XFAIL correctly, we need to distinguish when a test was marked XFAIL but it _didn't_ fail.
The implicit expectation is that a test outcome should be "pass". If something is marked "xfail", we're saying a successful test is that it fails. If it _passes_ instead of failing, this is unexpected and should be reported as well. (i.e. an XPASS -- unexpected pass)
I think if we mix intent with result code, we're going to lose the ability to make this distinction in the future. (Right now the harness doesn't do it either -- it treats XFAIL as a special SKIP.)
Hm.
Let's call "case" the combination of fixture + variant + test. Currently nothing identifies a single "case" in the harness. We just recursively walk dimensions.
We can add a new registration list and let user register expected failures. It should work nicely as long as the exceptions are very rare. Which is hopefully the case.
Let's see if I can code this up in 30 min. While I do that can you ELI5 what XPASS is for?! We'll never going to use it, right?
On Fri, 16 Feb 2024 16:31:19 -0800 Jakub Kicinski wrote:
Let's see if I can code this up in 30 min. While I do that can you ELI5 what XPASS is for?! We'll never going to use it, right?
Oh, it's UNexpected pass. Okay. So if we have a case on a list of expected failures and it passes we should throw xpass.
On Fri, 16 Feb 2024 16:33:04 -0800 Jakub Kicinski wrote:
On Fri, 16 Feb 2024 16:31:19 -0800 Jakub Kicinski wrote:
Let's see if I can code this up in 30 min. While I do that can you ELI5 what XPASS is for?! We'll never going to use it, right?
Oh, it's UNexpected pass. Okay. So if we have a case on a list of expected failures and it passes we should throw xpass.
I got distracted from this distraction :S Is this along the lines of what you had in mind? Both my series need to be rejigged to change the paradigm but as a PoC on top of them:
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 202f599c1462..399a200a1160 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -826,6 +826,27 @@ struct __fixture_metadata { .prev = &_fixture_global, };
+struct __test_xfail { + struct __fixture_metadata *fixture; + struct __fixture_variant_metadata *variant; + struct __test_metadata *test; + struct __test_xfail *prev, *next; +}; + +#define XFAIL_ADD(fixture_name, variant_name, test_name) \ + \ + static struct __test_xfail \ + _##fixture_name##_##variant_name##_##test_name##_xfail = \ + { .fixture = &_##fixture_name##_fixture_object, \ + .variant = &_##fixture_name##_##variant_name##_object, \ + .test = &_##fixture_name##_##test_name##_object, \ + };\ + static void __attribute__((constructor)) \ + _register_##fixture_name##_##variant_name##_##test_name##_xfail(void) \ + { \ + __register_xfail(&_##fixture_name##_##variant_name##_##test_name##_xfail); \ + } + static struct __fixture_metadata *__fixture_list = &_fixture_global; static int __constructor_order;
@@ -840,6 +861,7 @@ static inline void __register_fixture(struct __fixture_metadata *f) struct __fixture_variant_metadata { const char *name; const void *data; + struct __test_xfail *xfails; struct __fixture_variant_metadata *prev, *next; };
@@ -890,6 +912,11 @@ static inline void __register_test(struct __test_metadata *t) __LIST_APPEND(t->fixture->tests, t); }
+static inline void __register_xfail(struct __test_xfail *xf) +{ + __LIST_APPEND(xf->variant->xfails, xf); +} + static inline int __bail(int for_realz, struct __test_metadata *t) { /* if this is ASSERT, return immediately. */ @@ -1139,6 +1166,7 @@ void __run_test(struct __fixture_metadata *f, struct __fixture_variant_metadata *variant, struct __test_metadata *t) { + struct __test_xfail *xfail; char test_name[LINE_MAX]; const char *diagnostic;
@@ -1172,6 +1200,14 @@ void __run_test(struct __fixture_metadata *f, ksft_print_msg(" %4s %s\n", __test_passed(t) ? "OK" : "FAIL", test_name);
+ /* Check if we're expecting this test to fail */ + for (xfail = variant->xfails; xfail; xfail = xfail->next) + if (xfail->test == t) + break; + if (xfail) + t->exit_code = __test_passed(t) ? KSFT_XPASS : KSFT_XFAIL; + + if (t->results->reason[0]) diagnostic = t->results->reason; else if (t->exit_code == KSFT_PASS || t->exit_code == KSFT_FAIL) diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c index d4f789f524e5..242ff7de1b12 100644 --- a/tools/testing/selftests/net/ip_local_port_range.c +++ b/tools/testing/selftests/net/ip_local_port_range.c @@ -414,6 +414,9 @@ TEST_F(ip_local_port_range, late_bind) ASSERT_TRUE(!err) TH_LOG("close failed"); }
+XFAIL_ADD(ip_local_port_range, ip4_stcp, late_bind); +XFAIL_ADD(ip_local_port_range, ip6_stcp, late_bind); + TEST_F(ip_local_port_range, get_port_range) { __u16 lo, hi;
On February 16, 2024 5:26:21 PM PST, Jakub Kicinski kuba@kernel.org wrote:
On Fri, 16 Feb 2024 16:33:04 -0800 Jakub Kicinski wrote:
On Fri, 16 Feb 2024 16:31:19 -0800 Jakub Kicinski wrote:
Let's see if I can code this up in 30 min. While I do that can you ELI5 what XPASS is for?! We'll never going to use it, right?
Oh, it's UNexpected pass. Okay. So if we have a case on a list of expected failures and it passes we should throw xpass.
Right: it's still "ok" but it identifies something worth looking at ("why did this start passing?")
I got distracted from this distraction :S Is this along the lines of what you had in mind? Both my series need to be rejigged to change the paradigm but as a PoC on top of them:
Oh yeah! This looks good. I will give it a spin tomorrow.
-Kees
linux-kselftest-mirror@lists.linaro.org
-
Jakub Kicinski -
Kees Cook -
Kees Cook