[PATCH] selftests: kselftest_harness: return Kselftest Skip code for skipped tests

[PATCH] selftests: kselftest_harness: return Kselftest Skip code for skipped tests

[Shuah Khan (Samsung OSG)]

When a test is skipped because of unmet dependencies and/or unsupported
configuration, kselftest_harness exits with error which is treated as a
fail by the Kselftest framework. This leads to false negative result even
when the test could not be run.

Change it to return kselftest skip code when a test gets skipped to
clearly report that the test could not be run. This change add skip
handling to kselftest_harness with minimal changes adding a new skipped
field to struct __test_metadata and using it to recognize KSFT_SKIP exit
from the test function (t->fn) to __run_test() to the test_harness_run()
to return the right skip code to Kselftest framework.

Kselftest framework SKIP code is 4 and the framework prints appropriate
messages to indicate that the test is skipped.

This change is tested on uevent test and the results are as follows:

Before:

TAP version 13
selftests: uevent: uevent_filtering
========================================
[==========] Running 1 tests from 1 test cases.
[ RUN      ] global.uevent_filtering
uevent_filtering.c:355:global.uevent_filtering:Uevent filtering tests require root privileges. Skipping test
global.uevent_filtering: Test skipped at step #4
[     FAIL ] global.uevent_filtering
[==========] 0 / 1 tests passed.
[  FAILED  ]
not ok 1..1 selftests: uevent: uevent_filtering [FAIL]

After:

TAP version 13
selftests: uevent: uevent_filtering
========================================
[==========] Running 1 tests from 1 test cases.
[ RUN      ] global.uevent_filtering
uevent_filtering.c:355:global.uevent_filtering:Uevent filtering tests require root privileges. Skipping test
global.uevent_filtering: Test skipped at step #4
[     SKIP ] global.uevent_filtering
not ok 1..1 selftests: uevent: uevent_filtering [SKIP]

Signed-off-by: Shuah Khan (Samsung OSG) <shuah@kernel.org>
---
 tools/testing/selftests/kselftest_harness.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 6ae3730c4ee3..7af9ab97b367 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -631,6 +631,7 @@ struct __test_metadata {
 	void (*fn)(struct __test_metadata *);
 	int termsig;
 	int passed;
+	int skipped;
 	int trigger; /* extra handler after the evaluation */
 	__u8 step;
 	bool no_print; /* manual trigger when TH_LOG_STREAM is not available */
@@ -694,6 +695,7 @@ void __run_test(struct __test_metadata *t)
 	int status;
 
 	t->passed = 1;
+	t->skipped = 0;
 	t->trigger = 0;
 	printf("[ RUN      ] %s\n", t->name);
 	child_pid = fork();
@@ -716,9 +718,12 @@ void __run_test(struct __test_metadata *t)
 					t->name,
 					WEXITSTATUS(status));
 			} else if (!t->passed) {
+				if (WEXITSTATUS(status) == KSFT_SKIP)
+					t->skipped = 1;
 				fprintf(TH_LOG_STREAM,
-					"%s: Test failed at step #%d\n",
+					"%s: Test %s at step #%d\n",
 					t->name,
+					(t->skipped ? "skipped" : "failed"),
 					WEXITSTATUS(status));
 			}
 		} else if (WIFSIGNALED(status)) {
@@ -743,7 +748,11 @@ void __run_test(struct __test_metadata *t)
 				status);
 		}
 	}
-	printf("[     %4s ] %s\n", (t->passed ? "OK" : "FAIL"), t->name);
+	if (t->skipped)
+		printf("[     %4s ] %s\n", "SKIP", t->name);
+	else
+		printf("[     %4s ] %s\n", (t->passed ? "OK" : "FAIL"),
+			t->name);
 }
 
 static int test_harness_run(int __attribute__((unused)) argc,
@@ -762,6 +771,8 @@ static int test_harness_run(int __attribute__((unused)) argc,
 		__run_test(t);
 		if (t->passed)
 			pass_count++;
+		else if (t->skipped)
+			return KSFT_SKIP;
 		else
 			ret = 1;
 	}
-- 
2.17.0

Re: [PATCH] selftests: kselftest_harness: return Kselftest Skip code for skipped tests

[Kees Cook]

On Wed, Jun 13, 2018 at 6:34 PM, Shuah Khan (Samsung OSG)
<shuah@kernel.org> wrote:
> When a test is skipped because of unmet dependencies and/or unsupported
> configuration, kselftest_harness exits with error which is treated as a
> fail by the Kselftest framework. This leads to false negative result even
> when the test could not be run.
>
> Change it to return kselftest skip code when a test gets skipped to
> clearly report that the test could not be run. This change add skip
> handling to kselftest_harness with minimal changes adding a new skipped
> field to struct __test_metadata and using it to recognize KSFT_SKIP exit
> from the test function (t->fn) to __run_test() to the test_harness_run()
> to return the right skip code to Kselftest framework.
>
> Kselftest framework SKIP code is 4 and the framework prints appropriate
> messages to indicate that the test is skipped.

Unfortunately this will not work: test step # is used as the failure
code to let test runners know where a child failed. KSFT_SKIP is 4, so
every test failing in step 4 would be seen as a skip instead of a
fail.

Tests must not exit on their own with this harness: only the existing
ASSERT/EXPECT macros can be used. uevent test should never be doing
this:

        if (geteuid()) {
                TH_LOG("Uevent filtering tests require root
privileges. Skipping test");
                _exit(KSFT_SKIP);
        }

Nor the _exit(EXIT_FAILURE) calls. Those must all be ASSERT() instead.

Perhaps a new signal could be used, but the return codes are already being used.

-Kees

>
> This change is tested on uevent test and the results are as follows:
>
> Before:
>
> TAP version 13
> selftests: uevent: uevent_filtering
> ========================================
> [==========] Running 1 tests from 1 test cases.
> [ RUN      ] global.uevent_filtering
> uevent_filtering.c:355:global.uevent_filtering:Uevent filtering tests require root privileges. Skipping test
> global.uevent_filtering: Test skipped at step #4
> [     FAIL ] global.uevent_filtering
> [==========] 0 / 1 tests passed.
> [  FAILED  ]
> not ok 1..1 selftests: uevent: uevent_filtering [FAIL]
>
> After:
>
> TAP version 13
> selftests: uevent: uevent_filtering
> ========================================
> [==========] Running 1 tests from 1 test cases.
> [ RUN      ] global.uevent_filtering
> uevent_filtering.c:355:global.uevent_filtering:Uevent filtering tests require root privileges. Skipping test
> global.uevent_filtering: Test skipped at step #4
> [     SKIP ] global.uevent_filtering
> not ok 1..1 selftests: uevent: uevent_filtering [SKIP]
>
> Signed-off-by: Shuah Khan (Samsung OSG) <shuah@kernel.org>
> ---
>  tools/testing/selftests/kselftest_harness.h | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 6ae3730c4ee3..7af9ab97b367 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -631,6 +631,7 @@ struct __test_metadata {
>         void (*fn)(struct __test_metadata *);
>         int termsig;
>         int passed;
> +       int skipped;
>         int trigger; /* extra handler after the evaluation */
>         __u8 step;
>         bool no_print; /* manual trigger when TH_LOG_STREAM is not available */
> @@ -694,6 +695,7 @@ void __run_test(struct __test_metadata *t)
>         int status;
>
>         t->passed = 1;
> +       t->skipped = 0;
>         t->trigger = 0;
>         printf("[ RUN      ] %s\n", t->name);
>         child_pid = fork();
> @@ -716,9 +718,12 @@ void __run_test(struct __test_metadata *t)
>                                         t->name,
>                                         WEXITSTATUS(status));
>                         } else if (!t->passed) {
> +                               if (WEXITSTATUS(status) == KSFT_SKIP)
> +                                       t->skipped = 1;
>                                 fprintf(TH_LOG_STREAM,
> -                                       "%s: Test failed at step #%d\n",
> +                                       "%s: Test %s at step #%d\n",
>                                         t->name,
> +                                       (t->skipped ? "skipped" : "failed"),
>                                         WEXITSTATUS(status));
>                         }
>                 } else if (WIFSIGNALED(status)) {
> @@ -743,7 +748,11 @@ void __run_test(struct __test_metadata *t)
>                                 status);
>                 }
>         }
> -       printf("[     %4s ] %s\n", (t->passed ? "OK" : "FAIL"), t->name);
> +       if (t->skipped)
> +               printf("[     %4s ] %s\n", "SKIP", t->name);
> +       else
> +               printf("[     %4s ] %s\n", (t->passed ? "OK" : "FAIL"),
> +                       t->name);
>  }
>
>  static int test_harness_run(int __attribute__((unused)) argc,
> @@ -762,6 +771,8 @@ static int test_harness_run(int __attribute__((unused)) argc,
>                 __run_test(t);
>                 if (t->passed)
>                         pass_count++;
> +               else if (t->skipped)
> +                       return KSFT_SKIP;
>                 else
>                         ret = 1;
>         }
> --
> 2.17.0
>



-- 
Kees Cook
Pixel Security

Re: [PATCH] selftests: kselftest_harness: return Kselftest Skip code for skipped tests

[Shuah Khan]

On 06/13/2018 10:06 PM, Kees Cook wrote:
> On Wed, Jun 13, 2018 at 6:34 PM, Shuah Khan (Samsung OSG)
> <shuah@kernel.org> wrote:
>> When a test is skipped because of unmet dependencies and/or unsupported
>> configuration, kselftest_harness exits with error which is treated as a
>> fail by the Kselftest framework. This leads to false negative result even
>> when the test could not be run.
>>
>> Change it to return kselftest skip code when a test gets skipped to
>> clearly report that the test could not be run. This change add skip
>> handling to kselftest_harness with minimal changes adding a new skipped
>> field to struct __test_metadata and using it to recognize KSFT_SKIP exit
>> from the test function (t->fn) to __run_test() to the test_harness_run()
>> to return the right skip code to Kselftest framework.
>>
>> Kselftest framework SKIP code is 4 and the framework prints appropriate
>> messages to indicate that the test is skipped.
> 
> Unfortunately this will not work: test step # is used as the failure
> code to let test runners know where a child failed. KSFT_SKIP is 4, so
> every test failing in step 4 would be seen as a skip instead of a
> fail.
> 

Yeah. That is correct. __bail() does exit with step which could be step #4

> Tests must not exit on their own with this harness: only the existing
> ASSERT/EXPECT macros can be used. uevent test should never be doing
> this:
> 
>         if (geteuid()) {
>                 TH_LOG("Uevent filtering tests require root
> privileges. Skipping test");
>                 _exit(KSFT_SKIP);
>         }
> 
> Nor the _exit(EXIT_FAILURE) calls. Those must all be ASSERT() instead.

It did look like an improper use of the harness by this test. Okay that
makes sense.

> 
> Perhaps a new signal could be used, but the return codes are already being used.
> 

A a new harness hook for KSFT_SKIP case so tests can call that explicitly would
solve the problem once the problem of conflict with step #4.

Since he harness step metadata doesn't have special meaning than keeping track of
how far test ran, harness could be changed to treat step #4 as a special value and
not use it for step to solve the conflict between ksft and kselftest_harness.

thanks,
-- Shuah