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

[Willian Rampazzo]

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.

> +    except KeyError:
> +        env['PYTHONPATH'] = '../../python/'

Same here. You could do it once, before the 'try' and use it inside.

Other than that,

Reviewed-by: Willian Rampazzo <willianr@redhat.com>