[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach
From: |
Willian Rampazzo |
Subject: |
Re: [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach |
Date: |
Fri, 15 Jan 2021 16:27:00 -0300 |
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>
- [PATCH v4 00/10] iotests: Fix 129 and expand 297’s reach, Max Reitz, 2021/01/15
- [PATCH v4 01/10] iotests.py: Assume a couple of variables as given, Max Reitz, 2021/01/15
- [PATCH v4 04/10] iotests/129: Remove test images in tearDown(), Max Reitz, 2021/01/15
- [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach, Max Reitz, 2021/01/15
- Re: [PATCH v4 02/10] iotests/297: Rewrite in Python and extend reach,
Willian Rampazzo <=
- [PATCH v4 03/10] iotests: Move try_remove to iotests.py, Max Reitz, 2021/01/15
- [PATCH v4 05/10] iotests/129: Do not check @busy, Max Reitz, 2021/01/15
- [PATCH v4 09/10] iotests/129: Clean up pylint and mypy complaints, Max Reitz, 2021/01/15
- [PATCH v4 06/10] iotests/129: Use throttle node, Max Reitz, 2021/01/15
- [PATCH v4 08/10] iotests/129: Limit mirror job's buffer size, Max Reitz, 2021/01/15
- [PATCH v4 10/10] iotests/300: Clean up pylint and mypy complaints, Max Reitz, 2021/01/15