On 2020-09-14 4:09 p.m., Omar Sandoval wrote:
>> I noticed this too during my review, but based on my read of the current
>> blktest code, the return value of the requires() function is not
>> actually used. It seems to only check if SKIP_REASON is set.
>>
>> If we want to ensure the return value of requires() is correct, perhaps
>> we should check it after we call it and then consult SKIP_REASON? And
>> WARN or fail if SKIP_REASON is set when requires() didn't return 1.
> Oops, you're right, I actually changed this a few months ago in
> 4824ac3f5c4a ("Skip tests based on SKIP_REASON, not return value").
> Totally forgot about that :) IMO it's still cleaner to chain them
> together.
In my opinion, this creates a bit of confusion when reading the code.
The &&s make it look like the return value is important when, in fact,
it is not.
It is also easy to make the assumption that adding any bash command to
the && list will skip the test -- however that is not the case and
people may introduce subtle bugs that look correct, but go unnoticed.
Logan
