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

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

[John Snow]

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest-pt2
CI: https://gitlab.com/jsnow/qemu/-/pipelines/382320226
Based-on: <20210923180715.4168522-1-jsnow@redhat.com>
          [PATCH v2 0/6] iotests: update environment and linting configuration
          (Staged at kwolf/block sans patch #2, not needed here.)

Factor out pylint and mypy configuration from iotest 297 so that the
Python test infrastructure in python/ can also run these linters. This
will enable what is essentially iotest #297 to run via GitLab CI.

This series generally aims to split "linter configuration" out of code
so that both iotest #297 and the Python test suite can both invoke the
same linters (nearly) identically.

The differences occur where the Python entryway involves setting up a
virtual environment and installing linter packages pinned at specific
versions so that the CI test can be guaranteed to behave
deterministically.

iotest #297 is left as a way to run these tests as a convenience until I
can integrate environment setup and test execution as a part of 'make
check' or similar to serve as a replacement. This invocation just trusts
you've installed the right packages into the right places with the right
versions, as it always has.

V4:

- Some optimizations and touchups were included in 'PATCH v2 0/6]
  iotests: update environment and linting configuration' instead, upon
  which this series is now based.
- Rewrote most patches, being more aggressive about the factoring
  between "iotest" and "linter invocation". The patches are smaller now.
- Almost all configuration is split out into mypy.ini and pylintrc.
- Remove the PWD/CWD juggling that the previous versions added; it's not
  strictly needed for this integration. We can re-add it later if we
  wind up needing it for something.
- mypy and pylintrc tests are split into separate tests. The GitLab CI
  now lists them as two separate test cases, so it's easier to see what
  is failing and why. (And how long those tests take.) It is also now
  therefore possible to ask avocado to run just one or the other.
- mypy bug workaround is only applied strictly in cases where it is
  needed, optimizing speed of iotest 297.

V3:
 - Added patch 1 which has been submitted separately upstream,
   but was necessary for testing.
 - Rebased on top of hreitz/block, which fixed some linting issues.
 - Added a workaround for a rather nasty mypy bug ... >:(

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.

John Snow (13):
  iotests/297: Move pylint config into pylintrc
  iotests/297: Split mypy configuration out into mypy.ini
  iotests/297: Add get_files() function
  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: Split run_linters apart into run_pylint and run_mypy
  iotests/297: refactor run_[mypy|pylint] as generic execution shim
  iotests: split linters.py out from 297
  iotests/linters: Add entry point for linting via Python CI
  iotests/linters: Add workaround for mypy bug #9852
  python: Add iotest linters to test suite
  iotests: [RFC] drop iotest 297

 python/tests/iotests-mypy.sh           |  4 ++
 python/tests/iotests-pylint.sh         |  4 ++
 tests/qemu-iotests/297.out             |  2 -
 tests/qemu-iotests/{297 => linters.py} | 88 ++++++++++----------------
 tests/qemu-iotests/mypy.ini            | 12 ++++
 tests/qemu-iotests/pylintrc            | 16 +++++
 6 files changed, 71 insertions(+), 55 deletions(-)
 create mode 100755 python/tests/iotests-mypy.sh
 create mode 100755 python/tests/iotests-pylint.sh
 delete mode 100644 tests/qemu-iotests/297.out
 rename tests/qemu-iotests/{297 => linters.py} (52%)
 mode change 100755 => 100644
 create mode 100644 tests/qemu-iotests/mypy.ini

-- 
2.31.1

[PATCH 01/13] iotests/297: Move pylint config into pylintrc

[John Snow]

Move --score=n and --notes=XXX,FIXME into pylintrc. This pulls
configuration out of code, which I think is probably a good thing in
general.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297      |  4 +---
 tests/qemu-iotests/pylintrc | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 91ec34d9521..bc3a0ceb2aa 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -65,10 +65,8 @@ def run_linters():
     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()
-    subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
+    subprocess.run(('pylint-3', *files),
                    env=env, check=False)
 
     print('=== mypy ===')
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index 8cb4e1d6a6d..32ab77b8bb9 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -31,6 +31,22 @@ disable=invalid-name,
         too-many-statements,
         consider-using-f-string,
 
+
+[REPORTS]
+
+# Activate the evaluation score.
+score=no
+
+
+[MISCELLANEOUS]
+
+# List of note tags to take in consideration, separated by a comma.
+# TODO notes are fine, but FIXMEs or XXXs should probably just be
+# fixed (in tests, at least).
+notes=FIXME,
+      XXX,
+
+
 [FORMAT]
 
 # Maximum number of characters on a single line.
-- 
2.31.1

[PATCH 02/13] iotests/297: Split mypy configuration out into mypy.ini

[John Snow]

More separation of code and configuration.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297      | 14 +-------------
 tests/qemu-iotests/mypy.ini | 12 ++++++++++++
 2 files changed, 13 insertions(+), 13 deletions(-)
 create mode 100644 tests/qemu-iotests/mypy.ini

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index bc3a0ceb2aa..b8101e6024a 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -73,19 +73,7 @@ def run_linters():
     sys.stdout.flush()
 
     env['MYPYPATH'] = env['PYTHONPATH']
-    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',
-                        '--scripts-are-modules',
-                        *files),
+    p = subprocess.run(('mypy', *files),
                        env=env,
                        check=False,
                        stdout=subprocess.PIPE,
diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini
new file mode 100644
index 00000000000..4c0339f5589
--- /dev/null
+++ b/tests/qemu-iotests/mypy.ini
@@ -0,0 +1,12 @@
+[mypy]
+disallow_any_generics = True
+disallow_incomplete_defs = True
+disallow_subclassing_any = True
+disallow_untyped_decorators = True
+implicit_reexport = False
+namespace_packages = True
+no_implicit_optional = True
+scripts_are_modules = True
+warn_redundant_casts = True
+warn_unused_configs = True
+warn_unused_ignores = True
-- 
2.31.1

[PATCH 03/13] iotests/297: Add get_files() function

[John Snow]

Split out file discovery into its own method to begin separating out
configuration/setup and test execution.

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b8101e6024a..15b54594c11 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
 
@@ -54,10 +55,14 @@ def is_python_file(filename):
             return False
 
 
-def run_linters():
+def get_test_files() -> List[str]:
     named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
     check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-    files = [filename for filename in check_tests if is_python_file(filename)]
+    return list(filter(is_python_file, check_tests))
+
+
+def run_linters():
+    files = get_test_files()
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
-- 
2.31.1

[PATCH 04/13] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/297 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 15b54594c11..65b1e7058c2 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -71,14 +71,14 @@ def run_linters():
     sys.stdout.flush()
 
     env = os.environ.copy()
-    subprocess.run(('pylint-3', *files),
+    subprocess.run(('python3', '-m', 'pylint', *files),
                    env=env, check=False)
 
     print('=== mypy ===')
     sys.stdout.flush()
 
     env['MYPYPATH'] = env['PYTHONPATH']
-    p = subprocess.run(('mypy', *files),
+    p = subprocess.run((('python3', '-m', 'mypy', *files),
                        env=env,
                        check=False,
                        stdout=subprocess.PIPE,
-- 
2.31.1

[PATCH 05/13] 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>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.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 65b1e7058c2..f9fcb039e27 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -89,8 +89,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 06/13] 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>
---
 tests/qemu-iotests/297 | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index f9fcb039e27..fcbab0631be 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
 
@@ -61,23 +61,20 @@ def get_test_files() -> List[str]:
     return list(filter(is_python_file, check_tests))
 
 
-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()
 
-    env = os.environ.copy()
     subprocess.run(('python3', '-m', 'pylint', *files),
                    env=env, check=False)
 
     print('=== mypy ===')
     sys.stdout.flush()
 
-    env['MYPYPATH'] = env['PYTHONPATH']
     p = subprocess.run((('python3', '-m', 'mypy', *files),
                        env=env,
                        check=False,
@@ -94,7 +91,15 @@ 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()
+    env['MYPYPATH'] = env['PYTHONPATH']
+
+    run_linters(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH 07/13] iotests/297: Split run_linters apart into run_pylint and run_mypy

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>

---

Note, this patch really ought to be squashed with the next one, but I am
performing a move known as "Hedging my bets."
It's easier to squash than de-squash :)

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index fcbab0631be..91029dbb34e 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -61,20 +61,19 @@ def get_test_files() -> List[str]:
     return list(filter(is_python_file, check_tests))
 
 
-def run_linters(
+def run_pylint(
     files: List[str],
     env: Optional[Mapping[str, str]] = None,
 ) -> None:
 
-    print('=== pylint ===')
-    sys.stdout.flush()
-
     subprocess.run(('python3', '-m', 'pylint', *files),
                    env=env, check=False)
 
-    print('=== mypy ===')
-    sys.stdout.flush()
 
+def run_mypy(
+    files: List[str],
+    env: Optional[Mapping[str, str]] = None,
+) -> None:
     p = subprocess.run((('python3', '-m', 'mypy', *files),
                        env=env,
                        check=False,
@@ -99,7 +98,13 @@ def main() -> None:
     env = os.environ.copy()
     env['MYPYPATH'] = env['PYTHONPATH']
 
-    run_linters(files, env=env)
+    print('=== pylint ===')
+    sys.stdout.flush()
+    run_pylint(files, env=env)
+
+    print('=== mypy ===')
+    sys.stdout.flush()
+    run_mypy(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH 08/13] iotests/297: refactor run_[mypy|pylint] as generic execution shim

[John Snow]

There's virtually nothing special here anymore; we can combine these
into a single, rather generic function.

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 91029dbb34e..4c54dd39b46 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -61,29 +61,33 @@ def get_test_files() -> List[str]:
     return list(filter(is_python_file, check_tests))
 
 
-def run_pylint(
-    files: List[str],
-    env: Optional[Mapping[str, str]] = None,
-) -> None:
+def run_linter(
+        tool: str,
+        args: List[str],
+        env: Optional[Mapping[str, str]] = None,
+        suppress_output: bool = False,
+) -> int:
+    """
+    Run a python-based linting tool.
 
-    subprocess.run(('python3', '-m', 'pylint', *files),
-                   env=env, check=False)
+    If suppress_output is True, capture stdout/stderr of the child
+    process and only print that information back to stdout if the child
+    process's return code was non-zero.
+    """
+    p = subprocess.run(
+        ('python3', '-m', tool, *args),
+        env=env,
+        check=False,
+        stdout=subprocess.PIPE if suppress_output else None,
+        stderr=subprocess.STDOUT if suppress_output else None,
+        universal_newlines=True,
+    )
 
-
-def run_mypy(
-    files: List[str],
-    env: Optional[Mapping[str, str]] = None,
-) -> None:
-    p = subprocess.run((('python3', '-m', 'mypy', *files),
-                       env=env,
-                       check=False,
-                       stdout=subprocess.PIPE,
-                       stderr=subprocess.STDOUT,
-                       universal_newlines=True)
-
-    if p.returncode != 0:
+    if suppress_output and p.returncode != 0:
         print(p.stdout)
 
+    return p.returncode
+
 
 def main() -> None:
     for linter in ('pylint-3', 'mypy'):
@@ -100,11 +104,11 @@ def main() -> None:
 
     print('=== pylint ===')
     sys.stdout.flush()
-    run_pylint(files, env=env)
+    run_linter('pylint', files, env=env)
 
     print('=== mypy ===')
     sys.stdout.flush()
-    run_mypy(files, env=env)
+    run_linter('mypy', files, env=env, suppress_output=True)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH 09/13] iotests: split linters.py out from 297

[John Snow]

Now, 297 is just the iotests-specific incantations and linters.py is as
minimal as I can think to make it. The only remaining element in here
that ought to be configuration and not code is the list of skip files,
but they're still numerous enough that repeating them for mypy and
pylint configurations both would be ... a hassle.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297        | 72 +++---------------------------
 tests/qemu-iotests/linters.py | 83 +++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 67 deletions(-)
 create mode 100644 tests/qemu-iotests/linters.py

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 4c54dd39b46..f79c80216bf 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,76 +17,14 @@
 # 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', '194', '196', '202',
-    '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-    '218', '219', '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(filename):
-    if not os.path.isfile(filename):
-        return False
-
-    if filename.endswith('.py'):
-        return True
-
-    with open(filename, encoding='utf-8') 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() -> List[str]:
-    named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
-    check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
-    return list(filter(is_python_file, check_tests))
-
-
-def run_linter(
-        tool: str,
-        args: List[str],
-        env: Optional[Mapping[str, str]] = None,
-        suppress_output: bool = False,
-) -> int:
-    """
-    Run a python-based linting tool.
-
-    If suppress_output is True, capture stdout/stderr of the child
-    process and only print that information back to stdout if the child
-    process's return code was non-zero.
-    """
-    p = subprocess.run(
-        ('python3', '-m', tool, *args),
-        env=env,
-        check=False,
-        stdout=subprocess.PIPE if suppress_output else None,
-        stderr=subprocess.STDOUT if suppress_output else None,
-        universal_newlines=True,
-    )
-
-    if suppress_output and p.returncode != 0:
-        print(p.stdout)
-
-    return p.returncode
+# Looking for the list of files to exclude from linting? See linters.py.
 
 
 def main() -> None:
@@ -94,7 +32,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)))
@@ -104,11 +42,11 @@ def main() -> None:
 
     print('=== pylint ===')
     sys.stdout.flush()
-    run_linter('pylint', files, env=env)
+    linters.run_linter('pylint', files, env=env)
 
     print('=== mypy ===')
     sys.stdout.flush()
-    run_linter('mypy', files, env=env, suppress_output=True)
+    linters.run_linter('mypy', files, env=env, suppress_output=True)
 
 
 iotests.script_main(main)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100644
index 00000000000..f6a2dc139fd
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,83 @@
+# 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
+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', '194', '196', '202',
+    '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
+    '218', '219', '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(filename):
+    if not os.path.isfile(filename):
+        return False
+
+    if filename.endswith('.py'):
+        return True
+
+    with open(filename, encoding='utf-8') 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() -> List[str]:
+    named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
+    check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
+    return list(filter(is_python_file, check_tests))
+
+
+def run_linter(
+        tool: str,
+        args: List[str],
+        env: Optional[Mapping[str, str]] = None,
+        suppress_output: bool = False,
+) -> int:
+    """
+    Run a python-based linting tool.
+
+    If suppress_output is True, capture stdout/stderr of the child
+    process and only print that information back to stdout if the child
+    process's return code was non-zero.
+    """
+    p = subprocess.run(
+        ('python3', '-m', tool, *args),
+        env=env,
+        check=False,
+        stdout=subprocess.PIPE if suppress_output else None,
+        stderr=subprocess.STDOUT if suppress_output else None,
+        universal_newlines=True,
+    )
+
+    if suppress_output and p.returncode != 0:
+        print(p.stdout)
+
+    return p.returncode
+
-- 
2.31.1

[PATCH 10/13] iotests/linters: Add entry point for linting via Python CI

[John Snow]

We need at least a tiny little shim here to join test file discovery
with test invocation. This logic could conceivably be hosted somewhere
in python/, but I felt it was strictly the least-rude thing to keep the
test logic here in iotests/, even if this small function isn't itself an
iotest.

Note that we don't actually even need the executable bit here, we'll be
relying on the ability to run this module as a script using Python CLI
arguments. No chance it gets misunderstood as an actual iotest that way.

(It's named, not in tests/, doesn't have the execute bit, and doesn't
have an execution shebang.)

Signed-off-by: John Snow <jsnow@redhat.com>

---

(1) I think that the test file discovery logic and skip list belong together,
    and that those items belong in iotests/. I think they also belong in
    whichever directory pylintrc and mypy.ini are in, also in iotests/.

(2) Moving this logic into python/tests/ is challenging because I'd have
    to import iotests code from elsewhere in the source tree, which just
    inverts an existing problem I have been trying to rid us of --
    needing to muck around with PYTHONPATH or sys.path hacking in python
    scripts. I'm keen to avoid this.

(3) If we moved all python tests into tests/ and gave them *.py
    extensions, we wouldn't even need the test discovery functions
    anymore, and all of linters.py could be removed entirely, including
    this execution shim. We could rely on mypy/pylint's own file
    discovery mechanisms at that point. More work than I'm up for with
    just this series, but I could be coaxed into doing it if there was
    some promise of not rejecting all that busywork ;)

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

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index f6a2dc139fd..191df173064 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -16,6 +16,7 @@
 import os
 import re
 import subprocess
+import sys
 from typing import List, Mapping, Optional
 
 
@@ -81,3 +82,20 @@ def run_linter(
 
     return p.returncode
 
+
+def main() -> int:
+    """
+    Used by the Python CI system as an entry point to run these linters.
+    """
+    files = get_test_files()
+
+    if sys.argv[1] == '--pylint':
+        return run_linter('pylint', files)
+    elif sys.argv[1] == '--mypy':
+        return run_linter('mypy', files)
+
+    raise ValueError(f"Unrecognized argument(s): '{sys.argv[1:]}'")
+
+
+if __name__ == '__main__':
+    sys.exit(main())
-- 
2.31.1

[PATCH 11/13] iotests/linters: Add workaround for mypy bug #9852

[John Snow]

This one is insidious: if you write an import as "from {namespace}
import {subpackage}" as mirror-top-perms (now) does, mypy will fail on
every-other invocation *if* the package being imported is a typed,
installed, namespace-scoped package.

Upsettingly, that's exactly what 'qemu.[aqmp|qmp|machine]' et al are in
the context of Python CI tests.

Now, I could just edit mirror-top-perms to avoid this invocation, but
since I tripped on a landmine, I might as well head it off at the pass
and make sure nobody else trips on that same landmine.

It seems to have something to do with the order in which files are
checked as well, meaning the random order in which set(os.listdir())
produces the list of files to test will cause problems intermittently
and not just strictly "every other run".

This will be fixed in mypy >= 0.920, which is not released yet. The
workaround for now is to disable incremental checking, which avoids the
issue.

Note: This workaround is not applied when running iotest 297 directly,
because the bug does not surface there! Given the nature of CI jobs not
starting with any stale cache to begin with, this really only has a
half-second impact on manual runs of the Python test suite when executed
directly by a developer on their local machine. The workaround may be
removed when the Python package requirements can stipulate mypy 0.920 or
higher, which can happen as soon as it is released. (Barring any
unforseen compatibility issues that 0.920 may bring with it.)


See also:
 https://github.com/python/mypy/issues/11010
 https://github.com/python/mypy/issues/9852

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

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 191df173064..83fcc5a960c 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -92,7 +92,9 @@ def main() -> int:
     if sys.argv[1] == '--pylint':
         return run_linter('pylint', files)
     elif sys.argv[1] == '--mypy':
-        return run_linter('mypy', files)
+        # mypy bug #9852; disable incremental checking as a workaround.
+        args = ['--no-incremental'] + files
+        return run_linter('mypy', args)
 
     raise ValueError(f"Unrecognized argument(s): '{sys.argv[1:]}'")
 
-- 
2.31.1

[PATCH 12/13] python: Add iotest linters to test suite

[John Snow]

Run mypy and pylint on the iotests files directly from the Python CI
test infrastructure. This ensures that any accidental breakages to the
qemu.[qmp|aqmp|machine|utils] packages will be caught by that test
suite.

It also ensures that these linters are run with well-known versions and
test against a wide variety of python versions, which helps to find
accidental cross-version python compatibility issues.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/tests/iotests-mypy.sh   | 4 ++++
 python/tests/iotests-pylint.sh | 4 ++++
 2 files changed, 8 insertions(+)
 create mode 100755 python/tests/iotests-mypy.sh
 create mode 100755 python/tests/iotests-pylint.sh

diff --git a/python/tests/iotests-mypy.sh b/python/tests/iotests-mypy.sh
new file mode 100755
index 00000000000..ee764708199
--- /dev/null
+++ b/python/tests/iotests-mypy.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+
+cd ../tests/qemu-iotests/
+python3 -m linters --mypy
diff --git a/python/tests/iotests-pylint.sh b/python/tests/iotests-pylint.sh
new file mode 100755
index 00000000000..4cae03424b4
--- /dev/null
+++ b/python/tests/iotests-pylint.sh
@@ -0,0 +1,4 @@
+#!/bin/sh -e
+
+cd ../tests/qemu-iotests/
+python3 -m linters --pylint
-- 
2.31.1

[PATCH 13/13] iotests: [RFC] drop iotest 297

[John Snow]

(This is highlighting a what-if, which might make it clear why any
special infrastructure is still required at all. It's not intended to
actually be merged at this step -- running JUST the iotest linters from
e.g. 'make check' is not yet accommodated, so there's no suitable
replacement for 297 for block test authors.)

Drop 297. As a consequence, we no longer need to pass an environment
variable to the mypy/pylint invocations, so that can be dropped. We also
now no longer need to hide output-except-on-error, so that can be
dropped as well.

The only thing that necessitates any special running logic anymore is
the skip list and the python-test-detection code. Without those, we
could easily codify the tests as simply:

[pylint|mypy] *.py tests/*.py

... and drop this entire file. We're not quite there yet, though.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/297        | 52 -----------------------------------
 tests/qemu-iotests/297.out    |  2 --
 tests/qemu-iotests/linters.py | 20 ++------------
 3 files changed, 2 insertions(+), 72 deletions(-)
 delete mode 100755 tests/qemu-iotests/297
 delete mode 100644 tests/qemu-iotests/297.out

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
deleted file mode 100755
index f79c80216bf..00000000000
--- a/tests/qemu-iotests/297
+++ /dev/null
@@ -1,52 +0,0 @@
-#!/usr/bin/env python3
-# group: meta
-#
-# 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 shutil
-import sys
-
-import iotests
-import linters
-
-
-# Looking for the list of files to exclude from linting? See linters.py.
-
-
-def main() -> None:
-    for linter in ('pylint-3', 'mypy'):
-        if shutil.which(linter) is None:
-            iotests.notrun(f'{linter} not found')
-
-    files = linters.get_test_files()
-
-    iotests.logger.debug('Files to be checked:')
-    iotests.logger.debug(', '.join(sorted(files)))
-
-    env = os.environ.copy()
-    env['MYPYPATH'] = env['PYTHONPATH']
-
-    print('=== pylint ===')
-    sys.stdout.flush()
-    linters.run_linter('pylint', files, env=env)
-
-    print('=== mypy ===')
-    sys.stdout.flush()
-    linters.run_linter('mypy', files, env=env, suppress_output=True)
-
-
-iotests.script_main(main)
diff --git a/tests/qemu-iotests/297.out b/tests/qemu-iotests/297.out
deleted file mode 100644
index f2e1314d104..00000000000
--- a/tests/qemu-iotests/297.out
+++ /dev/null
@@ -1,2 +0,0 @@
-=== pylint ===
-=== mypy ===
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index 83fcc5a960c..ca90604d8d9 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -17,7 +17,7 @@
 import re
 import subprocess
 import sys
-from typing import List, Mapping, Optional
+from typing import List
 
 
 # TODO: Empty this list!
@@ -55,31 +55,15 @@ def get_test_files() -> List[str]:
     return list(filter(is_python_file, check_tests))
 
 
-def run_linter(
-        tool: str,
-        args: List[str],
-        env: Optional[Mapping[str, str]] = None,
-        suppress_output: bool = False,
-) -> int:
+def run_linter(tool: str, args: List[str]) -> int:
     """
     Run a python-based linting tool.
-
-    If suppress_output is True, capture stdout/stderr of the child
-    process and only print that information back to stdout if the child
-    process's return code was non-zero.
     """
     p = subprocess.run(
         ('python3', '-m', tool, *args),
-        env=env,
         check=False,
-        stdout=subprocess.PIPE if suppress_output else None,
-        stderr=subprocess.STDOUT if suppress_output else None,
-        universal_newlines=True,
     )
 
-    if suppress_output and p.returncode != 0:
-        print(p.stdout)
-
     return p.returncode
 
 
-- 
2.31.1

Re: [PATCH 01/13] iotests/297: Move pylint config into pylintrc

[Hanna Reitz]

On 04.10.21 23:04, John Snow wrote:
> Move --score=n and --notes=XXX,FIXME into pylintrc. This pulls
> configuration out of code, which I think is probably a good thing in
> general.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297      |  4 +---
>   tests/qemu-iotests/pylintrc | 16 ++++++++++++++++
>   2 files changed, 17 insertions(+), 3 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 02/13] iotests/297: Split mypy configuration out into mypy.ini

[Hanna Reitz]

On 04.10.21 23:04, John Snow wrote:
> More separation of code and configuration.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297      | 14 +-------------
>   tests/qemu-iotests/mypy.ini | 12 ++++++++++++
>   2 files changed, 13 insertions(+), 13 deletions(-)
>   create mode 100644 tests/qemu-iotests/mypy.ini

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index bc3a0ceb2aa..b8101e6024a 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -73,19 +73,7 @@ def run_linters():
>       sys.stdout.flush()
>   
>       env['MYPYPATH'] = env['PYTHONPATH']
> -    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',
> -                        '--scripts-are-modules',
> -                        *files),
> +    p = subprocess.run(('mypy', *files),
>                          env=env,
>                          check=False,
>                          stdout=subprocess.PIPE,
> diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini
> new file mode 100644
> index 00000000000..4c0339f5589
> --- /dev/null
> +++ b/tests/qemu-iotests/mypy.ini
> @@ -0,0 +1,12 @@
> +[mypy]
> +disallow_any_generics = True
> +disallow_incomplete_defs = True
> +disallow_subclassing_any = True
> +disallow_untyped_decorators = True
> +implicit_reexport = False

Out of curiosity: Any reason you chose to invert this one, but none of 
the rest?  (i.e. no_implicit_optional = True -> implicit_optional = 
False; or disallow* = True -> allow* = False)

Hanna

> +namespace_packages = True
> +no_implicit_optional = True
> +scripts_are_modules = True
> +warn_redundant_casts = True
> +warn_unused_configs = True
> +warn_unused_ignores = True

Re: [PATCH 03/13] iotests/297: Add get_files() function

[Hanna Reitz]

On 04.10.21 23:04, John Snow wrote:
> Split out file discovery into its own method to begin separating out
> configuration/setup and test execution.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 05/13] iotests/297: Create main() function

[Hanna Reitz]

On 04.10.21 23:04, John Snow wrote:
> 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>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reviewed-by: Hanna Reitz <hreitz@redhat.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 65b1e7058c2..f9fcb039e27 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -89,8 +89,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')

Now that I see it here: Given patch 4, shouldn’t we replace 
`shutil.which()` by some other check?

Hanna

>   
> -iotests.script_main(run_linters)
> +    run_linters()
> +
> +
> +iotests.script_main(main)

Re: [PATCH 06/13] iotests/297: Separate environment setup from test execution

[Hanna Reitz]

On 04.10.21 23:04, John Snow wrote:
> Move environment setup into main(), leaving pure test execution behind
> in run_linters().
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 07/13] iotests/297: Split run_linters apart into run_pylint and run_mypy

[Hanna Reitz]

On 04.10.21 23:04, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> Note, this patch really ought to be squashed with the next one,

Yes, it should be.

> but I am
> performing a move known as "Hedging my bets."
> It's easier to squash than de-squash :)

True.  Still, should be squashed. ;)

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

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 08/13] iotests/297: refactor run_[mypy|pylint] as generic execution shim

[Hanna Reitz]

On 04.10.21 23:04, John Snow wrote:
> There's virtually nothing special here anymore; we can combine these
> into a single, rather generic function.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 46 +++++++++++++++++++++++-------------------
>   1 file changed, 25 insertions(+), 21 deletions(-)

Acked-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 09/13] iotests: split linters.py out from 297

[Hanna Reitz]

On 04.10.21 23:04, John Snow wrote:
> Now, 297 is just the iotests-specific incantations and linters.py is as
> minimal as I can think to make it. The only remaining element in here
> that ought to be configuration and not code is the list of skip files,

Yeah...

> but they're still numerous enough that repeating them for mypy and
> pylint configurations both would be ... a hassle.

I agree.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297        | 72 +++---------------------------
>   tests/qemu-iotests/linters.py | 83 +++++++++++++++++++++++++++++++++++
>   2 files changed, 88 insertions(+), 67 deletions(-)
>   create mode 100644 tests/qemu-iotests/linters.py

I’d like to give an A-b because now the statuscode-returning function is 
in a library.  But I already gave an A-b on the last patch precisely 
because of the interface, and I shouldn’t be so grumpy.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 10/13] iotests/linters: Add entry point for linting via Python CI

[Hanna Reitz]

On 04.10.21 23:05, John Snow wrote:
> We need at least a tiny little shim here to join test file discovery
> with test invocation. This logic could conceivably be hosted somewhere
> in python/, but I felt it was strictly the least-rude thing to keep the
> test logic here in iotests/, even if this small function isn't itself an
> iotest.
>
> Note that we don't actually even need the executable bit here, we'll be
> relying on the ability to run this module as a script using Python CLI
> arguments. No chance it gets misunderstood as an actual iotest that way.
>
> (It's named, not in tests/, doesn't have the execute bit, and doesn't
> have an execution shebang.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> (1) I think that the test file discovery logic and skip list belong together,
>      and that those items belong in iotests/. I think they also belong in
>      whichever directory pylintrc and mypy.ini are in, also in iotests/.

Agreed.

> (2) Moving this logic into python/tests/ is challenging because I'd have
>      to import iotests code from elsewhere in the source tree, which just
>      inverts an existing problem I have been trying to rid us of --
>      needing to muck around with PYTHONPATH or sys.path hacking in python
>      scripts. I'm keen to avoid this.

OK.

> (3) If we moved all python tests into tests/ and gave them *.py
>      extensions, we wouldn't even need the test discovery functions
>      anymore, and all of linters.py could be removed entirely, including
>      this execution shim. We could rely on mypy/pylint's own file
>      discovery mechanisms at that point. More work than I'm up for with
>      just this series, but I could be coaxed into doing it if there was
>      some promise of not rejecting all that busywork ;)

I believe the only real value this would gain is that the tests would 
have nicer names and we would have to delint them.  If we find those 
goals to justify the effort, then we can put in the effort; and we can 
do that slowly, test by test.  I don’t think we must do it now in one 
big lump just to get rid of the file discovery functions.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/linters.py | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
> index f6a2dc139fd..191df173064 100644
> --- a/tests/qemu-iotests/linters.py
> +++ b/tests/qemu-iotests/linters.py
> @@ -16,6 +16,7 @@
>   import os
>   import re
>   import subprocess
> +import sys
>   from typing import List, Mapping, Optional
>   
>   
> @@ -81,3 +82,20 @@ def run_linter(
>   
>       return p.returncode
>   
> +
> +def main() -> int:
> +    """
> +    Used by the Python CI system as an entry point to run these linters.
> +    """
> +    files = get_test_files()
> +
> +    if sys.argv[1] == '--pylint':
> +        return run_linter('pylint', files)
> +    elif sys.argv[1] == '--mypy':
> +        return run_linter('mypy', files)

So I can run it as `python linters.py --pylint foo bar` and it won’t 
complain? :)

I don’t feel like it’s important, but, well, it isn’t right either.

Hanna

> +
> +    raise ValueError(f"Unrecognized argument(s): '{sys.argv[1:]}'")
> +
> +
> +if __name__ == '__main__':
> +    sys.exit(main())

Re: [PATCH 11/13] iotests/linters: Add workaround for mypy bug #9852

[Hanna Reitz]

On 04.10.21 23:05, John Snow wrote:
> This one is insidious: if you write an import as "from {namespace}
> import {subpackage}" as mirror-top-perms (now) does, mypy will fail on
> every-other invocation *if* the package being imported is a typed,
> installed, namespace-scoped package.
>
> Upsettingly, that's exactly what 'qemu.[aqmp|qmp|machine]' et al are in
> the context of Python CI tests.
>
> Now, I could just edit mirror-top-perms to avoid this invocation, but
> since I tripped on a landmine, I might as well head it off at the pass
> and make sure nobody else trips on that same landmine.
>
> It seems to have something to do with the order in which files are
> checked as well, meaning the random order in which set(os.listdir())
> produces the list of files to test will cause problems intermittently
> and not just strictly "every other run".
>
> This will be fixed in mypy >= 0.920, which is not released yet. The
> workaround for now is to disable incremental checking, which avoids the
> issue.
>
> Note: This workaround is not applied when running iotest 297 directly,
> because the bug does not surface there! Given the nature of CI jobs not
> starting with any stale cache to begin with, this really only has a
> half-second impact on manual runs of the Python test suite when executed
> directly by a developer on their local machine. The workaround may be
> removed when the Python package requirements can stipulate mypy 0.920 or
> higher, which can happen as soon as it is released. (Barring any
> unforseen compatibility issues that 0.920 may bring with it.)
>
>
> See also:
>   https://github.com/python/mypy/issues/11010
>   https://github.com/python/mypy/issues/9852
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/linters.py | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 12/13] python: Add iotest linters to test suite

[Hanna Reitz]

On 04.10.21 23:05, John Snow wrote:
> Run mypy and pylint on the iotests files directly from the Python CI
> test infrastructure. This ensures that any accidental breakages to the
> qemu.[qmp|aqmp|machine|utils] packages will be caught by that test
> suite.
>
> It also ensures that these linters are run with well-known versions and
> test against a wide variety of python versions, which helps to find
> accidental cross-version python compatibility issues.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/tests/iotests-mypy.sh   | 4 ++++
>   python/tests/iotests-pylint.sh | 4 ++++
>   2 files changed, 8 insertions(+)
>   create mode 100755 python/tests/iotests-mypy.sh
>   create mode 100755 python/tests/iotests-pylint.sh

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 13/13] iotests: [RFC] drop iotest 297

[Hanna Reitz]

On 04.10.21 23:05, John Snow wrote:
> (This is highlighting a what-if, which might make it clear why any
> special infrastructure is still required at all. It's not intended to
> actually be merged at this step -- running JUST the iotest linters from
> e.g. 'make check' is not yet accommodated, so there's no suitable
> replacement for 297 for block test authors.)

OK, acknowledged. :)

Hanna

> Drop 297. As a consequence, we no longer need to pass an environment
> variable to the mypy/pylint invocations, so that can be dropped. We also
> now no longer need to hide output-except-on-error, so that can be
> dropped as well.
>
> The only thing that necessitates any special running logic anymore is
> the skip list and the python-test-detection code. Without those, we
> could easily codify the tests as simply:
>
> [pylint|mypy] *.py tests/*.py
>
> ... and drop this entire file. We're not quite there yet, though.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297        | 52 -----------------------------------
>   tests/qemu-iotests/297.out    |  2 --
>   tests/qemu-iotests/linters.py | 20 ++------------
>   3 files changed, 2 insertions(+), 72 deletions(-)
>   delete mode 100755 tests/qemu-iotests/297
>   delete mode 100644 tests/qemu-iotests/297.out

Re: [PATCH 02/13] iotests/297: Split mypy configuration out into mypy.ini

[John Snow]

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

On Wed, Oct 13, 2021 at 6:53 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 04.10.21 23:04, John Snow wrote:
> > More separation of code and configuration.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297      | 14 +-------------
> >   tests/qemu-iotests/mypy.ini | 12 ++++++++++++
> >   2 files changed, 13 insertions(+), 13 deletions(-)
> >   create mode 100644 tests/qemu-iotests/mypy.ini
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index bc3a0ceb2aa..b8101e6024a 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -73,19 +73,7 @@ def run_linters():
> >       sys.stdout.flush()
> >
> >       env['MYPYPATH'] = env['PYTHONPATH']
> > -    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',
> > -                        '--scripts-are-modules',
> > -                        *files),
> > +    p = subprocess.run(('mypy', *files),
> >                          env=env,
> >                          check=False,
> >                          stdout=subprocess.PIPE,
> > diff --git a/tests/qemu-iotests/mypy.ini b/tests/qemu-iotests/mypy.ini
> > new file mode 100644
> > index 00000000000..4c0339f5589
> > --- /dev/null
> > +++ b/tests/qemu-iotests/mypy.ini
> > @@ -0,0 +1,12 @@
> > +[mypy]
> > +disallow_any_generics = True
> > +disallow_incomplete_defs = True
> > +disallow_subclassing_any = True
> > +disallow_untyped_decorators = True
> > +implicit_reexport = False
>
> Out of curiosity: Any reason you chose to invert this one, but none of
> the rest?  (i.e. no_implicit_optional = True -> implicit_optional =
> False; or disallow* = True -> allow* = False)
>
>
Anything written as '--no-xxx' I wrote as 'xxx = False', but
"implicit_optional = False" isn't a supported option, so that one kept the
"no" in it.

--js

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

Re: [PATCH 05/13] iotests/297: Create main() function

[John Snow]

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

On Wed, Oct 13, 2021 at 7:03 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 04.10.21 23:04, John Snow wrote:
> > 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>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Reviewed-by: Hanna Reitz <hreitz@redhat.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 65b1e7058c2..f9fcb039e27 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -89,8 +89,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')
>
> Now that I see it here: Given patch 4, shouldn’t we replace
> `shutil.which()` by some other check?
>
>
Yeah, ideally. Sorry, I was lazy and didn't do that part yet. Nobody had
asked O:-)

I'll bolster the previous patch for the next go-around. (Or maybe I'll send
a fixup patch to the list, depending on what the rest of your replies look
like.)

--js

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

Re: [PATCH 09/13] iotests: split linters.py out from 297

[John Snow]

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

On Wed, Oct 13, 2021 at 7:50 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 04.10.21 23:04, John Snow wrote:
> > Now, 297 is just the iotests-specific incantations and linters.py is as
> > minimal as I can think to make it. The only remaining element in here
> > that ought to be configuration and not code is the list of skip files,
>
> Yeah...
>
> > but they're still numerous enough that repeating them for mypy and
> > pylint configurations both would be ... a hassle.
>
> I agree.
>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297        | 72 +++---------------------------
> >   tests/qemu-iotests/linters.py | 83 +++++++++++++++++++++++++++++++++++
> >   2 files changed, 88 insertions(+), 67 deletions(-)
> >   create mode 100644 tests/qemu-iotests/linters.py
>
> I’d like to give an A-b because now the statuscode-returning function is
> in a library.  But I already gave an A-b on the last patch precisely
> because of the interface, and I shouldn’t be so grumpy.
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
I'm not entirely sure I understand your dislike(?) of status codes. I'm not
trying to ignore the feedback, but I don't think I understand it fully.

Would it be better if I removed check=False and allowed the functions to
raise an Exception on non-zero subprocess return? (Possibly having the
function itself print the stdout on the error case before re-raising.)

--js

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

Re: [PATCH 10/13] iotests/linters: Add entry point for linting via Python CI

[John Snow]

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

On Wed, Oct 13, 2021 at 8:11 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 04.10.21 23:05, John Snow wrote:
> > We need at least a tiny little shim here to join test file discovery
> > with test invocation. This logic could conceivably be hosted somewhere
> > in python/, but I felt it was strictly the least-rude thing to keep the
> > test logic here in iotests/, even if this small function isn't itself an
> > iotest.
> >
> > Note that we don't actually even need the executable bit here, we'll be
> > relying on the ability to run this module as a script using Python CLI
> > arguments. No chance it gets misunderstood as an actual iotest that way.
> >
> > (It's named, not in tests/, doesn't have the execute bit, and doesn't
> > have an execution shebang.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > (1) I think that the test file discovery logic and skip list belong
> together,
> >      and that those items belong in iotests/. I think they also belong in
> >      whichever directory pylintrc and mypy.ini are in, also in iotests/.
>
> Agreed.
>
> > (2) Moving this logic into python/tests/ is challenging because I'd have
> >      to import iotests code from elsewhere in the source tree, which just
> >      inverts an existing problem I have been trying to rid us of --
> >      needing to muck around with PYTHONPATH or sys.path hacking in python
> >      scripts. I'm keen to avoid this.
>
> OK.
>
> > (3) If we moved all python tests into tests/ and gave them *.py
> >      extensions, we wouldn't even need the test discovery functions
> >      anymore, and all of linters.py could be removed entirely, including
> >      this execution shim. We could rely on mypy/pylint's own file
> >      discovery mechanisms at that point. More work than I'm up for with
> >      just this series, but I could be coaxed into doing it if there was
> >      some promise of not rejecting all that busywork ;)
>
> I believe the only real value this would gain is that the tests would
> have nicer names and we would have to delint them.  If we find those
> goals to justify the effort, then we can put in the effort; and we can
> do that slowly, test by test.  I don’t think we must do it now in one
> big lump just to get rid of the file discovery functions.
>
>
Yeah, I agree -- just do it over time and as-needed. I'm sure I will be
bothered by something-or-other sooner-or-later and I'll wind up doing it
anyway. Just maybe not this week!


> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/linters.py | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/linters.py
> b/tests/qemu-iotests/linters.py
> > index f6a2dc139fd..191df173064 100644
> > --- a/tests/qemu-iotests/linters.py
> > +++ b/tests/qemu-iotests/linters.py
> > @@ -16,6 +16,7 @@
> >   import os
> >   import re
> >   import subprocess
> > +import sys
> >   from typing import List, Mapping, Optional
> >
> >
> > @@ -81,3 +82,20 @@ def run_linter(
> >
> >       return p.returncode
> >
> > +
> > +def main() -> int:
> > +    """
> > +    Used by the Python CI system as an entry point to run these linters.
> > +    """
> > +    files = get_test_files()
> > +
> > +    if sys.argv[1] == '--pylint':
> > +        return run_linter('pylint', files)
> > +    elif sys.argv[1] == '--mypy':
> > +        return run_linter('mypy', files)
>
> So I can run it as `python linters.py --pylint foo bar` and it won’t
> complain? :)
>
> I don’t feel like it’s important, but, well, it isn’t right either.
>
>
Alright. I hacked it together to be "minimal" in terms of SLOC, but I can
make it ... less minimal.

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

Re: [PATCH 09/13] iotests: split linters.py out from 297

[Hanna Reitz]

On 13.10.21 17:07, John Snow wrote:
>
>
> On Wed, Oct 13, 2021 at 7:50 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 04.10.21 23:04, John Snow wrote:
>     > Now, 297 is just the iotests-specific incantations and
>     linters.py is as
>     > minimal as I can think to make it. The only remaining element in
>     here
>     > that ought to be configuration and not code is the list of skip
>     files,
>
>     Yeah...
>
>     > but they're still numerous enough that repeating them for mypy and
>     > pylint configurations both would be ... a hassle.
>
>     I agree.
>
>     > Signed-off-by: John Snow <jsnow@redhat.com>
>     > ---
>     >   tests/qemu-iotests/297        | 72 +++---------------------------
>     >   tests/qemu-iotests/linters.py | 83
>     +++++++++++++++++++++++++++++++++++
>     >   2 files changed, 88 insertions(+), 67 deletions(-)
>     >   create mode 100644 tests/qemu-iotests/linters.py
>
>     I’d like to give an A-b because now the statuscode-returning
>     function is
>     in a library.  But I already gave an A-b on the last patch precisely
>     because of the interface, and I shouldn’t be so grumpy.
>
>     Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
> I'm not entirely sure I understand your dislike(?) of status codes. 
> I'm not trying to ignore the feedback, but I don't think I understand 
> it fully.

It’s the fact that we only use status codes because they are part of the 
interface of shell commands.  A python function isn’t a shell command, 
so I find it weird to use that interface there.  Returning True/False 
would make more sense, for example.

I understand we have the same thing with qemu* commands in iotests.py, 
so I shouldn’t be so stuck on it...

> Would it be better if I removed check=False and allowed the functions 
> to raise an Exception on non-zero subprocess return? (Possibly having 
> the function itself print the stdout on the error case before re-raising.)

Yes, I would like that better! :)

Hanna