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

[John Snow <jsnow@redhat.com>]

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.

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.

> - 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.

>   #
> @@ -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 [...]'

> +    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>