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

[PATCH] 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.
The error is printed that executed test was only 1 while 88 should have
run:

Old 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>
---
 tools/testing/selftests/openat2/resolve_test.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/openat2/resolve_test.c b/tools/testing/selftests/openat2/resolve_test.c
index bbafad440893c..5472ec478d227 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)
+	if (geteuid())
 		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] selftests: openat2: don't print total number of tests and then skip

[Muhammad Usama Anjum]

Adding more people for review

On 5/23/24 2:46 AM, Muhammad Usama Anjum wrote:
> Don't print that 88 sub-tests are going to be executed. But then skip.
> The error is printed that executed test was only 1 while 88 should have
> run:
> 
> Old 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>
> ---
>  tools/testing/selftests/openat2/resolve_test.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/openat2/resolve_test.c b/tools/testing/selftests/openat2/resolve_test.c
> index bbafad440893c..5472ec478d227 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)
> +	if (geteuid())
>  		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)

-- 
BR,
Muhammad Usama Anjum

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

[Aleksa Sarai]

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

On 2024-07-01, Muhammad Usama Anjum <usama.anjum@collabora.com> wrote:
> Adding more people for review
> 
> On 5/23/24 2:46 AM, Muhammad Usama Anjum wrote:
> > Don't print that 88 sub-tests are going to be executed. But then skip.
> > The error is printed that executed test was only 1 while 88 should have
> > run:
> > 
> > Old 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>
> > ---
> >  tools/testing/selftests/openat2/resolve_test.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/openat2/resolve_test.c b/tools/testing/selftests/openat2/resolve_test.c
> > index bbafad440893c..5472ec478d227 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)
> > +	if (geteuid())

This change isn't necessary, != 0 makes what we're checking clearer.

> >  		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)
> 
> -- 
> BR,
> Muhammad Usama Anjum

-- 
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] selftests: openat2: don't print total number of tests and then skip

[Muhammad Usama Anjum]

On 7/1/24 2:14 PM, Aleksa Sarai wrote:
> On 2024-07-01, Muhammad Usama Anjum <usama.anjum@collabora.com> wrote:
>> Adding more people for review
>>
>> On 5/23/24 2:46 AM, Muhammad Usama Anjum wrote:
>>> Don't print that 88 sub-tests are going to be executed. But then skip.
>>> The error is printed that executed test was only 1 while 88 should have
>>> run:
>>>
>>> Old 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>
>>> ---
>>>  tools/testing/selftests/openat2/resolve_test.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/openat2/resolve_test.c b/tools/testing/selftests/openat2/resolve_test.c
>>> index bbafad440893c..5472ec478d227 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)
>>> +	if (geteuid())
> 
> This change isn't necessary, != 0 makes what we're checking clearer.
It is wasteful to write != 0 when you can achieve the same without it.

I can update the patch by removing it. Please can you provide a reviewed-by
tag for remaining patch?

> 
>>>  		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)
>>
>> -- 
>> BR,
>> Muhammad Usama Anjum
> 

-- 
BR,
Muhammad Usama Anjum

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

[Muhammad Usama Anjum]

Hi Shuah,

Can you take the patch as is or by removing following from this patch:

-	if (geteuid() != 0)
+	if (geteuid())

Thanks,
Usama

On 7/2/24 12:02 PM, Muhammad Usama Anjum wrote:
> On 7/1/24 2:14 PM, Aleksa Sarai wrote:
>> On 2024-07-01, Muhammad Usama Anjum <usama.anjum@collabora.com> wrote:
>>> Adding more people for review
>>>
>>> On 5/23/24 2:46 AM, Muhammad Usama Anjum wrote:
>>>> Don't print that 88 sub-tests are going to be executed. But then skip.
>>>> The error is printed that executed test was only 1 while 88 should have
>>>> run:
>>>>
>>>> Old 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>
>>>> ---
>>>>  tools/testing/selftests/openat2/resolve_test.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/openat2/resolve_test.c b/tools/testing/selftests/openat2/resolve_test.c
>>>> index bbafad440893c..5472ec478d227 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)
>>>> +	if (geteuid())
>>
>> This change isn't necessary, != 0 makes what we're checking clearer.
> It is wasteful to write != 0 when you can achieve the same without it.
> 
> I can update the patch by removing it. Please can you provide a reviewed-by
> tag for remaining patch?
> 
>>
>>>>  		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)
>>>
>>> -- 
>>> BR,
>>> Muhammad Usama Anjum
>>
> 

-- 
BR,
Muhammad Usama Anjum

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

[Shuah Khan]

On 7/10/24 03:33, Muhammad Usama Anjum wrote:
> Hi Shuah,
> 
> Can you take the patch as is or by removing following from this patch:
> 
> -	if (geteuid() != 0)
> +	if (geteuid())

As Aleksa mentioned, geteuid() != 0 is preferred.

> 
> On 7/2/24 12:02 PM, Muhammad Usama Anjum wrote:
>> On 7/1/24 2:14 PM, Aleksa Sarai wrote:
>>> On 2024-07-01, Muhammad Usama Anjum <usama.anjum@collabora.com> wrote:
>>>> Adding more people for review
>>>>
>>>> On 5/23/24 2:46 AM, Muhammad Usama Anjum wrote:
>>>>> Don't print that 88 sub-tests are going to be executed. But then skip.
>>>>> The error is printed that executed test was only 1 while 88 should have
>>>>> run:
>>>>>
>>>>> Old 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

I think having total number tell you how many tests are there.
I don't this this is good change.

thanks,
-- Shuah

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

[Muhammad Usama Anjum]

On 7/10/24 9:18 PM, Shuah Khan wrote:
> On 7/10/24 03:33, Muhammad Usama Anjum wrote:
>> Hi Shuah,
>>
>> Can you take the patch as is or by removing following from this patch:
>>
>> -    if (geteuid() != 0)
>> +    if (geteuid())
> 
> As Aleksa mentioned, geteuid() != 0 is preferred.
I can make the change after concluding the following discussion.

> 
>>
>> On 7/2/24 12:02 PM, Muhammad Usama Anjum wrote:
>>> On 7/1/24 2:14 PM, Aleksa Sarai wrote:
>>>> On 2024-07-01, Muhammad Usama Anjum <usama.anjum@collabora.com> wrote:
>>>>> Adding more people for review
>>>>>
>>>>> On 5/23/24 2:46 AM, Muhammad Usama Anjum wrote:
>>>>>> Don't print that 88 sub-tests are going to be executed. But then skip.
>>>>>> The error is printed that executed test was only 1 while 88 should have
>>>>>> run:
>>>>>>
>>>>>> Old 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
This would return FAIL exit code to kselftest executor as the planned tests
were 88 while executed tests are only 1. Hence FAIL error code would be
returned. This is completely wrong.

>>>>>>
>>>>>> New and correct output:
>>>>>>    TAP version 13
>>>>>>    1..0 # SKIP all tests require euid == 0
This would return SKIP exit code which is correct.


> 
> I think having total number tell you how many tests are there.
> I don't this this is good change.
Having "1..88" misrepresents the number of executed tests. This is against
the TAP specs. The total number of tests must be printed after finding out
that initial conditions are fulfilled. From specs: [1]

> A plan line of 1..0 indicates that the test set was completely skipped;
> no tests are expected to follow, and none should have come before.
> Harnesses should report on 1..0 test runs similarly to their handling of
> SKIP Test Points, treating any comment in the Plan as the reason for
> skipping.
>
> 1..0 # WWW::Mechanize not installed

[1] https://testanything.org/tap-version-14-specification.html

> 
> thanks,
> -- Shuah
> 

-- 
BR,
Muhammad Usama Anjum