Re: [PATCH 2/2] selftests/powerpc: Skip test instead of failing

[Michael Ellerman <mpe@ellerman.id.au>]

Thiago Jung Bauermann <bauerman@linux.ibm.com> writes:

> Breno Leitao <leitao@debian.org> writes:
>
>> Hi Tyrel,
>>
>> On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:
>>>> --- a/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>>> +++ b/tools/testing/selftests/powerpc/ptrace/core-pkey.c
>>>> @@ -352,17 +352,11 @@ static int write_core_pattern(const char *core_pattern)
>>>>  	FILE *f;
>>>>
>>>>  	f = fopen(core_pattern_file, "w");
>>>> -	if (!f) {
>>>> -		perror("Error writing to core_pattern file");
>>>> -		return TEST_FAIL;
>>>> -	}
>>>> +	SKIP_IF(!f);
>>>>
>>>>  	ret = fwrite(core_pattern, 1, len, f);
>>>>  	fclose(f);
>>>> -	if (ret != len) {
>>>> -		perror("Error writing to core_pattern file");
>>>> -		return TEST_FAIL;
>>>> -	}
>>>> +	SKIP_IF(ret != len);
>>
>>> If we don't have proper privileges we should fail on the open, right?
>>> So wouldn't we still want to fail on the write if something goes
>>> wrong?
>>
>> That is a good point. Should the test fail or skip if it is not possible
>> to create the infrastructure to run the core test?
>>
>> Trying to find the answer in the current test sets, I find tests where
>> the self test skips if the test environment is not able to be set up, as
>> for example, when a memory allocation fails.
>>
>> File: tools/testing/selftests/powerpc/alignment/alignment_handler.c
>>
>>         ci1 = mmap(NULL, bufsize, PROT_WRITE, MAP_SHARED,
>>                    fd, bufsize);
>>         if ((ci0 == MAP_FAILED) || (ci1 == MAP_FAILED)) {
>>                 printf("\n");
>>                 perror("mmap failed");
>>                 SKIP_IF(1);
>>         }
>
> I think TEST_FAIL means the test was able to exercise the feature
> and found a problem with it. In this case, the test wasn't able to
> exercise the feature so it's not appropriate.
>
> Ideally, there should be a TEST_ERROR result for a case like this where
> an unexpected problem prevented the testcase from exercising the
> feature.
>
> If we're to use the an existing result then I vote for SKIP_IF.

Yeah I agree.

See for example some of the TM tests, which skip if TM is not available.
Or the alignment test which skips if it can't open /dev/fb0.

In this case it should print "you need to be root to run this" and then
skip.

cheers