Re: [PATCH v2 1/8] iotests/297: Allow checking all Python test files

[Max Reitz <mreitz@redhat.com>]

On 13.01.21 20:01, Eric Blake wrote:
> On 1/13/21 11:57 AM, Max Reitz wrote:
>> I.e., all Python files in the qemu-iotests/ directory.
>>
>> Most files of course do not pass, so there is an extensive skip list for
>> now.  (The only files that do pass are 209, 254, 283, and iotests.py.)
>>
>> (Alternatively, we could have the opposite, i.e. an explicit list of
>> files that we do want to check, but I think it is better to check files
>> by default.)
> 
> Concur with the choice for default.
> 
>>
>> I decided to include the list of files checked in the reference output,
>> so we do not accidentally lose coverage of anything.  That means adding
>> new Python tests will require a change to 297.out, but that should not
>> be a problem.
> 
> I'm okay with that.
> 
>>
>> On the other hand, I decided to hide mypy's "Success" lines from the
>> reference output, because they do not add anything useful.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/297     | 66 ++++++++++++++++++++++++++++++++++----
>>   tests/qemu-iotests/297.out |  6 +++-
>>   2 files changed, 65 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>> index 5c5420712b..b1a7d6d5e8 100755
>> --- a/tests/qemu-iotests/297
>> +++ b/tests/qemu-iotests/297
>> @@ -30,13 +30,67 @@ if ! type -p "mypy" > /dev/null; then
>>       _notrun "mypy not found"
>>   fi
>>   
>> -pylint-3 --score=n iotests.py
>> +# TODO: Empty this list!
> 
> :)
> 
> 
>> +file_list=()
>> +for file in *; do
>> +    # Check files with a .py extension or a Python shebang
>> +    # (Unless they are in the skip_files list)
>> +    if [ -f "$file" ] && ((echo "$file" | grep -q '\.py$') ||
>> +                          (head -n 1 "$file" | grep -q '^#!.*python'))
> 
> Bash has an (obsolete) operator (()) (behaves like a mix of $(()) and
> 'if'); when nesting subshells, POSIX recommends inserting a space to
> avoid inadvertent triggering of the alternate semantics of the operator.
>   But why do you need nested subshells?  This is equivalent:
> 
> if [ -f "$file" ] && (echo  "$file" | grep -q '\.py$' ||
>                        head -n 1 "$file" | grep -q '^#!.*python')

I just wasn’t sure of the order of pipe and ||.  Sure, I can change it 
(depending on whether or not I rewrite it in Python, as suggested by 
Vladimir).

>> +    then
>> +        skip_file=false
>> +        for skip in "${skip_files[@]}"; do
> 
> bashism, but iotests require bash, so fine.
> 
>> +            if [ "$skip" = "$file" ]; then
>> +                skip_file=true
>> +                break
>> +            fi
>> +        done
>> +
>> +        if ! $skip_file; then
>> +            file_list+=("$file")
> 
> Likewise.
> 
> Whether or not you strip the extra (),
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!