[PATCH v2 00/17] python/iotests: Run iotest linters during Python CI

[PATCH v2 00/17] python/iotests: Run iotest linters during Python CI

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest
CI: https://gitlab.com/jsnow/qemu/-/pipelines/340144191

Since iotests are such a heavy and prominent user of the Python qemu.qmp
and qemu.machine packages, it would be convenient if the Python linting
suite also checked this client for any possible regressions introduced
by shifting around signatures, types, or interfaces in these packages.

(We'd eventually find those problems when iotest 297 ran, but with
increasing distance between Python development and Block development,
the risk of an accidental breakage in this regard increases. I,
personally, know to run iotests (and especially 297) after changing
Python code, but not everyone in the future might. Plus, I am lazy, and
I like only having to push one button.)

Add the ability for the Python CI to run the iotest linters too, which
means that the iotest linters would be checked against:

- Python 3.6, using a frozen set of linting packages at their oldest
  supported versions, using 'pipenv'
- Python 3.6 through Python 3.10 inclusive, using 'tox' and the latest
  versions of mypy/pylint that happen to be installed during test
  time. This CI test is allowed to fail with a warning, and can serve
  as a bellwether for when new incompatible changes may disrupt the
  linters. Testing against old and new Python interpreters alike can
  help surface incompatibility issues we may need to be aware of.)

Here are example outputs of those CI jobs with this series applied:
 - "check-python-pipenv": https://gitlab.com/jsnow/qemu/-/jobs/1377735087
 - "check-python-tox": https://gitlab.com/jsnow/qemu/-/jobs/1377735088

You can also run these same tests locally from ./python, plus one more:

- "make check-dev" to test against whatever python you have.
- "make check-pipenv", if you have Python 3.6 and pipenv installed.
- "make check-tox", if you have Python 3.6 through Python 3.10 installed.

See the old commit message for more sample output, etc.

https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg07056.html

V2:
 - Added patches 1-5 which do some more delinting.
 - Added patch 8, which scans subdirs for tests to lint.
 - Added patch 17, which improves the speed of mypy analysis.
 - Patch 14 is different because of the new patch 8.

Unreviewed patches:

[01] iotests-use-with-statement-for # [SOB] JS
[02] iotests-use-subprocess.devnull # [SOB] JS
[03] iotests-mirror-top-perms       # [SOB] JS
[04] iotests-migrate-bitmaps        # [SOB] JS
[05] iotests-migrate-bitmaps-test   # [SOB] JS
[07] iotests-297-add-get_files      # [SOB] JS
[08] wip-make-the-test-finding      # [SOB] JS
[14] iotests-297-split-linters-py   # [SOB] JS
[17] iotests-297-check-mypy-files   # [SOB] JS

--js

John Snow (17):
  iotests: use with-statement for open() calls
  iotests: use subprocess.DEVNULL instead of open("/dev/null")
  iotests/mirror-top-perms: Adjust import paths
  iotests/migrate-bitmaps-postcopy-test: declare instance variables
  iotests/migrate-bitmaps-test: delint
  iotests/297: modify is_python_file to work from any CWD
  iotests/297: Add get_files() function
  iotests/297: Include sub-directories when finding tests to lint
  iotests/297: Don't rely on distro-specific linter binaries
  iotests/297: Create main() function
  iotests/297: Separate environment setup from test execution
  iotests/297: Add 'directory' argument to run_linters
  iotests/297: return error code from run_linters()
  iotests/297: split linters.py off from 297
  iotests/linters: Add entry point for Python CI linters
  python: Add iotest linters to test suite
  iotests/linters: check mypy files all at once

 python/tests/iotests.sh                       |   2 +
 tests/qemu-iotests/297                        |  80 ++---------
 tests/qemu-iotests/iotests.py                 |  21 +--
 tests/qemu-iotests/linters.py                 | 130 ++++++++++++++++++
 .../tests/migrate-bitmaps-postcopy-test       |   3 +
 tests/qemu-iotests/tests/migrate-bitmaps-test |  70 +++++-----
 tests/qemu-iotests/tests/mirror-top-perms     |   7 +-
 7 files changed, 198 insertions(+), 115 deletions(-)
 create mode 100755 python/tests/iotests.sh
 create mode 100755 tests/qemu-iotests/linters.py

-- 
2.31.1

[PATCH v2 01/17] iotests: use with-statement for open() calls

[John Snow]

Silences a new pylint warning. The dangers of *not* doing this are
somewhat unclear; I believe the file object gets garbage collected
eventually, but possibly the way in which it happens is
non-deterministic. Maybe this is a valid warning, but if there are
consequences of not doing it, I am not aware of them at present.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 89663dac06d..beadf5c821c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1075,7 +1075,8 @@ def notrun(reason):
     # Each test in qemu-iotests has a number ("seq")
     seq = os.path.basename(sys.argv[0])
 
-    open('%s/%s.notrun' % (output_dir, seq), 'w').write(reason + '\n')
+    with open('%s/%s.notrun' % (output_dir, seq), 'w') as outfile:
+        outfile.write(reason + '\n')
     logger.warning("%s not run: %s", seq, reason)
     sys.exit(0)
 
@@ -1088,8 +1089,8 @@ def case_notrun(reason):
     # Each test in qemu-iotests has a number ("seq")
     seq = os.path.basename(sys.argv[0])
 
-    open('%s/%s.casenotrun' % (output_dir, seq), 'a').write(
-        '    [case not run] ' + reason + '\n')
+    with open('%s/%s.casenotrun' % (output_dir, seq), 'a') as outfile:
+        outfile.write('    [case not run] ' + reason + '\n')
 
 def _verify_image_format(supported_fmts: Sequence[str] = (),
                          unsupported_fmts: Sequence[str] = ()) -> None:
-- 
2.31.1

[PATCH v2 02/17] iotests: use subprocess.DEVNULL instead of open("/dev/null")

[John Snow]

Avoids a warning from pylint not to use open() outside of a
with-statement, and is ... probably more portable anyway. Not that I
think we care too much about running tests *on* Windows, but... eh.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index beadf5c821c..64f9b688f55 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -219,18 +219,18 @@ def qemu_io_silent(*args):
         default_args = qemu_io_args
 
     args = default_args + list(args)
-    exitcode = subprocess.call(args, stdout=open('/dev/null', 'w'))
-    if exitcode < 0:
+    result = subprocess.run(args, stdout=subprocess.DEVNULL, check=False)
+    if result.returncode < 0:
         sys.stderr.write('qemu-io received signal %i: %s\n' %
-                         (-exitcode, ' '.join(args)))
-    return exitcode
+                         (-result.returncode, ' '.join(args)))
+    return result.returncode
 
 def qemu_io_silent_check(*args):
     '''Run qemu-io and return the true if subprocess returned 0'''
     args = qemu_io_args + list(args)
-    exitcode = subprocess.call(args, stdout=open('/dev/null', 'w'),
-                               stderr=subprocess.STDOUT)
-    return exitcode == 0
+    result = subprocess.run(args, stdout=subprocess.DEVNULL,
+                            stderr=subprocess.STDOUT, check=False)
+    return result.returncode == 0
 
 class QemuIoInteractive:
     def __init__(self, *args):
-- 
2.31.1

[PATCH v2 03/17] iotests/mirror-top-perms: Adjust import paths

[John Snow]

Technically AbnormalShutdown isn't exported via
'qemu.machine.AbnormalShutdown', but instead from
'qemu.machine.machine.AbnormalShutdown'. Odd, I know, and it's on the
list to fix. For now, change the imports here.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/tests/mirror-top-perms | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/tests/mirror-top-perms b/tests/qemu-iotests/tests/mirror-top-perms
index 451a0666f81..de181825901 100755
--- a/tests/qemu-iotests/tests/mirror-top-perms
+++ b/tests/qemu-iotests/tests/mirror-top-perms
@@ -25,7 +25,8 @@ from iotests import qemu_img
 
 # Import qemu after iotests.py has amended sys.path
 # pylint: disable=wrong-import-order
-import qemu
+from qemu import qmp
+from qemu.machine import machine
 
 
 image_size = 1 * 1024 * 1024
@@ -47,7 +48,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
     def tearDown(self):
         try:
             self.vm.shutdown()
-        except qemu.machine.AbnormalShutdown:
+        except machine.AbnormalShutdown:
             pass
 
         if self.vm_b is not None:
@@ -102,7 +103,7 @@ class TestMirrorTopPerms(iotests.QMPTestCase):
             self.vm_b.launch()
             print('ERROR: VM B launched successfully, this should not have '
                   'happened')
-        except qemu.qmp.QMPConnectError:
+        except qmp.QMPConnectError:
             assert 'Is another process using the image' in self.vm_b.get_log()
 
         result = self.vm.qmp('block-job-cancel',
-- 
2.31.1

[PATCH v2 04/17] iotests/migrate-bitmaps-postcopy-test: declare instance variables

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
index 584062b4128..78dc19e3e3d 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
@@ -115,6 +115,9 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
         self.vm_a_events = []
         self.vm_b_events = []
 
+        self.discards1_sha256: str
+        self.all_discards_sha256: str
+
     def start_postcopy(self):
         """ Run migration until RESUME event on target. Return this event. """
         for i in range(nb_bitmaps):
-- 
2.31.1

[PATCH v2 05/17] iotests/migrate-bitmaps-test: delint

[John Snow]

Mostly uninteresting stuff. Move the test injections under a function
named main() so that the variables used during that process aren't in
the global scope.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/tests/migrate-bitmaps-test | 70 +++++++++++--------
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test b/tests/qemu-iotests/tests/migrate-bitmaps-test
index a5c7bc83e0e..fd36ee4a245 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test
@@ -19,12 +19,12 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
-import os
-import iotests
-import time
 import itertools
 import operator
+import os
 import re
+
+import iotests
 from iotests import qemu_img, qemu_img_create, Timeout
 
 
@@ -62,9 +62,10 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
             params['persistent'] = True
 
         result = vm.qmp('block-dirty-bitmap-add', **params)
-        self.assert_qmp(result, 'return', {});
+        self.assert_qmp(result, 'return', {})
 
-    def get_bitmap_hash(self, vm):
+    @classmethod
+    def get_bitmap_hash(cls, vm):
         result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
                         node='drive0', name='bitmap0')
         return result['return']['sha256']
@@ -73,10 +74,10 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
         result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
                         node='drive0', name='bitmap0')
         if sha256:
-            self.assert_qmp(result, 'return/sha256', sha256);
+            self.assert_qmp(result, 'return/sha256', sha256)
         else:
             self.assert_qmp(result, 'error/desc',
-                            "Dirty bitmap 'bitmap0' not found");
+                            "Dirty bitmap 'bitmap0' not found")
 
     def do_test_migration_resume_source(self, persistent, migrate_bitmaps):
         granularity = 512
@@ -106,7 +107,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
                 break
         while True:
             result = self.vm_a.qmp('query-status')
-            if (result['return']['status'] == 'postmigrate'):
+            if result['return']['status'] == 'postmigrate':
                 break
 
         # test that bitmap is still here
@@ -216,25 +217,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 
 def inject_test_case(klass, name, method, *args, **kwargs):
     mc = operator.methodcaller(method, *args, **kwargs)
-    setattr(klass, 'test_' + method + name, lambda self: mc(self))
-
-for cmb in list(itertools.product((True, False), repeat=5)):
-    name = ('_' if cmb[0] else '_not_') + 'persistent_'
-    name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
-    name += '_online' if cmb[2] else '_offline'
-    name += '_shared' if cmb[3] else '_nonshared'
-    if (cmb[4]):
-        name += '__pre_shutdown'
-
-    inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
-                     *list(cmb))
-
-for cmb in list(itertools.product((True, False), repeat=2)):
-    name = ('_' if cmb[0] else '_not_') + 'persistent_'
-    name += ('_' if cmb[1] else '_not_') + 'migbitmap'
-
-    inject_test_case(TestDirtyBitmapMigration, name,
-                     'do_test_migration_resume_source', *list(cmb))
+    setattr(klass, 'test_' + method + name, mc)
 
 
 class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
@@ -270,7 +253,8 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         # Check that the bitmaps are there
-        for node in self.vm.qmp('query-named-block-nodes', flat=True)['return']:
+        nodes = self.vm.qmp('query-named-block-nodes', flat=True)['return']
+        for node in nodes:
             if 'node0' in node['node-name']:
                 self.assert_qmp(node, 'dirty-bitmaps[0]/name', 'bmap0')
 
@@ -287,7 +271,7 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
         """
         Continue the source after migration.
         """
-        result = self.vm.qmp('migrate', uri=f'exec: cat > /dev/null')
+        result = self.vm.qmp('migrate', uri='exec: cat > /dev/null')
         self.assert_qmp(result, 'return', {})
 
         with Timeout(10, 'Migration timeout'):
@@ -297,6 +281,30 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
 
+def main():
+    for cmb in list(itertools.product((True, False), repeat=5)):
+        name = ('_' if cmb[0] else '_not_') + 'persistent_'
+        name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
+        name += '_online' if cmb[2] else '_offline'
+        name += '_shared' if cmb[3] else '_nonshared'
+        if cmb[4]:
+            name += '__pre_shutdown'
+
+        inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
+                         *list(cmb))
+
+    for cmb in list(itertools.product((True, False), repeat=2)):
+        name = ('_' if cmb[0] else '_not_') + 'persistent_'
+        name += ('_' if cmb[1] else '_not_') + 'migbitmap'
+
+        inject_test_case(TestDirtyBitmapMigration, name,
+                         'do_test_migration_resume_source', *list(cmb))
+
+    iotests.main(
+        supported_fmts=['qcow2'],
+        supported_protocols=['file']
+    )
+
+
 if __name__ == '__main__':
-    iotests.main(supported_fmts=['qcow2'],
-                 supported_protocols=['file'])
+    main()
-- 
2.31.1

[PATCH v2 06/17] iotests/297: modify is_python_file to work from any CWD

[John Snow]

Add a directory argument to is_python_file to allow it to work correctly
no matter what CWD we happen to run it from. This is done in
anticipation of running the iotests from another directory (./python/).

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 433b7323368..493dda17fb6 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -39,14 +39,16 @@ SKIP_FILES = (
 )
 
 
-def is_python_file(filename):
-    if not os.path.isfile(filename):
+def is_python_file(filename: str, directory: str = '.') -> bool:
+    filepath = os.path.join(directory, filename)
+
+    if not os.path.isfile(filepath):
         return False
 
     if filename.endswith('.py'):
         return True
 
-    with open(filename) as f:
+    with open(filepath) as f:
         try:
             first_line = f.readline()
             return re.match('^#!.*python', first_line) is not None
-- 
2.31.1

[PATCH v2 07/17] iotests/297: Add get_files() function

[John Snow]

Split out file discovery into its own method to begin separating out the
"environment setup" and "test execution" phases.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297 | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 493dda17fb6..0bc11958059 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,6 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
+from typing import List
 
 import iotests
 
@@ -56,9 +57,15 @@ def is_python_file(filename: str, directory: str = '.') -> bool:
             return False
 
 
+def get_test_files(directory: str = '.') -> List[str]:
+    return [
+        f for f in (set(os.listdir(directory)) - set(SKIP_FILES))
+        if is_python_file(f, directory)
+    ]
+
+
 def run_linters():
-    files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
-             if is_python_file(filename)]
+    files = get_test_files()
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
-- 
2.31.1

[PATCH v2 08/17] iotests/297: Include sub-directories when finding tests to lint

[John Snow]

Choosing to interpret the SKIP_FILES list as a list of filenames instead
of a list of paths, to keep things simple.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297 | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 0bc11958059..665ac0aa361 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -40,13 +40,8 @@ SKIP_FILES = (
 )
 
 
-def is_python_file(filename: str, directory: str = '.') -> bool:
-    filepath = os.path.join(directory, filename)
-
-    if not os.path.isfile(filepath):
-        return False
-
-    if filename.endswith('.py'):
+def is_python_file(filepath: str) -> bool:
+    if filepath.endswith('.py'):
         return True
 
     with open(filepath) as f:
@@ -58,10 +53,20 @@ def is_python_file(filename: str, directory: str = '.') -> bool:
 
 
 def get_test_files(directory: str = '.') -> List[str]:
-    return [
-        f for f in (set(os.listdir(directory)) - set(SKIP_FILES))
-        if is_python_file(f, directory)
-    ]
+    files = []
+
+    iotests.logger.debug("get_test_files(%s)", directory)
+    for dirent in os.scandir(directory):
+        if dirent.name in SKIP_FILES:
+            continue
+
+        relpath = os.path.join(directory, dirent.name)
+        if dirent.is_dir():
+            files.extend(get_test_files(relpath))
+        elif is_python_file(relpath):
+            files.append(relpath)
+
+    return files
 
 
 def run_linters():
-- 
2.31.1

[PATCH v2 09/17] iotests/297: Don't rely on distro-specific linter binaries

[John Snow]

'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
mypy" to access these scripts instead. This style of invocation will
prefer the "correct" tool when run in a virtual environment.

Note that we still check for "pylint-3" before the test begins -- this
check is now "overly strict", but shouldn't cause anything that was
already running correctly to start failing.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 45 ++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 665ac0aa361..dc4460dd84b 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -87,8 +87,11 @@ def run_linters():
         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)
+    subprocess.run(
+        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        env=env,
+        check=False,
+    )
 
     print('=== mypy ===')
     sys.stdout.flush()
@@ -99,23 +102,27 @@ def run_linters():
     # 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',
-                            '--namespace-packages',
-                            filename),
-                           env=env,
-                           check=False,
-                           stdout=subprocess.PIPE,
-                           stderr=subprocess.STDOUT,
-                           universal_newlines=True)
+        p = subprocess.run(
+            (
+                'python3', '-m', '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',
+                '--namespace-packages',
+                filename,
+            ),
+            env=env,
+            check=False,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+            universal_newlines=True
+        )
 
         if p.returncode != 0:
             print(p.stdout)
-- 
2.31.1

[PATCH v2 10/17] iotests/297: Create main() function

[John Snow]

Instead of running "run_linters" directly, create a main() function that
will be responsible for environment setup, leaving run_linters()
responsible only for execution of the linters.

(That environment setup will be moved over in forthcoming commits.)

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index dc4460dd84b..7ef4cdc7615 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -128,8 +128,12 @@ def run_linters():
             print(p.stdout)
 
 
-for linter in ('pylint-3', 'mypy'):
-    if shutil.which(linter) is None:
-        iotests.notrun(f'{linter} not found')
+def main() -> None:
+    for linter in ('pylint-3', 'mypy'):
+        if shutil.which(linter) is None:
+            iotests.notrun(f'{linter} not found')
 
-iotests.script_main(run_linters)
+    run_linters()
+
+
+iotests.script_main(main)
-- 
2.31.1

[PATCH v2 11/17] iotests/297: Separate environment setup from test execution

[John Snow]

Move environment setup into main(), leaving pure test execution behind
in run_linters().

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 7ef4cdc7615..961c9170d21 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -21,7 +21,7 @@ import re
 import shutil
 import subprocess
 import sys
-from typing import List
+from typing import List, Mapping, Optional
 
 import iotests
 
@@ -69,24 +69,16 @@ def get_test_files(directory: str = '.') -> List[str]:
     return files
 
 
-def run_linters():
-    files = get_test_files()
-
-    iotests.logger.debug('Files to be checked:')
-    iotests.logger.debug(', '.join(sorted(files)))
+def run_linters(
+    files: List[str],
+    env: Optional[Mapping[str, str]] = None,
+) -> None:
 
     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(
         ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
         env=env,
@@ -100,7 +92,6 @@ def run_linters():
     # 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(
             (
@@ -133,7 +124,22 @@ def main() -> None:
         if shutil.which(linter) is None:
             iotests.notrun(f'{linter} not found')
 
-    run_linters()
+    files = get_test_files()
+
+    iotests.logger.debug('Files to be checked:')
+    iotests.logger.debug(', '.join(sorted(files)))
+
+    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
+
+    env['MYPYPATH'] = env['PYTHONPATH']
+
+    run_linters(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH v2 12/17] iotests/297: Add 'directory' argument to run_linters

[John Snow]

Allow run_linters to work well if it's executed from a different
directory.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 961c9170d21..b2bf7928c5d 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -71,6 +71,7 @@ def get_test_files(directory: str = '.') -> List[str]:
 
 def run_linters(
     files: List[str],
+    directory: str = '.',
     env: Optional[Mapping[str, str]] = None,
 ) -> None:
 
@@ -81,6 +82,7 @@ def run_linters(
     # fixed (in tests, at least)
     subprocess.run(
         ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        cwd=directory,
         env=env,
         check=False,
     )
@@ -108,6 +110,7 @@ def run_linters(
                 '--namespace-packages',
                 filename,
             ),
+            cwd=directory,
             env=env,
             check=False,
             stdout=subprocess.PIPE,
-- 
2.31.1

[PATCH v2 13/17] iotests/297: return error code from run_linters()

[John Snow]

This turns run_linters() into a bit of a hybrid test; returning non-zero
on failed execution while also printing diffable information. This is
done for the benefit of the avocado simple test runner, which will soon
be attempting to execute this test from a different environment.

(Note: universal_newlines is added to the pylint invocation for type
consistency with the mypy run -- it's not strictly necessary, but it
avoids some typing errors caused by our re-use of the 'p' variable.)

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/297 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b2bf7928c5d..ca9ee72d2fa 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -73,19 +73,22 @@ def run_linters(
     files: List[str],
     directory: str = '.',
     env: Optional[Mapping[str, str]] = None,
-) -> None:
+) -> int:
+    ret = 0
 
     print('=== pylint ===')
     sys.stdout.flush()
 
     # Todo notes are fine, but fixme's or xxx's should probably just be
     # fixed (in tests, at least)
-    subprocess.run(
+    p = subprocess.run(
         ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
         cwd=directory,
         env=env,
         check=False,
+        universal_newlines=True,
     )
+    ret += p.returncode
 
     print('=== mypy ===')
     sys.stdout.flush()
@@ -118,9 +121,12 @@ def run_linters(
             universal_newlines=True
         )
 
+        ret += p.returncode
         if p.returncode != 0:
             print(p.stdout)
 
+    return ret
+
 
 def main() -> None:
     for linter in ('pylint-3', 'mypy'):
-- 
2.31.1

[PATCH v2 14/17] iotests/297: split linters.py off from 297

[John Snow]

Split the linter execution itself out from 297, leaving just environment
setup in 297. This is done so that non-iotest code can invoke the
linters without needing to worry about imports of unpackaged iotest
code.

Eventually, it should be possible to replace linters.py with mypy.ini
and pylintrc files that instruct these tools how to run properly in this
directory, but ... not yet, and not in this series.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297        | 110 ++-----------------------------
 tests/qemu-iotests/linters.py | 121 ++++++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+), 106 deletions(-)
 create mode 100755 tests/qemu-iotests/linters.py

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index ca9ee72d2fa..3d29af5b78a 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,115 +17,13 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import re
 import shutil
-import subprocess
-import sys
-from typing import List, Mapping, Optional
 
 import iotests
+import linters
 
 
-# TODO: Empty this list!
-SKIP_FILES = (
-    '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
-    '096', '118', '124', '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', '302', '303', '304', '307',
-    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
-)
-
-
-def is_python_file(filepath: str) -> bool:
-    if filepath.endswith('.py'):
-        return True
-
-    with open(filepath) as f:
-        try:
-            first_line = f.readline()
-            return re.match('^#!.*python', first_line) is not None
-        except UnicodeDecodeError:  # Ignore binary files
-            return False
-
-
-def get_test_files(directory: str = '.') -> List[str]:
-    files = []
-
-    iotests.logger.debug("get_test_files(%s)", directory)
-    for dirent in os.scandir(directory):
-        if dirent.name in SKIP_FILES:
-            continue
-
-        relpath = os.path.join(directory, dirent.name)
-        if dirent.is_dir():
-            files.extend(get_test_files(relpath))
-        elif is_python_file(relpath):
-            files.append(relpath)
-
-    return files
-
-
-def run_linters(
-    files: List[str],
-    directory: str = '.',
-    env: Optional[Mapping[str, str]] = None,
-) -> int:
-    ret = 0
-
-    print('=== pylint ===')
-    sys.stdout.flush()
-
-    # Todo notes are fine, but fixme's or xxx's should probably just be
-    # fixed (in tests, at least)
-    p = subprocess.run(
-        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
-        cwd=directory,
-        env=env,
-        check=False,
-        universal_newlines=True,
-    )
-    ret += p.returncode
-
-    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).
-    for filename in files:
-        p = subprocess.run(
-            (
-                'python3', '-m', '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',
-                '--namespace-packages',
-                filename,
-            ),
-            cwd=directory,
-            env=env,
-            check=False,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT,
-            universal_newlines=True
-        )
-
-        ret += p.returncode
-        if p.returncode != 0:
-            print(p.stdout)
-
-    return ret
+# Looking for the list of excluded tests? See linters.py !
 
 
 def main() -> None:
@@ -133,7 +31,7 @@ def main() -> None:
         if shutil.which(linter) is None:
             iotests.notrun(f'{linter} not found')
 
-    files = get_test_files()
+    files = linters.get_test_files()
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
@@ -148,7 +46,7 @@ def main() -> None:
 
     env['MYPYPATH'] = env['PYTHONPATH']
 
-    run_linters(files, env=env)
+    linters.run_linters(files, env=env)
 
 
 iotests.script_main(main)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100755
index 00000000000..e7e8baded02
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,121 @@
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import re
+import subprocess
+import sys
+from typing import List, Mapping, Optional
+
+
+# TODO: Empty this list!
+SKIP_FILES = (
+    '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
+    '096', '118', '124', '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', '302', '303', '304', '307',
+    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
+
+
+def is_python_file(filepath: str) -> bool:
+    if filepath.endswith('.py'):
+        return True
+
+    with open(filepath) as f:
+        try:
+            first_line = f.readline()
+            return re.match('^#!.*python', first_line) is not None
+        except UnicodeDecodeError:  # Ignore binary files
+            return False
+
+
+def get_test_files(directory: str = '.') -> List[str]:
+    files = []
+
+    for dirent in os.scandir(directory):
+        if dirent.name in SKIP_FILES:
+            continue
+
+        relpath = os.path.join(directory, dirent.name)
+        if dirent.is_dir():
+            files.extend(get_test_files(relpath))
+        elif is_python_file(relpath):
+            files.append(relpath)
+
+    return files
+
+
+def run_linters(
+    files: List[str],
+    directory: str = '.',
+    env: Optional[Mapping[str, str]] = None,
+) -> int:
+    ret = 0
+
+    print('=== pylint ===')
+    sys.stdout.flush()
+
+    # Todo notes are fine, but fixme's or xxx's should probably just be
+    # fixed (in tests, at least)
+    p = subprocess.run(
+        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        cwd=directory,
+        env=env,
+        check=False,
+        universal_newlines=True,
+    )
+    ret += p.returncode
+
+    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).
+    for filename in files:
+        p = subprocess.run(
+            (
+                'python3', '-m', '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',
+                '--namespace-packages',
+                filename,
+            ),
+            cwd=directory,
+            env=env,
+            check=False,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+            universal_newlines=True
+        )
+
+        ret += p.returncode
+        if p.returncode != 0:
+            print(p.stdout)
+
+    return ret
-- 
2.31.1

[PATCH v2 15/17] iotests/linters: Add entry point for Python CI linters

[John Snow]

Add a main() function to linters.py so that the Python CI infrastructure
has something it can run.

Now, linters.py represents an invocation of the linting scripts that
more resembles a "normal" execution of pylint/mypy, like you'd expect to
use if 'qemu' was a bona-fide package you obtained from PyPI.

297, by contrast, now represents the iotests-specific configuration bits
you need to get it to function correctly as a part of iotests, and with
'qemu' as a namespace package that isn't "installed" to the current
environment, but just lives elsewhere in our source tree.

By doing this, we will able to run the same linting configuration from
the Python CI tests without calling iotest logging functions or messing
around with PYTHONPATH / MYPYPATH.

iotest 297 continues to operate in a standalone fashion for now --
presumably, it's convenient for block maintainers and contributors to
run in this manner.

See the following commit for how this is used from the Python packaging side.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/linters.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index e7e8baded02..242cb9c3e69 100755
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -119,3 +119,16 @@ def run_linters(
             print(p.stdout)
 
     return ret
+
+
+def main() -> int:
+    """
+    Used by the Python CI system as an entry point to run these linters.
+    """
+    directory = os.path.dirname(os.path.realpath(__file__))
+    files = get_test_files(directory)
+    return run_linters(files, directory)
+
+
+if __name__ == '__main__':
+    sys.exit(main())
-- 
2.31.1

[PATCH v2 16/17] python: Add iotest linters to test suite

[John Snow]

As a convenience, since iotests is an extremely prominent user of the
qemu.qmp and qemu.machine packages and already implements a linting
regime, run those tests as well so that it's very hard to miss
regressions caused by changes to the python library.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 python/tests/iotests.sh | 2 ++
 1 file changed, 2 insertions(+)
 create mode 100755 python/tests/iotests.sh

diff --git a/python/tests/iotests.sh b/python/tests/iotests.sh
new file mode 100755
index 00000000000..ec2fc58066d
--- /dev/null
+++ b/python/tests/iotests.sh
@@ -0,0 +1,2 @@
+#!/bin/sh -e
+PYTHONPATH=../tests/qemu-iotests/ python3 -m linters
-- 
2.31.1

[PATCH v2 17/17] iotests/linters: check mypy files all at once

[John Snow]

We can circumvent the '__main__' redefinition problem by passing
--scripts-are-modules. Take mypy out of the loop per-filename and check
everything in one go: it's quite a bit faster.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/linters.py | 56 ++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 242cb9c3e69..b95425e81aa 100755
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -86,37 +86,33 @@ def run_linters(
     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).
-    for filename in files:
-        p = subprocess.run(
-            (
-                'python3', '-m', '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',
-                '--namespace-packages',
-                filename,
-            ),
-            cwd=directory,
-            env=env,
-            check=False,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT,
-            universal_newlines=True
-        )
+    p = subprocess.run(
+        (
+            'python3', '-m', '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',
+            '--namespace-packages',
+            '--scripts-are-modules',
+            *files,
+        ),
+        cwd=directory,
+        env=env,
+        check=False,
+        stdout=subprocess.PIPE,
+        stderr=subprocess.STDOUT,
+        universal_newlines=True
+    )
 
-        ret += p.returncode
-        if p.returncode != 0:
-            print(p.stdout)
+    ret += p.returncode
+    if p.returncode != 0:
+        print(p.stdout)
 
     return ret
 
-- 
2.31.1

Re: [PATCH v2 01/17] iotests: use with-statement for open() calls

[Philippe Mathieu-Daudé]

On 7/20/21 7:33 PM, John Snow wrote:
> Silences a new pylint warning. The dangers of *not* doing this are
> somewhat unclear; I believe the file object gets garbage collected
> eventually, but possibly the way in which it happens is
> non-deterministic. Maybe this is a valid warning, but if there are
> consequences of not doing it, I am not aware of them at present.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 02/17] iotests: use subprocess.DEVNULL instead of open("/dev/null")

[Philippe Mathieu-Daudé]

On 7/20/21 7:33 PM, John Snow wrote:
> Avoids a warning from pylint not to use open() outside of a
> with-statement, and is ... probably more portable anyway. Not that I
> think we care too much about running tests *on* Windows, but... eh.

But we should IMO, because the community expressed some interest
(see the increasing Windows fixes over last year and CI added).

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 03/17] iotests/mirror-top-perms: Adjust import paths

[Philippe Mathieu-Daudé]

On 7/20/21 7:33 PM, John Snow wrote:
> Technically AbnormalShutdown isn't exported via
> 'qemu.machine.AbnormalShutdown', but instead from
> 'qemu.machine.machine.AbnormalShutdown'. Odd, I know, and it's on the
> list to fix. For now, change the imports here.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/qemu-iotests/tests/mirror-top-perms | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 09/17] iotests/297: Don't rely on distro-specific linter binaries

[Philippe Mathieu-Daudé]

On 7/20/21 7:33 PM, John Snow wrote:
> 'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
> mypy" to access these scripts instead. This style of invocation will
> prefer the "correct" tool when run in a virtual environment.
> 
> Note that we still check for "pylint-3" before the test begins -- this
> check is now "overly strict", but shouldn't cause anything that was
> already running correctly to start failing.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/297 | 45 ++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 19 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH v2 00/17] python/iotests: Run iotest linters during Python CI

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 4719 bytes --]

I realize that some of this has been covered before:
https://lists.gnu.org/archive/html/qemu-devel/2021-05/msg04221.html

Missed it at the time -- I suppose these patches never went in. I'll rebase
my series and front-load these patches.

--js

On Tue, Jul 20, 2021 at 1:33 PM John Snow <jsnow@redhat.com> wrote:

> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/340144191
>
> Since iotests are such a heavy and prominent user of the Python qemu.qmp
> and qemu.machine packages, it would be convenient if the Python linting
> suite also checked this client for any possible regressions introduced
> by shifting around signatures, types, or interfaces in these packages.
>
> (We'd eventually find those problems when iotest 297 ran, but with
> increasing distance between Python development and Block development,
> the risk of an accidental breakage in this regard increases. I,
> personally, know to run iotests (and especially 297) after changing
> Python code, but not everyone in the future might. Plus, I am lazy, and
> I like only having to push one button.)
>
> Add the ability for the Python CI to run the iotest linters too, which
> means that the iotest linters would be checked against:
>
> - Python 3.6, using a frozen set of linting packages at their oldest
>   supported versions, using 'pipenv'
> - Python 3.6 through Python 3.10 inclusive, using 'tox' and the latest
>   versions of mypy/pylint that happen to be installed during test
>   time. This CI test is allowed to fail with a warning, and can serve
>   as a bellwether for when new incompatible changes may disrupt the
>   linters. Testing against old and new Python interpreters alike can
>   help surface incompatibility issues we may need to be aware of.)
>
> Here are example outputs of those CI jobs with this series applied:
>  - "check-python-pipenv": https://gitlab.com/jsnow/qemu/-/jobs/1377735087
>  - "check-python-tox": https://gitlab.com/jsnow/qemu/-/jobs/1377735088
>
> You can also run these same tests locally from ./python, plus one more:
>
> - "make check-dev" to test against whatever python you have.
> - "make check-pipenv", if you have Python 3.6 and pipenv installed.
> - "make check-tox", if you have Python 3.6 through Python 3.10 installed.
>
> See the old commit message for more sample output, etc.
>
> https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg07056.html
>
> V2:
>  - Added patches 1-5 which do some more delinting.
>  - Added patch 8, which scans subdirs for tests to lint.
>  - Added patch 17, which improves the speed of mypy analysis.
>  - Patch 14 is different because of the new patch 8.
>
> Unreviewed patches:
>
> [01] iotests-use-with-statement-for # [SOB] JS
> [02] iotests-use-subprocess.devnull # [SOB] JS
> [03] iotests-mirror-top-perms       # [SOB] JS
> [04] iotests-migrate-bitmaps        # [SOB] JS
> [05] iotests-migrate-bitmaps-test   # [SOB] JS
> [07] iotests-297-add-get_files      # [SOB] JS
> [08] wip-make-the-test-finding      # [SOB] JS
> [14] iotests-297-split-linters-py   # [SOB] JS
> [17] iotests-297-check-mypy-files   # [SOB] JS
>
> --js
>
> John Snow (17):
>   iotests: use with-statement for open() calls
>   iotests: use subprocess.DEVNULL instead of open("/dev/null")
>   iotests/mirror-top-perms: Adjust import paths
>   iotests/migrate-bitmaps-postcopy-test: declare instance variables
>   iotests/migrate-bitmaps-test: delint
>   iotests/297: modify is_python_file to work from any CWD
>   iotests/297: Add get_files() function
>   iotests/297: Include sub-directories when finding tests to lint
>   iotests/297: Don't rely on distro-specific linter binaries
>   iotests/297: Create main() function
>   iotests/297: Separate environment setup from test execution
>   iotests/297: Add 'directory' argument to run_linters
>   iotests/297: return error code from run_linters()
>   iotests/297: split linters.py off from 297
>   iotests/linters: Add entry point for Python CI linters
>   python: Add iotest linters to test suite
>   iotests/linters: check mypy files all at once
>
>  python/tests/iotests.sh                       |   2 +
>  tests/qemu-iotests/297                        |  80 ++---------
>  tests/qemu-iotests/iotests.py                 |  21 +--
>  tests/qemu-iotests/linters.py                 | 130 ++++++++++++++++++
>  .../tests/migrate-bitmaps-postcopy-test       |   3 +
>  tests/qemu-iotests/tests/migrate-bitmaps-test |  70 +++++-----
>  tests/qemu-iotests/tests/mirror-top-perms     |   7 +-
>  7 files changed, 198 insertions(+), 115 deletions(-)
>  create mode 100755 python/tests/iotests.sh
>  create mode 100755 tests/qemu-iotests/linters.py
>
> --
> 2.31.1
>
>
>

[-- Attachment #2: Type: text/html, Size: 6168 bytes --]

Re: [PATCH v2 00/17] python/iotests: Run iotest linters during Python CI

[Hanna Reitz]

On 20.07.21 19:33, John Snow wrote:
> GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest
> CI: https://gitlab.com/jsnow/qemu/-/pipelines/340144191

I’ll take the liberty of applying patches 1 and 2 to my block-next 
branch, because, well, they fix some of the 297 errors I’m seeing with 
an updated pylint.

(There’s more still, namely some unspecified-encoding errors, one 
use-dict-literal, and one use-list-literal, but I don’t think there are 
patches for that in this series, so I guess I’ll have to have a go 
myself...)

To make this formal:

Thanks, I’ve applied patches 1 and 2 to my block-next branch:

https://github.com/XanClic/qemu/commits/block-next

Hanna