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

Tyrel Datwyler [off-list ref] writes:

On 10/30/2018 08:16 AM, Michael Ellerman wrote:

quoted

Thiago Jung Bauermann [off-list ref] writes:

quoted

Breno Leitao [off-list ref] writes:

quoted

Hi Tyrel,

On 10/23/2018 05:41 PM, Tyrel Datwyler wrote:

quoted

quoted

--- 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);

quoted

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.

Agreed that there should be some indicator of why we are skipping the
test. My original point I was trying to make was that I thought
skipping a failed open was okay because we are likely not root.
However, I wasn't sure that a failed write was okay to skip as that
could be an indicator that something has actually been broken.

Yes I agree. I didn't read your mail closely enough. Have just replied
to it.

cheers