| Message ID | 20240216004122.2004689-1-kuba@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | selftests: kselftest_harness: use common result printing helper | expand |
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
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(-)