[PATCH v2] selftests: openat2: don't print total number of tests and then skip

[PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Muhammad Usama Anjum]

Don't print that 88 sub-tests are going to be executed, but then skip.
This is against TAP compliance. Instead check pre-requisites first
before printing total number of tests.

Old non-tap compliant output:
  TAP version 13
  1..88
  ok 2 # SKIP all tests require euid == 0
  # Planned tests != run tests (88 != 1)
  # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0

New and correct output:
  TAP version 13
  1..0 # SKIP all tests require euid == 0

Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
---
Changes since v1:
- Remove simplifying if condition lines
- Update the patch message
---
 tools/testing/selftests/openat2/resolve_test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/openat2/resolve_test.c b/tools/testing/selftests/openat2/resolve_test.c
index bbafad440893c..85a4c64ee950d 100644
--- a/tools/testing/selftests/openat2/resolve_test.c
+++ b/tools/testing/selftests/openat2/resolve_test.c
@@ -508,12 +508,13 @@ void test_openat2_opath_tests(void)
 int main(int argc, char **argv)
 {
 	ksft_print_header();
-	ksft_set_plan(NUM_TESTS);
 
 	/* NOTE: We should be checking for CAP_SYS_ADMIN here... */
 	if (geteuid() != 0)
 		ksft_exit_skip("all tests require euid == 0\n");
 
+	ksft_set_plan(NUM_TESTS);
+
 	test_openat2_opath_tests();
 
 	if (ksft_get_fail_cnt() + ksft_get_error_cnt() > 0)
-- 
2.39.2

Re: [PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

On 2024-07-31, Muhammad Usama Anjum <usama.anjum@collabora.com> wrote:
> Don't print that 88 sub-tests are going to be executed, but then skip.
> This is against TAP compliance. Instead check pre-requisites first
> before printing total number of tests.
> 
> Old non-tap compliant output:
>   TAP version 13
>   1..88
>   ok 2 # SKIP all tests require euid == 0
>   # Planned tests != run tests (88 != 1)
>   # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
> 
> New and correct output:
>   TAP version 13
>   1..0 # SKIP all tests require euid == 0
> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
> Changes since v1:
> - Remove simplifying if condition lines
> - Update the patch message

Feel free to take my

Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>

> ---
>  tools/testing/selftests/openat2/resolve_test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/openat2/resolve_test.c b/tools/testing/selftests/openat2/resolve_test.c
> index bbafad440893c..85a4c64ee950d 100644
> --- a/tools/testing/selftests/openat2/resolve_test.c
> +++ b/tools/testing/selftests/openat2/resolve_test.c
> @@ -508,12 +508,13 @@ void test_openat2_opath_tests(void)
>  int main(int argc, char **argv)
>  {
>  	ksft_print_header();
> -	ksft_set_plan(NUM_TESTS);
>  
>  	/* NOTE: We should be checking for CAP_SYS_ADMIN here... */
>  	if (geteuid() != 0)
>  		ksft_exit_skip("all tests require euid == 0\n");
>  
> +	ksft_set_plan(NUM_TESTS);
> +
>  	test_openat2_opath_tests();
>  
>  	if (ksft_get_fail_cnt() + ksft_get_error_cnt() > 0)
> -- 
> 2.39.2
> 

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Shuah Khan]

On 7/31/24 07:39, Muhammad Usama Anjum wrote:
> Don't print that 88 sub-tests are going to be executed, but then skip.
> This is against TAP compliance. Instead check pre-requisites first
> before printing total number of tests.

Does TAP clearly mention this?

> 
> Old non-tap compliant output:
>    TAP version 13
>    1..88
>    ok 2 # SKIP all tests require euid == 0
>    # Planned tests != run tests (88 != 1)
>    # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
> 
> New and correct output:
>    TAP version 13
>    1..0 # SKIP all tests require euid == 0

The problem is that this new output doesn't show how many tests
are in this test suite that could be run.

I am not use if this is better for communicating coverage information
even if meets the TAP compliance.

> 
> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
> Changes since v1:
> - Remove simplifying if condition lines
> - Update the patch message
> ---
>   tools/testing/selftests/openat2/resolve_test.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/openat2/resolve_test.c b/tools/testing/selftests/openat2/resolve_test.c
> index bbafad440893c..85a4c64ee950d 100644
> --- a/tools/testing/selftests/openat2/resolve_test.c
> +++ b/tools/testing/selftests/openat2/resolve_test.c
> @@ -508,12 +508,13 @@ void test_openat2_opath_tests(void)
>   int main(int argc, char **argv)
>   {
>   	ksft_print_header();
> -	ksft_set_plan(NUM_TESTS);
>   
>   	/* NOTE: We should be checking for CAP_SYS_ADMIN here... */
>   	if (geteuid() != 0)
>   		ksft_exit_skip("all tests require euid == 0\n");
>   
> +	ksft_set_plan(NUM_TESTS);
> +
>   	test_openat2_opath_tests();
>   
>   	if (ksft_get_fail_cnt() + ksft_get_error_cnt() > 0)

thanks,
-- Shuah

Re: [PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Muhammad Usama Anjum]

On 7/31/24 9:57 PM, Shuah Khan wrote:
> On 7/31/24 07:39, Muhammad Usama Anjum wrote:
>> Don't print that 88 sub-tests are going to be executed, but then skip.
>> This is against TAP compliance. Instead check pre-requisites first
>> before printing total number of tests.
> 
> Does TAP clearly mention this?
Yes from https://testanything.org/tap-version-13-specification.html

Skipping everything
This listing shows that the entire listing is a skip. No tests were run.

TAP version 13
1..0 # skip because English-to-French translator isn't installed

We can see above that we need to print 1..0 and skip without printing the
total number of tests to be executed as they are going to be skipped.

> 
>>
>> Old non-tap compliant output:
>>    TAP version 13
>>    1..88
>>    ok 2 # SKIP all tests require euid == 0
>>    # Planned tests != run tests (88 != 1)
>>    # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
>>
>> New and correct output:
>>    TAP version 13
>>    1..0 # SKIP all tests require euid == 0
> 
> The problem is that this new output doesn't show how many tests
> are in this test suite that could be run.
> 
> I am not use if this is better for communicating coverage information
> even if meets the TAP compliance.
I think the number of tests represents the number of planned tests. If we
don't plan to run X number of tests, we shouldn't print it.

> 
>>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>> Changes since v1:
>> - Remove simplifying if condition lines
>> - Update the patch message
>> ---
>>   tools/testing/selftests/openat2/resolve_test.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/openat2/resolve_test.c
>> b/tools/testing/selftests/openat2/resolve_test.c
>> index bbafad440893c..85a4c64ee950d 100644
>> --- a/tools/testing/selftests/openat2/resolve_test.c
>> +++ b/tools/testing/selftests/openat2/resolve_test.c
>> @@ -508,12 +508,13 @@ void test_openat2_opath_tests(void)
>>   int main(int argc, char **argv)
>>   {
>>       ksft_print_header();
>> -    ksft_set_plan(NUM_TESTS);
>>         /* NOTE: We should be checking for CAP_SYS_ADMIN here... */
>>       if (geteuid() != 0)
>>           ksft_exit_skip("all tests require euid == 0\n");
>>   +    ksft_set_plan(NUM_TESTS);
>> +
>>       test_openat2_opath_tests();
>>         if (ksft_get_fail_cnt() + ksft_get_error_cnt() > 0)
> 
> thanks,
> -- Shuah
> 
> 

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Shuah Khan]

On 8/1/24 02:42, Muhammad Usama Anjum wrote:
> On 7/31/24 9:57 PM, Shuah Khan wrote:
>> On 7/31/24 07:39, Muhammad Usama Anjum wrote:
>>> Don't print that 88 sub-tests are going to be executed, but then skip.
>>> This is against TAP compliance. Instead check pre-requisites first
>>> before printing total number of tests.
>>
>> Does TAP clearly mention this?
> Yes from https://testanything.org/tap-version-13-specification.html
> 
> Skipping everything
> This listing shows that the entire listing is a skip. No tests were run.
> 
> TAP version 13
> 1..0 # skip because English-to-French translator isn't installed

I don't see how this is applicable to the current scenario. The user
needs to have root privilege to run the test.

It is important to mention how many tests could have been run.
As mentioned before, this information is important for users and testers.

I would like to see this information in the output.

> 
> We can see above that we need to print 1..0 and skip without printing the
> total number of tests to be executed as they are going to be skipped.
> 
>>
>>>
>>> Old non-tap compliant output:
>>>     TAP version 13
>>>     1..88
>>>     ok 2 # SKIP all tests require euid == 0
>>>     # Planned tests != run tests (88 != 1)>>>     # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
>>>
>>> New and correct output:
>>>     TAP version 13
>>>     1..0 # SKIP all tests require euid == 0
>>
>> The problem is that this new output doesn't show how many tests
>> are in this test suite that could be run.
>>
>> I am not use if this is better for communicating coverage information
>> even if meets the TAP compliance.
> I think the number of tests represents the number of planned tests. If we
> don't plan to run X number of tests, we shouldn't print it.

88 tests are planned to be run except for the fact the first check
failed.

Planned tests could not be run because of user privileges. So these
tests are all skips because of unmet dependencies.

So the a good report would show that 88 tests could have been run. You
can meet the specification and still make it work for us. When we
adapt TAP 13 we didn't require 100% compliance.

There are cases where you can comply and still provide how many test
could be run.

I think you are applying the spec strictly thereby removing useful
information from the report.

Can you tell me what would fail because of this "non-compliance"?

thanks,
-- Shuah

Re: [PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Shuah Khan]

On 8/1/24 10:27, Shuah Khan wrote:
> On 8/1/24 02:42, Muhammad Usama Anjum wrote:
>> On 7/31/24 9:57 PM, Shuah Khan wrote:
>>> On 7/31/24 07:39, Muhammad Usama Anjum wrote:
>>>> Don't print that 88 sub-tests are going to be executed, but then skip.
>>>> This is against TAP compliance. Instead check pre-requisites first
>>>> before printing total number of tests.
>>>
>>> Does TAP clearly mention this?
>> Yes from https://testanything.org/tap-version-13-specification.html
>>
>> Skipping everything
>> This listing shows that the entire listing is a skip. No tests were run.
>>
>> TAP version 13
>> 1..0 # skip because English-to-French translator isn't installed
> 

One more thing on TAP compliance - we don't strive to be TAP compliant
as it doesn't meet our special needs.

It is important to keep the how many tests could be run to improve testing
coverage.

Refer to: https://www.kernel.org/doc/html/latest/dev-tools/ktap.html

"The Linux Kernel largely uses TAP output for test results. However, Kernel
testing frameworks have special needs for test results which don’t align with
the original TAP specification. Thus, a “Kernel TAP” (KTAP) format is specified
to extend and alter TAP to support these use-cases. This specification describes
the generally accepted format of KTAP as it is currently used in the kernel."

I appreciate the effort you are putting into improving the reports. I request
that you refer to the above document and also keep in mind what would help us
improve our testing over focusing just on reports and compliance.

thanks,
-- Shuah

Re: [PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Muhammad Usama Anjum]

On 8/1/24 10:27 PM, Shuah Khan wrote:
> On 8/1/24 10:27, Shuah Khan wrote:
>> On 8/1/24 02:42, Muhammad Usama Anjum wrote:
>>> On 7/31/24 9:57 PM, Shuah Khan wrote:
>>>> On 7/31/24 07:39, Muhammad Usama Anjum wrote:
>>>>> Don't print that 88 sub-tests are going to be executed, but then skip.
>>>>> This is against TAP compliance. Instead check pre-requisites first
>>>>> before printing total number of tests.
>>>>
>>>> Does TAP clearly mention this?
>>> Yes from https://testanything.org/tap-version-13-specification.html
>>>
>>> Skipping everything
>>> This listing shows that the entire listing is a skip. No tests were run.
>>>
>>> TAP version 13
>>> 1..0 # skip because English-to-French translator isn't installed
>>
> 
> One more thing on TAP compliance - we don't strive to be TAP compliant
> as it doesn't meet our special needs.
> 
> It is important to keep the how many tests could be run to improve testing
> coverage.
> 
> Refer to: https://www.kernel.org/doc/html/latest/dev-tools/ktap.html
> 
> "The Linux Kernel largely uses TAP output for test results. However, Kernel
> testing frameworks have special needs for test results which don’t align with
> the original TAP specification. Thus, a “Kernel TAP” (KTAP) format is
> specified
> to extend and alter TAP to support these use-cases. This specification
> describes
> the generally accepted format of KTAP as it is currently used in the kernel."
> 
> I appreciate the effort you are putting into improving the reports. I request
> that you refer to the above document and also keep in mind what would help us
> improve our testing over focusing just on reports and compliance.
That makes sense. I'll work on improving the testing going forward.

> 
> thanks,
> -- Shuah
> 

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Muhammad Usama Anjum]

On 8/1/24 9:27 PM, Shuah Khan wrote:
> On 8/1/24 02:42, Muhammad Usama Anjum wrote:
>> On 7/31/24 9:57 PM, Shuah Khan wrote:
>>> On 7/31/24 07:39, Muhammad Usama Anjum wrote:
>>>> Don't print that 88 sub-tests are going to be executed, but then skip.
>>>> This is against TAP compliance. Instead check pre-requisites first
>>>> before printing total number of tests.
>>>
>>> Does TAP clearly mention this?
>> Yes from https://testanything.org/tap-version-13-specification.html
>>
>> Skipping everything
>> This listing shows that the entire listing is a skip. No tests were run.
>>
>> TAP version 13
>> 1..0 # skip because English-to-French translator isn't installed
> 
> I don't see how this is applicable to the current scenario. The user
> needs to have root privilege to run the test.
> 
> It is important to mention how many tests could have been run.
> As mentioned before, this information is important for users and testers.
> 
> I would like to see this information in the output.
> 
>>
>> We can see above that we need to print 1..0 and skip without printing the
>> total number of tests to be executed as they are going to be skipped.
>>
>>>
>>>>
>>>> Old non-tap compliant output:
>>>>     TAP version 13
>>>>     1..88
>>>>     ok 2 # SKIP all tests require euid == 0
>>>>     # Planned tests != run tests (88 != 1)>>>     # Totals: pass:0
>>>> fail:0 xfail:0 xpass:0 skip:1 error:0
>>>>
>>>> New and correct output:
>>>>     TAP version 13
>>>>     1..0 # SKIP all tests require euid == 0
>>>
>>> The problem is that this new output doesn't show how many tests
>>> are in this test suite that could be run.
>>>
>>> I am not use if this is better for communicating coverage information
>>> even if meets the TAP compliance.
>> I think the number of tests represents the number of planned tests. If we
>> don't plan to run X number of tests, we shouldn't print it.
> 
> 88 tests are planned to be run except for the fact the first check
> failed.
> 
> Planned tests could not be run because of user privileges. So these
> tests are all skips because of unmet dependencies.
Agreed.

> 
> So the a good report would show that 88 tests could have been run. You
> can meet the specification and still make it work for us. When we
> adapt TAP 13 we didn't require 100% compliance.
> 
> There are cases where you can comply and still provide how many test
> could be run.
> 
> I think you are applying the spec strictly thereby removing useful
> information from the report.
> 
> Can you tell me what would fail because of this "non-compliance"?
Some months ago, someone had reported for one of my test that it says it is
going to execute X number of tests. But then it just skips saying it
couldn't run X tests and final footer of tests also didn't had the correct
number of tests in it.

> TAP version 13
> 1..88
This gives information that 88 tests are going to be executed.
> ok 2 # SKIP all tests require euid == 0
Why not ok 1 here?
> # Planned tests != run tests (88 != 1)
This gives a error occured signal instead of telling us that preconditions
failed.
> # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0
The tests exit with KSFT_FAIL instead of KSFT_SKIP. This was the biggest
concern from the report.

> 
> thanks,
> -- Shuah
> 
> 
> 
> 
> 

-- 
BR,
Muhammad Usama Anjum

Re: [PATCH v2] selftests: openat2: don't print total number of tests and then skip

[Shuah Khan]

On 8/1/24 23:38, Muhammad Usama Anjum wrote:
> On 8/1/24 9:27 PM, Shuah Khan wrote:
>> On 8/1/24 02:42, Muhammad Usama Anjum wrote:
>>> On 7/31/24 9:57 PM, Shuah Khan wrote:
>>>> On 7/31/24 07:39, Muhammad Usama Anjum wrote:
>>>>> Don't print that 88 sub-tests are going to be executed, but then skip.
>>>>> This is against TAP compliance. Instead check pre-requisites first
>>>>> before printing total number of tests.
>>>>
>>>> Does TAP clearly mention this?
>>> Yes from https://testanything.org/tap-version-13-specification.html
>>>
>>> Skipping everything
>>> This listing shows that the entire listing is a skip. No tests were run.
>>>
>>> TAP version 13
>>> 1..0 # skip because English-to-French translator isn't installed
>>
>> I don't see how this is applicable to the current scenario. The user
>> needs to have root privilege to run the test.
>>
>> It is important to mention how many tests could have been run.
>> As mentioned before, this information is important for users and testers.
>>
>> I would like to see this information in the output.
>>
>>>
>>> We can see above that we need to print 1..0 and skip without printing the
>>> total number of tests to be executed as they are going to be skipped.
>>>
>>>>
>>>>>
>>>>> Old non-tap compliant output:
>>>>>      TAP version 13
>>>>>      1..88
>>>>>      ok 2 # SKIP all tests require euid == 0
>>>>>      # Planned tests != run tests (88 != 1)>>>     # Totals: pass:0
>>>>> fail:0 xfail:0 xpass:0 skip:1 error:0
>>>>>
>>>>> New and correct output:
>>>>>      TAP version 13
>>>>>      1..0 # SKIP all tests require euid == 0
>>>>
>>>> The problem is that this new output doesn't show how many tests
>>>> are in this test suite that could be run.
>>>>
>>>> I am not use if this is better for communicating coverage information
>>>> even if meets the TAP compliance.
>>> I think the number of tests represents the number of planned tests. If we
>>> don't plan to run X number of tests, we shouldn't print it.
>>
>> 88 tests are planned to be run except for the fact the first check
>> failed.
>>
>> Planned tests could not be run because of user privileges. So these
>> tests are all skips because of unmet dependencies.
> Agreed.
> 
>>
>> So the a good report would show that 88 tests could have been run. You
>> can meet the specification and still make it work for us. When we
>> adapt TAP 13 we didn't require 100% compliance.
>>
>> There are cases where you can comply and still provide how many test
>> could be run.
>>
>> I think you are applying the spec strictly thereby removing useful
>> information from the report.
>>
>> Can you tell me what would fail because of this "non-compliance"?
> Some months ago, someone had reported for one of my test that it says it is
> going to execute X number of tests. But then it just skips saying it
> couldn't run X tests and final footer of tests also didn't had the correct
> number of tests in it.
> 
>> TAP version 13
>> 1..88
> This gives information that 88 tests are going to be executed.
>> ok 2 # SKIP all tests require euid == 0

I agree this should be 1 if we ran just one test.

> Why not ok 1 here?
>> # Planned tests != run tests (88 != 1)
> This gives a error occured signal instead of telling us that preconditions
> failed.

This is correct. We report skip and don't fail when dependencies aren't
met. If we did that it would be reporting false failures since tests
couldn't run tests.

I have asked Laura to see if we can add a message to tell the user
that we couldn't run tests that could have run so they can look at the
config and make changes as needed to increase coverage of testing.

>> # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:1 error:0

This is correct - it shows 1 skip and rest are zero.

> The tests exit with KSFT_FAIL instead of KSFT_SKIP. This was the biggest
> concern from the report.
> 

Exiting with KSFT_FAIL is the real problem that needs to be fixed.
Please take a look to see why this is and send me the fix.

thanks,
-- Shuah