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

[Willian Rampazzo <wrampazz@redhat.com>]

On 1/18/21 7:09 AM, Max Reitz wrote:
> 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
> 

Sorry to take so long to answer, I was on vacation. I see v5 was already 
merged. I took a look at it and the developed solution looks good, 
thanks for all your effort here.