Re: [PATCH] selftests: ksft: Track skipped tests when finishing the test suite

[Laura Nao]

Hi Shuah,

On 7/23/24 18:17, Shuah Khan wrote:
> On 7/22/24 09:43, Laura Nao wrote:
>> Consider skipped tests in addition to passed tests when evaluating the
>> overall result of the test suite in the finished() helper.
>>
>> Signed-off-by: Laura Nao <laura.nao@xxxxxxxxxxxxx>
>> ---
>>   tools/testing/selftests/kselftest/ksft.py | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/kselftest/ksft.py 
>> b/tools/testing/selftests/kselftest/ksft.py
>> index cd89fb2bc10e..bf215790a89d 100644
>> --- a/tools/testing/selftests/kselftest/ksft.py
>> +++ b/tools/testing/selftests/kselftest/ksft.py
>> @@ -70,7 +70,7 @@ def test_result(condition, description=""):
>>   def finished():
>> -    if ksft_cnt["pass"] == ksft_num_tests:
>> +    if ksft_cnt["pass"] + ksft_cnt["skip"] == ksft_num_tests:
>>           exit_code = KSFT_PASS
> 
> Laura and Nícolas,
> 
> I saw both your emails explaining how this fixes the problem in
> a previous patch.
> 
> However looks like you haven't see my response about the implications
> of the exit_code = KSFT_PASS when tests are skipped.
> 
> if ksft_cnt["pass"] + ksft_cnt["skip"] == ksft_num_tests:
>>           exit_code = KSFT_PASS
> 
> Let me reiterate in case you missed it:
> 
> There is a reason why you don't want to mark all tests passed
> when there are several skips.Skips are an indication that
> there are several tests and/or test cases that couldn't not
> be run because of unmet dependencies. This condition needs
> to be investigated to see if there are any config options
> that could be enabled to get a better coverage.
> 
> Including skips to determine pass gives a false sense security
> that all is well when it isn't.
> 
> So it is incorrect to set the exit code to KSFT_PASS when there
> are skipped tests.

Just to be clear, the logic in ksft_finished() in kselftest.h (the C
helper) is to exit with KSFT_PASS when there are only passed and skipped 
tests. You're suggesting we change it to exit with KSFT_FAIL in that 
case?

Under this new logic, the runner would effectively treat skips as 
failures, impacting existing kselftests.

> +    if ksft_cnt["pass"] + ksft_cnt["skip"] == ksft_num_tests:
> 
> 
>>       else:
>>           exit_code = KSFT_FAIL
> 
> The logic here seems to not take into account when you have a
> conditions where you have a mixed results of passed tests,
> skipped tests, and failed tests.
>
> Please revisit and figure out how to address this and report
> the status correctly.

The logic ensures that if there’s a mix of passed, skipped, and failed 
tests, the exit code returned is KSFT_FAIL. I believe that if there is at 
least one failed test, the overall test should be reported as failed, 
which is what happens in this case.

Thanks,

Laura