Re: [PATCH v2 6/8] selftests/harness: Refactor XFAIL into SKIP

Ralph Campbell <rcampbell@xxxxxxxxxx> · Mon, 13 Jul 2020 17:50:34 -0700

On 7/13/20 5:13 PM, Kees Cook wrote:
On Mon, Jul 13, 2020 at 12:08:08PM -0700, Ralph Campbell wrote:

On 6/22/20 11:16 AM, Kees Cook wrote:
Plumb the old XFAIL result into a TAP SKIP.

Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
---
   tools/testing/selftests/kselftest_harness.h   | 64 ++++++++++++++-----
   tools/testing/selftests/seccomp/seccomp_bpf.c |  8 +--
   2 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index f8f7e47c739a..b519765904a6 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -112,22 +112,22 @@
   			__FILE__, __LINE__, _metadata->name, ##__VA_ARGS__)
   /**
- * XFAIL(statement, fmt, ...)
+ * SKIP(statement, fmt, ...)
    *
- * @statement: statement to run after reporting XFAIL
+ * @statement: statement to run after reporting SKIP
    * @fmt: format string
    * @...: optional arguments
    *
- * This forces a "pass" after reporting a failure with an XFAIL prefix,
+ * This forces a "pass" after reporting why something is being skipped
    * and runs "statement", which is usually "return" or "goto skip".
    */
-#define XFAIL(statement, fmt, ...) do { \
+#define SKIP(statement, fmt, ...) do { \
   	if (TH_LOG_ENABLED) { \
-		fprintf(TH_LOG_STREAM, "#      XFAIL     " fmt "\n", \
+		fprintf(TH_LOG_STREAM, "#      SKIP     " fmt "\n", \
   			##__VA_ARGS__); \
   	} \
-	/* TODO: find a way to pass xfail to test runner process. */ \
   	_metadata->passed = 1; \
+	_metadata->skip = 1; \
   	_metadata->trigger = 0; \
   	statement; \
   } while (0)
@@ -777,6 +777,7 @@ struct __test_metadata {
   	struct __fixture_metadata *fixture;
   	int termsig;
   	int passed;
+	int skip;	/* did SKIP 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? */
@@ -866,17 +867,31 @@ void __wait_for_test(struct __test_metadata *t)
   		fprintf(TH_LOG_STREAM,
   			"# %s: Test terminated by timeout\n", t->name);
   	} else if (WIFEXITED(status)) {
-		t->passed = t->termsig == -1 ? !WEXITSTATUS(status) : 0;
   		if (t->termsig != -1) {
+			t->passed = 0;
   			fprintf(TH_LOG_STREAM,
   				"# %s: Test exited normally instead of by signal (code: %d)\n",
   				t->name,
   				WEXITSTATUS(status));
-		} else if (!t->passed) {
-			fprintf(TH_LOG_STREAM,
-				"# %s: Test failed at step #%d\n",
-				t->name,
-				WEXITSTATUS(status));
+		} else {
+			switch (WEXITSTATUS(status)) {
+			/* Success */
+			case 0:
+				t->passed = 1;
+				break;
+			/* SKIP */
+			case 255:
+				t->passed = 1;
+				t->skip = 1;
+				break;
+			/* Other failure, assume step report. */
+			default:
+				t->passed = 0;
+				fprintf(TH_LOG_STREAM,
+					"# %s: Test failed at step #%d\n",
+					t->name,
+					WEXITSTATUS(status));
+			}
   		}
   	} else if (WIFSIGNALED(status)) {
   		t->passed = 0;
@@ -906,6 +921,7 @@ void __run_test(struct __fixture_metadata *f,
   {
   	/* reset test struct */
   	t->passed = 1;
+	t->skip = 0;
   	t->trigger = 0;
   	t->step = 0;
   	t->no_print = 0;
@@ -918,15 +934,31 @@ void __run_test(struct __fixture_metadata *f,
   		t->passed = 0;
   	} else if (t->pid == 0) {
   		t->fn(t, variant);
-		/* return the step that failed or 0 */
-		_exit(t->passed ? 0 : t->step);
+		/* Make sure step doesn't get lost in reporting */
+		if (t->step >= 255) {
+			ksft_print_msg("Too many test steps (%u)!?\n", t->step);
+			t->step = 254;
+		}

I noticed that this message is now appearing in the HMM self tests.
I haven't quite tracked down why ->steps should be 255 after running
the first test. I did notice that ASSERT*() calls __INC_STEP() but
that doesn't explain it.
Separately, maybe __INC_STEP() should check for < 254 instead of < 255?

     Set CONFIG_HMM_TESTS=m, build and install kernel and modules.
     cd tools/testing/selftests/vm
     make
     ./test_hmm.sh smoke
     Running smoke test. Note, this test provides basic coverage.
     [  106.803476] memmap_init_zone_device initialised 65536 pages in 7ms
     [  106.810141] added new 256 MB chunk (total 1 chunks, 256 MB) PFNs [0x3ffff0000 0x400000000)
     [  106.823703] memmap_init_zone_device initialised 65536 pages in 4ms
     [  106.829968] added new 256 MB chunk (total 1 chunks, 256 MB) PFNs [0x3fffe0000 0x3ffff0000)
     [  106.838655] HMM test module loaded. This is only for testing HMM.
     TAP version 13
     1..20
     # Starting 20 tests from 3 test cases.
     #  RUN           hmm.open_close ...
     #            OK  hmm.open_close
     ok 1 hmm.open_close
     #  RUN           hmm.anon_read ...
     # Too many test steps (255)!?
     #            OK  hmm.anon_read

Oooh:

#define NTIMES          256

Yes, that's a lot of steps. :)

I agree,__ INC_STEP() needs adjustment, though it should be 253. Does
this work for you?

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 935029d4fb21..4f78e4805633 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -680,7 +680,8 @@
  			__bail(_assert, _metadata->no_print, _metadata->step))
  
  #define __INC_STEP(_metadata) \
-	if (_metadata->passed && _metadata->step < 255) \
+	/* Keep "step" below 255 (which is used for "SKIP" reporting). */	\
+	if (_metadata->passed && _metadata->step < 253) \
  		_metadata->step++;
  
  #define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
@@ -976,12 +977,6 @@ void __run_test(struct __fixture_metadata *f,
  		t->passed = 0;
  	} else if (t->pid == 0) {
  		t->fn(t, variant);
-		/* Make sure step doesn't get lost in reporting */
-		if (t->step >= 255) {
-			ksft_print_msg("Too many test steps (%u)!?\n", t->step);
-			t->step = 254;
-		}
-		/* Use 255 for SKIP */
  		if (t->skip)
  			_exit(255);
  		/* Pass is exit 0 */


Yes, this works for me.
Thanks!






