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

[Max Reitz]

On 26.01.21 22:28, John Snow wrote:
> On 1/18/21 5:57 AM, 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.)
>
> Chiming in to say that I tried to tackle this before; I wrote some 
> preliminary cleanups and sent to the list as an "WIP RFC" or something 
> like that in earlyish 2020. I paid attention to qed.py and the other 
> non-numerical files.
>
> Maybe badly rotted by now, I don't know.
>
> > (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.)
>
> I agree. Stop the bleeding first and worry about the rest after.
>
> > 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.
>
> Does this solve an observed problem, or is it preventative? I ran into 
> trouble once by pointing mypy to my system python libraries; it seemed 
> to have a check that explicitly warned me against such tricks.

Yes, that happens when you derive it from sys.path.  (Which I tried at 
one point, and then ran into that exact problem.)

PYTHONPATH generally doesn’t include the system libraries, though, 
generally it shouldn’t even be set for the iotests.  So the only thing 
that’s put in there is ../../python/, and we need that in MYPYPATH, too.

(As I wrote, perhaps in the future the check script will add 
../../python/, so we don’t need to do that anywhere in the iotests, and 
then it makes more sense why MYPYPATH should just be $PYTHONPATH.)

> I guess for now, if it works, it works. :o)
>
> > - Passing --notes=FIXME,XXX to pylint suppresses warnings for TODO
> >    comments.  TODO is fine, we do not need 297 to complain about such
> >    comments.
>
> Agreed. You can also edit pylintrc to choose which keywords trigger the 
> check -- "TODO" is probably fine, but "FIXME" is maybe a shade worse. 
> Season to taste.

Yes, definitely a matter of taste.  I kind of like pylint to complain 
about TODO when I’m running it explicitly, so, well.

> > - 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     | 112 +++++++++++++++++++++++++++++--------
> >   tests/qemu-iotests/297.out |   5 +-
> >   2 files changed, 92 insertions(+), 25 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 5c5420712b..e3db3e061e 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.
>
> You could bump it up, if you wanted.

Do I, though? :)

> >   #
> > @@ -15,30 +15,98 @@
> >   # 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()
> > +    qemu_module_path = os.path.join(os.path.dirname(__file__),
> > +                                    '..', '..', 'python')
> > +    try:
> > +        env['PYTHONPATH'] += os.pathsep + qemu_module_path
> > +    except KeyError:
> > +        env['PYTHONPATH'] = qemu_module_path
> > +    subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', 
> > *files),
> > +                   env=env, check=False)
> > +
>
> Is there a reason to use 'pylint-3' here? I realize that this is not 
> something you are introducing, but it jogged the question loose for me.
>
> 'pylint-3' is, I believe, a fedora-ism. The pip package for pylint 
> installs only a 'pylint' script. It might be better to just use it 
> without the suffix.
>
> If we are concerned about accidentally invoking a python 2 version of 
> pylint that might be installed on a system, I'd then recommend doing 
> something like this:
>
> 'python3 -m pylint [...]'

Sounds reasonable to me (can’t find pylint-3 on Arch, for example).  As 
a follow-up, that is, because I’m afraid I’ve already put this series in 
a pull request...

Max

> > +    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,
> > +                           universal_newlines=True)
> > +
> > +        if p.returncode != 0:
> > +            print(p.stdout)
> > +
> > +
> > +for linter in ('pylint-3', 'mypy'):
> > +    if shutil.which(linter) is None:
> > +        iotests.notrun(f'{linter} not found')
> > +
> > +iotests.script_main(run_linters)
> > diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
> > index 6acc843649..f2e1314d10 100644
> > --- a/tests/qemu-iotests/297.out
> > +++ b/tests/qemu-iotests/297.out
> > @@ -1,3 +1,2 @@
> > -QA output created by 297
> > -Success: no issues found in 1 source file
> > -*** done
> > +=== pylint ===
> > +=== mypy ===
>
> Reviewed-by: John Snow <jsnow@redhat.com>