Re: [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach

[Max Reitz <mreitz@redhat.com>]

On 15.01.21 20:27, Willian Rampazzo wrote:
> On Fri, Jan 15, 2021 at 2:43 PM Max Reitz <mreitz@redhat.com> wrote:
>>
>> Instead of checking iotests.py only, check all Python files in the
>> qemu-iotests/ directory.  Of course, most of them 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.)
>>
>> Unless started in debug mode (./check -d), the output has no information
>> on which files are tested, so we will not have a problem e.g. with
>> backports, where some files may be missing when compared to upstream.
>>
>> Besides the technical rewrite, some more things are changed:
>>
>> - For the pylint invocation, PYTHONPATH is adjusted.  This mirrors
>>    setting MYPYPATH for mypy.
>>
>> - Also, MYPYPATH is now derived from PYTHONPATH, so that we include
>>    paths set by the environment.  Maybe at some point we want to let the
>>    check script add '../../python/' to PYTHONPATH so that iotests.py does
>>    not need to do that.
>>
>> - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
>>    comments.  TODO is fine, we do not need 297 to complain about such
>>    comments.
>>
>> - The "Success" line from mypy's output is suppressed, because (A) it
>>    does not add useful information, and (B) it would leak information
>>    about the files having been tested to the reference output, which we
>>    decidedly do not want.
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/297     | 110 +++++++++++++++++++++++++++++--------
>>   tests/qemu-iotests/297.out |   5 +-
>>   2 files changed, 90 insertions(+), 25 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>> index 5c5420712b..fa9e2cac78 100755
>> --- a/tests/qemu-iotests/297
>> +++ b/tests/qemu-iotests/297
>> @@ -1,4 +1,4 @@
>> -#!/usr/bin/env bash
>> +#!/usr/bin/env python3
>>   #
>>   # Copyright (C) 2020 Red Hat, Inc.
>>   #
>> @@ -15,30 +15,96 @@
>>   # You should have received a copy of the GNU General Public License
>>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>
>> -seq=$(basename $0)
>> -echo "QA output created by $seq"
>> +import os
>> +import re
>> +import shutil
>> +import subprocess
>> +import sys
>>
>> -status=1       # failure is the default!
>> +import iotests
>>
>> -# get standard environment
>> -. ./common.rc
>>
>> -if ! type -p "pylint-3" > /dev/null; then
>> -    _notrun "pylint-3 not found"
>> -fi
>> -if ! type -p "mypy" > /dev/null; then
>> -    _notrun "mypy not found"
>> -fi
>> +# TODO: Empty this list!
>> +SKIP_FILES = (
>> +    '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
>> +    '096', '118', '124', '129', '132', '136', '139', '147', '148', '149',
>> +    '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
>> +    '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
>> +    '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
>> +    '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
>> +    '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
>> +    '299', '300', '302', '303', '304', '307',
>> +    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
>> +)
>>
>> -pylint-3 --score=n iotests.py
>>
>> -MYPYPATH=../../python/ mypy --warn-unused-configs --disallow-subclassing-any \
>> -    --disallow-any-generics --disallow-incomplete-defs \
>> -    --disallow-untyped-decorators --no-implicit-optional \
>> -    --warn-redundant-casts --warn-unused-ignores \
>> -    --no-implicit-reexport iotests.py
>> +def is_python_file(filename):
>> +    if not os.path.isfile(filename):
>> +        return False
>>
>> -# success, all done
>> -echo "*** done"
>> -rm -f $seq.full
>> -status=0
>> +    if filename.endswith('.py'):
>> +        return True
>> +
>> +    with open(filename) as f:
>> +        try:
>> +            first_line = f.readline()
>> +            return re.match('^#!.*python', first_line) is not None
>> +        except UnicodeDecodeError:  # Ignore binary files
>> +            return False
>> +
>> +
>> +def run_linters():
>> +    files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
>> +             if is_python_file(filename)]
>> +
>> +    iotests.logger.debug('Files to be checked:')
>> +    iotests.logger.debug(', '.join(sorted(files)))
>> +
>> +    print('=== pylint ===')
>> +    sys.stdout.flush()
>> +
>> +    # Todo notes are fine, but fixme's or xxx's should probably just be
>> +    # fixed (in tests, at least)
>> +    env = os.environ.copy()
>> +    try:
>> +        env['PYTHONPATH'] += ':../../python/'
> 
> Do you have any objection to using os.path.dirname and os.path.join
> here? This would make the code more pythonic.

Intuitively, I felt a bit uneasy about os.path.join here, because it 
would make it look like this was platform-independent, when it is not: 
The colon as a PATH separator is probably more platform-dependent than 
the slashes.

So turns out there is os.pathsep, which yields a colon on e.g. Linux and 
a semicolon on e.g. Windows.

I wondered if

   env['PYTHONPATH'] = os.pathsep.join(sys.path)

wouldn’t be the most simple solution, but seems like mypy doesn’t like 
that very much for the MYPYPATH.  Too bad.

Guess the try-except block will have to remain, then.
(Just with os.pathsep instead of a colon, and with 
os.path.join(os.path.dirname(__file__), ...).)

Max