Hi!
When running selftests for our subsystem in our CI we'd like all tests to pass. Currently some tests use SKIP for cases they expect to fail, because the kselftest_harness limits the return codes to pass/fail/skip.
Clean up and support the use of the full range of ksft exit codes under kselftest_harness.
To avoid conflicts and get the functionality into the networking tree ASAP I'd like to put the patches on shared branch so that both linux-kselftest and net-next can pull it in. Shuah, please LMK if that'd work for you, and if so which -rc should I base the branch on. Or is merging directly into net-next okay?
Jakub Kicinski (4): selftests: kselftest_harness: pass step via shared memory selftests: kselftest_harness: use KSFT_* exit codes selftests: kselftest_harness: support using xfail selftests: ip_local_port_range: use XFAIL instead of SKIP
tools/testing/selftests/kselftest_harness.h | 67 ++++++++++++++----- .../selftests/net/ip_local_port_range.c | 2 +- 2 files changed, 52 insertions(+), 17 deletions(-)
Commit 0ef67a888375 ("selftests/harness: Report skip reason") added shared memory to communicate between harness and test. Use that instead of exit codes to send the failing step back to the harness. The exit codes are limited and because of the step passing we can't use the full range of KSFT_* exit codes.
Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest_harness.h | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index e05ac8261046..98bdedf9a53a 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -101,8 +101,8 @@ * ASSERT_* number for which the test failed. This behavior can be enabled by * writing `_metadata->no_print = true;` before the check sequence that is * unable to print. When an error occur, instead of printing an error message - * and calling `abort(3)`, the test process call `_exit(2)` with the assert - * number as argument, which is then printed by the parent process. + * and calling `abort(3)`, the test process call `_exit(2)` and pass the error + * to be printed to the parent process via shared memory. */ #define TH_LOG(fmt, ...) do { \ if (TH_LOG_ENABLED) \ @@ -695,9 +695,8 @@ __bail(_assert, _metadata))
#define __INC_STEP(_metadata) \ - /* Keep "step" below 255 (which is used for "SKIP" reporting). */ \ - if (_metadata->passed && _metadata->step < 253) \ - _metadata->step++; + if (_metadata->passed) \ + _metadata->results->step++;
#define is_signed_type(var) (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
@@ -784,6 +783,7 @@
struct __test_results { char reason[1024]; /* Reason for test result */ + unsigned int step; /* Test step reached without failure */ };
struct __test_metadata; @@ -837,7 +837,6 @@ struct __test_metadata { 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? */ - __u8 step; bool no_print; /* manual trigger when TH_LOG_STREAM is not available */ bool aborted; /* stopped test due to failed ASSERT */ bool setup_completed; /* did setup finish? */ @@ -875,7 +874,7 @@ static inline void __test_check_assert(struct __test_metadata *t) { if (t->aborted) { if (t->no_print) - _exit(t->step); + _exit(1); abort(); } } @@ -960,7 +959,7 @@ void __wait_for_test(struct __test_metadata *t) fprintf(TH_LOG_STREAM, "# %s: Test failed at step #%d\n", t->name, - WEXITSTATUS(status)); + t->results->step); } } } else if (WIFSIGNALED(status)) { @@ -1114,9 +1113,9 @@ void __run_test(struct __fixture_metadata *f, t->passed = 1; t->skip = 0; t->trigger = 0; - t->step = 1; t->no_print = 0; 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); @@ -1137,8 +1136,8 @@ void __run_test(struct __fixture_metadata *f, /* Pass is exit 0 */ if (t->passed) _exit(0); - /* Something else happened, report the step. */ - _exit(t->step); + /* Something else happened. */ + _exit(1); } else { __wait_for_test(t); }
On Tue, Feb 13, 2024 at 07:44:13AM -0800, Jakub Kicinski wrote:
Commit 0ef67a888375 ("selftests/harness: Report skip reason") added shared memory to communicate between harness and test. Use that instead of exit codes to send the failing step back to the harness. The exit codes are limited and because of the step passing we can't use the full range of KSFT_* exit codes.
Signed-off-by: Jakub Kicinski kuba@kernel.org
Ah, very nice! Good idea.
Acked-by: Kees Cook keescook@chromium.org
Hi Jakub,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Jakub-Kicinski/selftests-ksel... base: net-next/main patch link: https://lore.kernel.org/r/20240213154416.422739-2-kuba%40kernel.org patch subject: [PATCH net-next 1/4] selftests: kselftest_harness: pass step via shared memory compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240219/202402191034.76fuePpP-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202402191034.76fuePpP-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from fs_test.c:26: fs_test.c: In function 'layout1_no_restriction':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:403:1: note: in expansion of macro 'TEST_F_FORK' 403 | TEST_F_FORK(layout1, no_restriction) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:403:1: note: in expansion of macro 'TEST_F_FORK' 403 | TEST_F_FORK(layout1, no_restriction) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:403:1: note: in expansion of macro 'TEST_F_FORK' 403 | TEST_F_FORK(layout1, no_restriction) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_inval':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:426:1: note: in expansion of macro 'TEST_F_FORK' 426 | TEST_F_FORK(layout1, inval) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:426:1: note: in expansion of macro 'TEST_F_FORK' 426 | TEST_F_FORK(layout1, inval) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:426:1: note: in expansion of macro 'TEST_F_FORK' 426 | TEST_F_FORK(layout1, inval) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_file_and_dir_access_rights':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:548:1: note: in expansion of macro 'TEST_F_FORK' 548 | TEST_F_FORK(layout1, file_and_dir_access_rights) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:548:1: note: in expansion of macro 'TEST_F_FORK' 548 | TEST_F_FORK(layout1, file_and_dir_access_rights) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:548:1: note: in expansion of macro 'TEST_F_FORK' 548 | TEST_F_FORK(layout1, file_and_dir_access_rights) | ^~~~~~~~~~~ fs_test.c: In function 'layout0_ruleset_with_unknown_access':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:592:1: note: in expansion of macro 'TEST_F_FORK' 592 | TEST_F_FORK(layout0, ruleset_with_unknown_access) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:592:1: note: in expansion of macro 'TEST_F_FORK' 592 | TEST_F_FORK(layout0, ruleset_with_unknown_access) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:592:1: note: in expansion of macro 'TEST_F_FORK' 592 | TEST_F_FORK(layout0, ruleset_with_unknown_access) | ^~~~~~~~~~~ fs_test.c: In function 'layout0_rule_with_unknown_access':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:608:1: note: in expansion of macro 'TEST_F_FORK' 608 | TEST_F_FORK(layout0, rule_with_unknown_access) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:608:1: note: in expansion of macro 'TEST_F_FORK' 608 | TEST_F_FORK(layout0, rule_with_unknown_access) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:608:1: note: in expansion of macro 'TEST_F_FORK' 608 | TEST_F_FORK(layout0, rule_with_unknown_access) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_rule_with_unhandled_access':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:635:1: note: in expansion of macro 'TEST_F_FORK' 635 | TEST_F_FORK(layout1, rule_with_unhandled_access) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:635:1: note: in expansion of macro 'TEST_F_FORK' 635 | TEST_F_FORK(layout1, rule_with_unhandled_access) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:635:1: note: in expansion of macro 'TEST_F_FORK' 635 | TEST_F_FORK(layout1, rule_with_unhandled_access) | ^~~~~~~~~~~ fs_test.c: In function 'layout0_proc_nsfs':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:741:1: note: in expansion of macro 'TEST_F_FORK' 741 | TEST_F_FORK(layout0, proc_nsfs) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:741:1: note: in expansion of macro 'TEST_F_FORK' 741 | TEST_F_FORK(layout0, proc_nsfs) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:741:1: note: in expansion of macro 'TEST_F_FORK' 741 | TEST_F_FORK(layout0, proc_nsfs) | ^~~~~~~~~~~ fs_test.c: In function 'layout0_unpriv':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:790:1: note: in expansion of macro 'TEST_F_FORK' 790 | TEST_F_FORK(layout0, unpriv) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:790:1: note: in expansion of macro 'TEST_F_FORK' 790 | TEST_F_FORK(layout0, unpriv) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:790:1: note: in expansion of macro 'TEST_F_FORK' 790 | TEST_F_FORK(layout0, unpriv) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_effective_access':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:813:1: note: in expansion of macro 'TEST_F_FORK' 813 | TEST_F_FORK(layout1, effective_access) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:813:1: note: in expansion of macro 'TEST_F_FORK' 813 | TEST_F_FORK(layout1, effective_access) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:813:1: note: in expansion of macro 'TEST_F_FORK' 813 | TEST_F_FORK(layout1, effective_access) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_unhandled_access':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:871:1: note: in expansion of macro 'TEST_F_FORK' 871 | TEST_F_FORK(layout1, unhandled_access) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:871:1: note: in expansion of macro 'TEST_F_FORK' 871 | TEST_F_FORK(layout1, unhandled_access) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:871:1: note: in expansion of macro 'TEST_F_FORK' 871 | TEST_F_FORK(layout1, unhandled_access) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_ruleset_overlap':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:898:1: note: in expansion of macro 'TEST_F_FORK' 898 | TEST_F_FORK(layout1, ruleset_overlap) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:898:1: note: in expansion of macro 'TEST_F_FORK' 898 | TEST_F_FORK(layout1, ruleset_overlap) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:898:1: note: in expansion of macro 'TEST_F_FORK' 898 | TEST_F_FORK(layout1, ruleset_overlap) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_layer_rule_unions':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:939:1: note: in expansion of macro 'TEST_F_FORK' 939 | TEST_F_FORK(layout1, layer_rule_unions) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:939:1: note: in expansion of macro 'TEST_F_FORK' 939 | TEST_F_FORK(layout1, layer_rule_unions) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:939:1: note: in expansion of macro 'TEST_F_FORK' 939 | TEST_F_FORK(layout1, layer_rule_unions) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_non_overlapping_accesses':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:1046:1: note: in expansion of macro 'TEST_F_FORK' 1046 | TEST_F_FORK(layout1, non_overlapping_accesses) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1046:1: note: in expansion of macro 'TEST_F_FORK' 1046 | TEST_F_FORK(layout1, non_overlapping_accesses) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1046:1: note: in expansion of macro 'TEST_F_FORK' 1046 | TEST_F_FORK(layout1, non_overlapping_accesses) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_interleaved_masked_accesses':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:1095:1: note: in expansion of macro 'TEST_F_FORK' 1095 | TEST_F_FORK(layout1, interleaved_masked_accesses) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1095:1: note: in expansion of macro 'TEST_F_FORK' 1095 | TEST_F_FORK(layout1, interleaved_masked_accesses) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1095:1: note: in expansion of macro 'TEST_F_FORK' 1095 | TEST_F_FORK(layout1, interleaved_masked_accesses) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_inherit_subset':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:1280:1: note: in expansion of macro 'TEST_F_FORK' 1280 | TEST_F_FORK(layout1, inherit_subset) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1280:1: note: in expansion of macro 'TEST_F_FORK' 1280 | TEST_F_FORK(layout1, inherit_subset) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1280:1: note: in expansion of macro 'TEST_F_FORK' 1280 | TEST_F_FORK(layout1, inherit_subset) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_inherit_superset':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:1397:1: note: in expansion of macro 'TEST_F_FORK' 1397 | TEST_F_FORK(layout1, inherit_superset) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1397:1: note: in expansion of macro 'TEST_F_FORK' 1397 | TEST_F_FORK(layout1, inherit_superset) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1397:1: note: in expansion of macro 'TEST_F_FORK' 1397 | TEST_F_FORK(layout1, inherit_superset) | ^~~~~~~~~~~ fs_test.c: In function 'layout0_max_layers':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:1434:1: note: in expansion of macro 'TEST_F_FORK' 1434 | TEST_F_FORK(layout0, max_layers) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1434:1: note: in expansion of macro 'TEST_F_FORK' 1434 | TEST_F_FORK(layout0, max_layers) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1434:1: note: in expansion of macro 'TEST_F_FORK' 1434 | TEST_F_FORK(layout0, max_layers) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_empty_or_same_ruleset':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:1458:1: note: in expansion of macro 'TEST_F_FORK' 1458 | TEST_F_FORK(layout1, empty_or_same_ruleset) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1458:1: note: in expansion of macro 'TEST_F_FORK' 1458 | TEST_F_FORK(layout1, empty_or_same_ruleset) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1458:1: note: in expansion of macro 'TEST_F_FORK' 1458 | TEST_F_FORK(layout1, empty_or_same_ruleset) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_rule_on_mountpoint':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:1492:1: note: in expansion of macro 'TEST_F_FORK' 1492 | TEST_F_FORK(layout1, rule_on_mountpoint) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1492:1: note: in expansion of macro 'TEST_F_FORK' 1492 | TEST_F_FORK(layout1, rule_on_mountpoint) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1492:1: note: in expansion of macro 'TEST_F_FORK' 1492 | TEST_F_FORK(layout1, rule_on_mountpoint) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_rule_over_mountpoint':
common.h:52:40: error: 'struct __test_metadata' has no member named 'step'
52 | _exit(_metadata->step); \ | ^~ fs_test.c:1521:1: note: in expansion of macro 'TEST_F_FORK' 1521 | TEST_F_FORK(layout1, rule_over_mountpoint) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1521:1: note: in expansion of macro 'TEST_F_FORK' 1521 | TEST_F_FORK(layout1, rule_over_mountpoint) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1521:1: note: in expansion of macro 'TEST_F_FORK' 1521 | TEST_F_FORK(layout1, rule_over_mountpoint) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_rule_over_root_allow_then_deny': common.h:52:40: error: 'struct __test_metadata' has no member named 'step' 52 | _exit(_metadata->step); \ | ^~ fs_test.c:1554:1: note: in expansion of macro 'TEST_F_FORK' 1554 | TEST_F_FORK(layout1, rule_over_root_allow_then_deny) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1554:1: note: in expansion of macro 'TEST_F_FORK' 1554 | TEST_F_FORK(layout1, rule_over_root_allow_then_deny) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1554:1: note: in expansion of macro 'TEST_F_FORK' 1554 | TEST_F_FORK(layout1, rule_over_root_allow_then_deny) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_rule_over_root_deny': common.h:52:40: error: 'struct __test_metadata' has no member named 'step' 52 | _exit(_metadata->step); \ | ^~ fs_test.c:1584:1: note: in expansion of macro 'TEST_F_FORK' 1584 | TEST_F_FORK(layout1, rule_over_root_deny) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1584:1: note: in expansion of macro 'TEST_F_FORK' 1584 | TEST_F_FORK(layout1, rule_over_root_deny) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1584:1: note: in expansion of macro 'TEST_F_FORK' 1584 | TEST_F_FORK(layout1, rule_over_root_deny) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_rule_inside_mount_ns': common.h:52:40: error: 'struct __test_metadata' has no member named 'step' 52 | _exit(_metadata->step); \ | ^~ fs_test.c:1604:1: note: in expansion of macro 'TEST_F_FORK' 1604 | TEST_F_FORK(layout1, rule_inside_mount_ns) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1604:1: note: in expansion of macro 'TEST_F_FORK' 1604 | TEST_F_FORK(layout1, rule_inside_mount_ns) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1604:1: note: in expansion of macro 'TEST_F_FORK' 1604 | TEST_F_FORK(layout1, rule_inside_mount_ns) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_mount_and_pivot': common.h:52:40: error: 'struct __test_metadata' has no member named 'step' 52 | _exit(_metadata->step); \ | ^~ fs_test.c:1632:1: note: in expansion of macro 'TEST_F_FORK' 1632 | TEST_F_FORK(layout1, mount_and_pivot) | ^~~~~~~~~~~ common.h:58:34: error: 'struct __test_metadata' has no member named 'step' 58 | _metadata->step = 1; \ | ^~ fs_test.c:1632:1: note: in expansion of macro 'TEST_F_FORK' 1632 | TEST_F_FORK(layout1, mount_and_pivot) | ^~~~~~~~~~~ common.h:71:34: error: 'struct __test_metadata' has no member named 'step' 71 | _metadata->step = WEXITSTATUS(status); \ | ^~ fs_test.c:1632:1: note: in expansion of macro 'TEST_F_FORK' 1632 | TEST_F_FORK(layout1, mount_and_pivot) | ^~~~~~~~~~~ fs_test.c: In function 'layout1_move_mount': common.h:52:40: error: 'struct __test_metadata' has no member named 'step' 52 | _exit(_metadata->step); \ | ^~ fs_test.c:1655:1: note: in expansion of macro 'TEST_F_FORK' 1655 | TEST_F_FORK(layout1, move_mount) | ^~~~~~~~~~~
Now that we no longer need low exit codes to communicate assertion steps - use normal KSFT exit codes.
Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/kselftest_harness.h | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h index 98bdedf9a53a..618b41eac749 100644 --- a/tools/testing/selftests/kselftest_harness.h +++ b/tools/testing/selftests/kselftest_harness.h @@ -874,7 +874,7 @@ static inline void __test_check_assert(struct __test_metadata *t) { if (t->aborted) { if (t->no_print) - _exit(1); + _exit(KSFT_FAIL); abort(); } } @@ -937,7 +937,7 @@ 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) == 255) { + if (WEXITSTATUS(status) == KSFT_SKIP) { /* SKIP */ t->passed = 1; t->skip = 1; @@ -950,7 +950,7 @@ void __wait_for_test(struct __test_metadata *t) } else { switch (WEXITSTATUS(status)) { /* Success */ - case 0: + case KSFT_PASS: t->passed = 1; break; /* Other failure, assume step report. */ @@ -1132,12 +1132,11 @@ void __run_test(struct __fixture_metadata *f, setpgrp(); t->fn(t, variant); if (t->skip) - _exit(255); - /* Pass is exit 0 */ + _exit(KSFT_SKIP); if (t->passed) - _exit(0); + _exit(KSFT_PASS); /* Something else happened. */ - _exit(1); + _exit(KSFT_FAIL); } else { __wait_for_test(t); }
On Tue, Feb 13, 2024 at 07:44:14AM -0800, Jakub Kicinski wrote:
Now that we no longer need low exit codes to communicate assertion steps - use normal KSFT exit codes.
Signed-off-by: Jakub Kicinski kuba@kernel.org
Yeah, good cleanup.
Acked-by: Kees Cook keescook@chromium.org
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 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;@@ -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);@@ -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");
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.
SCTP does not support IP_LOCAL_PORT_RANGE and we know it, so use XFAIL instead of SKIP.
Signed-off-by: Jakub Kicinski kuba@kernel.org --- tools/testing/selftests/net/ip_local_port_range.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/ip_local_port_range.c b/tools/testing/selftests/net/ip_local_port_range.c index 0f217a1cc837..c1d3f001c240 100644 --- a/tools/testing/selftests/net/ip_local_port_range.c +++ b/tools/testing/selftests/net/ip_local_port_range.c @@ -362,7 +362,7 @@ TEST_F(ip_local_port_range, late_bind) __u16 port;
if (variant->so_protocol == IPPROTO_SCTP) - SKIP(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT"); + XFAIL(return, "SCTP doesn't support IP_BIND_ADDRESS_NO_PORT");
fd = socket(variant->so_domain, variant->so_type, 0); ASSERT_GE(fd, 0) TH_LOG("socket failed");
On Tue, Feb 13, 2024 at 07:44:16AM -0800, Jakub Kicinski wrote:
SCTP does not support IP_LOCAL_PORT_RANGE and we know it, so use XFAIL instead of SKIP.
Signed-off-by: Jakub Kicinski kuba@kernel.org
Reviewed-by: Kees Cook keescook@chromium.org
On Tue, Feb 13, 2024 at 07:44:12AM -0800, Jakub Kicinski wrote:
Hi!
When running selftests for our subsystem in our CI we'd like all tests to pass. Currently some tests use SKIP for cases they expect to fail, because the kselftest_harness limits the return codes to pass/fail/skip.
Clean up and support the use of the full range of ksft exit codes under kselftest_harness.
To avoid conflicts and get the functionality into the networking tree ASAP I'd like to put the patches on shared branch so that both linux-kselftest and net-next can pull it in. Shuah, please LMK if that'd work for you, and if so which -rc should I base the branch on. Or is merging directly into net-next okay?
I would use XFAIL for seccomp selftests too, but I can wait for the next release. i.e. I don't need a shared branch -- it'd be fine in net-next. But I defer to Shuah as far as the selftest tree is concerned. (FWIW, I don't see any current conflicts.)
On Tue, Feb 13, 2024 at 07:44 AM -08, Jakub Kicinski wrote:
Hi!
When running selftests for our subsystem in our CI we'd like all tests to pass. Currently some tests use SKIP for cases they expect to fail, because the kselftest_harness limits the return codes to pass/fail/skip.
Clean up and support the use of the full range of ksft exit codes under kselftest_harness.
To avoid conflicts and get the functionality into the networking tree ASAP I'd like to put the patches on shared branch so that both linux-kselftest and net-next can pull it in. Shuah, please LMK if that'd work for you, and if so which -rc should I base the branch on. Or is merging directly into net-next okay?
Jakub Kicinski (4): selftests: kselftest_harness: pass step via shared memory selftests: kselftest_harness: use KSFT_* exit codes selftests: kselftest_harness: support using xfail selftests: ip_local_port_range: use XFAIL instead of SKIP
tools/testing/selftests/kselftest_harness.h | 67 ++++++++++++++----- .../selftests/net/ip_local_port_range.c | 2 +- 2 files changed, 52 insertions(+), 17 deletions(-)
Nice! We've been ignoring skipped tests in our internal CI. But this is the wrong approach, as you point out.
For the series:
Tested-by: Jakub Sitnicki jakub@cloudflare.com
linux-kselftest-mirror@lists.linaro.org
-
Jakub Kicinski -
Jakub Sitnicki -
Kees Cook -
kernel test robot