Re: [PATCH net-next 3/4] selftests: kselftest_harness: support using xfail

[Jakub Sitnicki <jakub@cloudflare.com>]

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 {									\