| Message ID | 20210115174315.30949-3-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: Fix 129 and expand 297’s reach | expand |
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>
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
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.
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/' + except KeyError: + env['PYTHONPATH'] = '../../python/' + subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files), + env=env, check=False) + + 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 ===