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

[Max Reitz <mreitz@redhat.com>]

On 15.01.21 12:13, Vladimir Sementsov-Ogievskiy wrote:
> 14.01.2021 20:02, Max Reitz 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>
>> ---
>>   tests/qemu-iotests/297     | 109 +++++++++++++++++++++++++++++--------
>>   tests/qemu-iotests/297.out |   5 +-
>>   2 files changed, 89 insertions(+), 25 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>> index 5c5420712b..bfa26d280b 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,95 @@
>>   # 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
> 
> PEP8 asks for two spaces before inline comment

Wow.  I really hate PEP8.

>> +            return False
>> +
> 
> and two empty lines here
> 
> (good ALE vim plugin runs flake8, mypy and pylint asynchronously for me 
> and shows these things)

I’d like to argue that that isn’t good, but I need to quench my anger. 
Perhaps one day I can quench it sufficiently to install ALE myself.

>> +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)))
> 
> O, good.
> 
>> +
>> +    print('=== pylint ===')
>> +    sys.stdout.flush()
> 
> Should not hurt.. But why? Should be flushed anyway as print will put a 
> '\n'.. And why you care about it at all?

When pylint fails, I can see its result above the '=== pylint ===' line, 
even though its output is on stdout.  I suspect the Python output indeed 
isn’t flushed on \n.

(Test it by dropping the flush(), and then also e.g. 041 from the ignore 
list.  Perhaps you get a different result, but for me, the errors appear 
above the '=== pylint ===' line.)

>> +
>> +    # Todo notes are fine, but fixme's or xxx's should probably just be
>> +    # fixed (in tests, at least)
>> +    env = os.environ
>> +    try:
>> +        env['PYTHONPATH'] += ':../../python/'
>> +    except KeyError:
>> +        env['PYTHONPATH'] = '../../python/'
>> +    subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', 
>> *files),
>> +                   env=env, check=False)
> 
> as I understand, you don't need env argument, as os.environ is inherited 
> by default.

Yes, but os.environ['PYTHONPATH'] doesn’t include ../../python/, which 
is why I copy it.

On second though, I don’t copy it.  I think the “env = os.environ” line 
should be “env = os.environ.copy()”, actually, or I’ll modify 297’s own 
PYTHONPATH.

>> +
>> +    print('=== mypy ===')
>> +    sys.stdout.flush()
>> +
>> +    # We have to call mypy separately for each file.  Otherwise, it
>> +    # will interpret all given files as belonging together (i.e., they
>> +    # may not both define the same classes, etc.; most notably, they
>> +    # must not both define the __main__ module).
>> +    env['MYPYPATH'] = env['PYTHONPATH']
>> +    for filename in files:
>> +        p = subprocess.run(('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',
>> +                            filename),
>> +                           env=env,
>> +                           check=False,
>> +                           stdout=subprocess.PIPE,
>> +                           stderr=subprocess.STDOUT,
>> +                           text=True)
> 
> Can you really use "text" ? We require python 3.6 (in check), but text 
> comes from 3.7..

Oh well.  My fault for just reading the current docs.

> It may break some test environments. I think we need old good 
> universal_newlines=True which is the same.

Good enough for me.

> With s/text/universal_newlines/:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks!  I’ll clean up the PEP8 violations, make the “env = os.environ” 
line an “env = os.environ.copy()”, and use universal_newlines here.

Max