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

[PATCH v2 00/15] 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/388626603

(There's no real rush on my part for this particular series, so review
at-leisure, I'm just getting my edits back out onto the list. The AQMP
series is more important to me in the short-term. I suppose this would
be good to have prior to the RC testing phase, though.)

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, via
the 'check-python-pipenv' and 'check-python-tox' tests with new
'iotests-pylint' and 'iotests-mypy' cases.

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.

iotest #297 is left as a valid way to run these tests as a convenience
for block layer developers until I can integrate environment setup and
test execution as a part of 'make check' or similar to serve as a
replacement. This invocation (at present) just trusts you've installed
the right packages into the right places with the right versions, as it
always has. Future patches may begin to build the Python testing
environment as part of 'make check', but there are some details to
hammer out there, first.

v5:

Broadly:

 - Remove int return from linting helpers
 - Tighten tool availability checks
 - Explicitly ensured no regressions on 297 mid-series

In detail:

001/15:[----] [--] 'iotests/297: Move pylint config into pylintrc'
002/15:[----] [--] 'iotests/297: Split mypy configuration out into mypy.ini'
003/15:[----] [--] 'iotests/297: Add get_files() function'
004/15:[----] [--] 'iotests/297: Create main() function'
005/15:[0002] [FC] 'iotests/297: Don't rely on distro-specific linter binaries'
006/15:[0023] [FC] 'iotests/297: Split run_linters apart into run_pylint and run_mypy'
007/15:[0006] [FC] 'iotests/297: refactor run_[mypy|pylint] as generic execution shim'
008/15:[down] 'iotests/297: Change run_linter() to raise an exception on failure'
009/15:[down] 'iotests/297: update tool availability checks'
010/15:[down] 'iotests/297: split test into sub-cases'
011/15:[0051] [FC] 'iotests: split linters.py out from 297'
012/15:[0021] [FC] 'iotests/linters: Add entry point for linting via Python CI'
013/15:[0004] [FC] 'iotests/linters: Add workaround for mypy bug #9852'
014/15:[----] [--] 'python: Add iotest linters to test suite'
015/15:[0072] [FC] 'iotests: [RFC] drop iotest 297'

- 005: Fixed extra parenthesis. (Oops.)
- 006: Squashed with intermediate patch from v1.
- 007: Removed 'int' return from run_linter()
- 008-010: New.
- 011: Modified comment, otherwise just rebased.
- 012: Rewrote CLI handling to be more thorough.
- 013: Rebase changes only.
- 015: Rebase changes, not that it matters!

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 (15):
  iotests/297: Move pylint config into pylintrc
  iotests/297: Split mypy configuration out into mypy.ini
  iotests/297: Add get_files() function
  iotests/297: Create main() function
  iotests/297: Don't rely on distro-specific linter binaries
  iotests/297: Split run_linters apart into run_pylint and run_mypy
  iotests/297: refactor run_[mypy|pylint] as generic execution shim
  iotests/297: Change run_linter() to raise an exception on failure
  iotests/297: update tool availability checks
  iotests/297: split test into sub-cases
  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} | 96 ++++++++++++--------------
 tests/qemu-iotests/mypy.ini            | 12 ++++
 tests/qemu-iotests/pylintrc            | 16 +++++
 6 files changed, 79 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 v2 01/15] 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>
Reviewed-by: Hanna Reitz <hreitz@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 v2 02/15] 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>
Reviewed-by: Hanna Reitz <hreitz@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 v2 03/15] 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>
Reviewed-by: Hanna Reitz <hreitz@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 v2 04/15] 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 15b54594c11..163ebc8ebfd 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 v2 05/15] 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. This is addressed by a
commit later in this series;
  'iotests/297: update tool availability checks'.

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 163ebc8ebfd..c1bddb9ce0e 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 v2 06/15] iotests/297: Split run_linters apart into run_pylint and run_mypy

[John Snow]

Move environment setup into main(), and split the actual linter
execution into run_pylint and run_mypy, respectively.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Hanna Reitz <hreitz@redhat.com>
---
 tests/qemu-iotests/297 | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index c1bddb9ce0e..189bcaf5f94 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,19 @@ def get_test_files() -> List[str]:
     return list(filter(is_python_file, check_tests))
 
 
-def run_linters():
-    files = get_test_files()
+def run_pylint(
+    files: List[str],
+    env: Optional[Mapping[str, str]] = None,
+) -> None:
 
-    iotests.logger.debug('Files to be checked:')
-    iotests.logger.debug(', '.join(sorted(files)))
-
-    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']
+def run_mypy(
+    files: List[str],
+    env: Optional[Mapping[str, str]] = None,
+) -> None:
     p = subprocess.run(('python3', '-m', 'mypy', *files),
                        env=env,
                        check=False,
@@ -94,7 +90,21 @@ 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']
+
+    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 v2 07/15] 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 | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 189bcaf5f94..d21673a2929 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -61,27 +61,29 @@ 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,
+def run_linter(
+        tool: str,
+        args: List[str],
+        env: Optional[Mapping[str, str]] = None,
+        suppress_output: bool = False,
 ) -> None:
+    """
+    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)
 
 
@@ -100,11 +102,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 v2 08/15] iotests/297: Change run_linter() to raise an exception on failure

[John Snow]

Instead of using a process return code as the python function return
value (or just not returning anything at all), allow run_linter() to
raise an exception instead.

The responsibility for printing output on error shifts from the function
itself to the caller, who will know best how to present/format that
information. (Also, "suppress_output" is now a lot more accurate of a
parameter name.)

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index d21673a2929..76d6a23f531 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -70,22 +70,18 @@ def run_linter(
     """
     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.
+    :param suppress_output: If True, suppress all stdout/stderr output.
+    :raise CalledProcessError: If the linter process exits with failure.
     """
-    p = subprocess.run(
+    subprocess.run(
         ('python3', '-m', tool, *args),
         env=env,
-        check=False,
+        check=True,
         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)
-
 
 def main() -> None:
     for linter in ('pylint-3', 'mypy'):
@@ -102,11 +98,19 @@ def main() -> None:
 
     print('=== pylint ===')
     sys.stdout.flush()
-    run_linter('pylint', files, env=env)
+    try:
+        run_linter('pylint', files, env=env)
+    except subprocess.CalledProcessError:
+        # pylint failure will be caught by diffing the IO.
+        pass
 
     print('=== mypy ===')
     sys.stdout.flush()
-    run_linter('mypy', files, env=env, suppress_output=True)
+    try:
+        run_linter('mypy', files, env=env, suppress_output=True)
+    except subprocess.CalledProcessError as exc:
+        if exc.output:
+            print(exc.output)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH v2 09/15] iotests/297: update tool availability checks

[John Snow]

As mentioned in 'iotests/297: Don't rely on distro-specific linter
binaries', these checks are overly strict. Update them to be in-line
with how we actually invoke the linters themselves.

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 76d6a23f531..b2ad8d1cbe0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -18,7 +18,6 @@
 
 import os
 import re
-import shutil
 import subprocess
 import sys
 from typing import List, Mapping, Optional
@@ -84,9 +83,11 @@ def run_linter(
 
 
 def main() -> None:
-    for linter in ('pylint-3', 'mypy'):
-        if shutil.which(linter) is None:
-            iotests.notrun(f'{linter} not found')
+    for linter in ('pylint', 'mypy'):
+        try:
+            run_linter(linter, ['--version'], suppress_output=True)
+        except subprocess.CalledProcessError:
+            iotests.notrun(f"'{linter}' not found")
 
     files = get_test_files()
 
-- 
2.31.1

[PATCH v2 10/15] iotests/297: split test into sub-cases

[John Snow]

Take iotest 297's main() test function and split it into two sub-cases
that can be skipped individually. We can also drop custom environment
setup from the pylint test as it isn't needed.

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b2ad8d1cbe0..b7d9d6077b3 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -82,36 +82,51 @@ def run_linter(
     )
 
 
+def check_linter(linter: str) -> bool:
+    try:
+        run_linter(linter, ['--version'], suppress_output=True)
+    except subprocess.CalledProcessError:
+        iotests.case_notrun(f"'{linter}' not found")
+        return False
+    return True
+
+
+def test_pylint(files: List[str]) -> None:
+    print('=== pylint ===')
+    sys.stdout.flush()
+
+    if not check_linter('pylint'):
+        return
+
+    run_linter('pylint', files)
+
+
+def test_mypy(files: List[str]) -> None:
+    print('=== mypy ===')
+    sys.stdout.flush()
+
+    if not check_linter('mypy'):
+        return
+
+    env = os.environ.copy()
+    env['MYPYPATH'] = env['PYTHONPATH']
+
+    run_linter('mypy', files, env=env, suppress_output=True)
+
+
 def main() -> None:
-    for linter in ('pylint', 'mypy'):
-        try:
-            run_linter(linter, ['--version'], suppress_output=True)
-        except subprocess.CalledProcessError:
-            iotests.notrun(f"'{linter}' not found")
-
     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']
-
-    print('=== pylint ===')
-    sys.stdout.flush()
-    try:
-        run_linter('pylint', files, env=env)
-    except subprocess.CalledProcessError:
-        # pylint failure will be caught by diffing the IO.
-        pass
-
-    print('=== mypy ===')
-    sys.stdout.flush()
-    try:
-        run_linter('mypy', files, env=env, suppress_output=True)
-    except subprocess.CalledProcessError as exc:
-        if exc.output:
-            print(exc.output)
+    for test in (test_pylint, test_mypy):
+        try:
+            test(files)
+        except subprocess.CalledProcessError as exc:
+            # Linter failure will be caught by diffing the IO.
+            if exc.output:
+                print(exc.output)
 
 
 iotests.script_main(main)
-- 
2.31.1

[PATCH v2 11/15] 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 | 76 +++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 61 deletions(-)
 create mode 100644 tests/qemu-iotests/linters.py

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index b7d9d6077b3..ee78a627359 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,74 +17,24 @@
 # 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
+from typing import List
 
 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,
-) -> None:
-    """
-    Run a python-based linting tool.
-
-    :param suppress_output: If True, suppress all stdout/stderr output.
-    :raise CalledProcessError: If the linter process exits with failure.
-    """
-    subprocess.run(
-        ('python3', '-m', tool, *args),
-        env=env,
-        check=True,
-        stdout=subprocess.PIPE if suppress_output else None,
-        stderr=subprocess.STDOUT if suppress_output else None,
-        universal_newlines=True,
-    )
+# Looking for something?
+#
+#  List of files to exclude from linting: linters.py
+#  mypy configuration:                    mypy.ini
+#  pylint configuration:                  pylintrc
 
 
 def check_linter(linter: str) -> bool:
     try:
-        run_linter(linter, ['--version'], suppress_output=True)
+        linters.run_linter(linter, ['--version'], suppress_output=True)
     except subprocess.CalledProcessError:
         iotests.case_notrun(f"'{linter}' not found")
         return False
@@ -98,7 +48,7 @@ def test_pylint(files: List[str]) -> None:
     if not check_linter('pylint'):
         return
 
-    run_linter('pylint', files)
+    linters.run_linter('pylint', files)
 
 
 def test_mypy(files: List[str]) -> None:
@@ -111,11 +61,11 @@ def test_mypy(files: List[str]) -> None:
     env = os.environ.copy()
     env['MYPYPATH'] = env['PYTHONPATH']
 
-    run_linter('mypy', files, env=env, suppress_output=True)
+    linters.run_linter('mypy', files, env=env, suppress_output=True)
 
 
 def main() -> None:
-    files = get_test_files()
+    files = linters.get_test_files()
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100644
index 00000000000..c515c7afe36
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,76 @@
+# 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,
+) -> None:
+    """
+    Run a python-based linting tool.
+
+    :param suppress_output: If True, suppress all stdout/stderr output.
+    :raise CalledProcessError: If the linter process exits with failure.
+    """
+    subprocess.run(
+        ('python3', '-m', tool, *args),
+        env=env,
+        check=True,
+        stdout=subprocess.PIPE if suppress_output else None,
+        stderr=subprocess.STDOUT if suppress_output else None,
+        universal_newlines=True,
+    )
-- 
2.31.1

[PATCH v2 12/15] 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>
---
 tests/qemu-iotests/linters.py | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
index c515c7afe36..46c28fdcda0 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
 
 
@@ -74,3 +75,29 @@ def run_linter(
         stderr=subprocess.STDOUT if suppress_output else None,
         universal_newlines=True,
     )
+
+
+def main() -> None:
+    """
+    Used by the Python CI system as an entry point to run these linters.
+    """
+    def show_usage() -> None:
+        print(f"Usage: {sys.argv[0]} < --mypy | --pylint >", file=sys.stderr)
+        sys.exit(1)
+
+    if len(sys.argv) != 2:
+        show_usage()
+
+    files = get_test_files()
+
+    if sys.argv[1] == '--pylint':
+        run_linter('pylint', files)
+    elif sys.argv[1] == '--mypy':
+        run_linter('mypy', files)
+    else:
+        print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr)
+        show_usage()
+
+
+if __name__ == '__main__':
+    main()
-- 
2.31.1

[PATCH v2 13/15] 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>
Reviewed-by: Hanna Reitz <hreitz@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 46c28fdcda0..65c4c4e8272 100644
--- a/tests/qemu-iotests/linters.py
+++ b/tests/qemu-iotests/linters.py
@@ -93,7 +93,9 @@ def show_usage() -> None:
     if sys.argv[1] == '--pylint':
         run_linter('pylint', files)
     elif sys.argv[1] == '--mypy':
-        run_linter('mypy', files)
+        # mypy bug #9852; disable incremental checking as a workaround.
+        args = ['--no-incremental'] + files
+        run_linter('mypy', args)
     else:
         print(f"Unrecognized argument: '{sys.argv[1]}'", file=sys.stderr)
         show_usage()
-- 
2.31.1

[PATCH v2 14/15] 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>
Reviewed-by: Hanna Reitz <hreitz@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 v2 15/15] 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        | 82 -----------------------------------
 tests/qemu-iotests/297.out    |  2 -
 tests/qemu-iotests/linters.py | 14 +-----
 3 files changed, 2 insertions(+), 96 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 ee78a627359..00000000000
--- a/tests/qemu-iotests/297
+++ /dev/null
@@ -1,82 +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 subprocess
-import sys
-from typing import List
-
-import iotests
-import linters
-
-
-# Looking for something?
-#
-#  List of files to exclude from linting: linters.py
-#  mypy configuration:                    mypy.ini
-#  pylint configuration:                  pylintrc
-
-
-def check_linter(linter: str) -> bool:
-    try:
-        linters.run_linter(linter, ['--version'], suppress_output=True)
-    except subprocess.CalledProcessError:
-        iotests.case_notrun(f"'{linter}' not found")
-        return False
-    return True
-
-
-def test_pylint(files: List[str]) -> None:
-    print('=== pylint ===')
-    sys.stdout.flush()
-
-    if not check_linter('pylint'):
-        return
-
-    linters.run_linter('pylint', files)
-
-
-def test_mypy(files: List[str]) -> None:
-    print('=== mypy ===')
-    sys.stdout.flush()
-
-    if not check_linter('mypy'):
-        return
-
-    env = os.environ.copy()
-    env['MYPYPATH'] = env['PYTHONPATH']
-
-    linters.run_linter('mypy', files, env=env, suppress_output=True)
-
-
-def main() -> None:
-    files = linters.get_test_files()
-
-    iotests.logger.debug('Files to be checked:')
-    iotests.logger.debug(', '.join(sorted(files)))
-
-    for test in (test_pylint, test_mypy):
-        try:
-            test(files)
-        except subprocess.CalledProcessError as exc:
-            # Linter failure will be caught by diffing the IO.
-            if exc.output:
-                print(exc.output)
-
-
-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 65c4c4e8272..486b7125c1d 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,25 +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,
-) -> None:
+def run_linter(tool: str, args: List[str]) -> None:
     """
     Run a python-based linting tool.
 
-    :param suppress_output: If True, suppress all stdout/stderr output.
     :raise CalledProcessError: If the linter process exits with failure.
     """
     subprocess.run(
         ('python3', '-m', tool, *args),
-        env=env,
         check=True,
-        stdout=subprocess.PIPE if suppress_output else None,
-        stderr=subprocess.STDOUT if suppress_output else None,
-        universal_newlines=True,
     )
 
 
-- 
2.31.1

Re: [PATCH v2 07/15] iotests/297: refactor run_[mypy|pylint] as generic execution shim

[Hanna Reitz]

On 19.10.21 16:49, 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 | 42 ++++++++++++++++++++++--------------------
>   1 file changed, 22 insertions(+), 20 deletions(-)

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

Re: [PATCH v2 08/15] iotests/297: Change run_linter() to raise an exception on failure

[Hanna Reitz]

On 19.10.21 16:49, John Snow wrote:
> Instead of using a process return code as the python function return
> value (or just not returning anything at all), allow run_linter() to
> raise an exception instead.
>
> The responsibility for printing output on error shifts from the function
> itself to the caller, who will know best how to present/format that
> information. (Also, "suppress_output" is now a lot more accurate of a
> parameter name.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)

Thanks! :)

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

Re: [PATCH v2 09/15] iotests/297: update tool availability checks

[Hanna Reitz]

On 19.10.21 16:49, John Snow wrote:
> As mentioned in 'iotests/297: Don't rely on distro-specific linter
> binaries', these checks are overly strict. Update them to be in-line
> with how we actually invoke the linters themselves.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)

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

Re: [PATCH v2 10/15] iotests/297: split test into sub-cases

[Hanna Reitz]

On 19.10.21 16:49, John Snow wrote:
> Take iotest 297's main() test function and split it into two sub-cases
> that can be skipped individually. We can also drop custom environment
> setup from the pylint test as it isn't needed.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 63 ++++++++++++++++++++++++++----------------
>   1 file changed, 39 insertions(+), 24 deletions(-)

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

(while heavily scratching myself to ease the itch to turn this into a 
unit test now)

Re: [PATCH v2 11/15] iotests: split linters.py out from 297

[Hanna Reitz]

On 19.10.21 16:49, 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,
> 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 | 76 +++++++++++++++++++++++++++++++++++
>   2 files changed, 87 insertions(+), 61 deletions(-)
>   create mode 100644 tests/qemu-iotests/linters.py

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

I wonder about `check_linter()`, though.  By not moving it to 
linters.py, we can’t use it in its entry point, and so the Python test 
infrastructure will have a strong dependency on these linters. Though 
then again, it probably already does, and I suppose that’s one of the 
points hindering us from running this from make check?

Hanna

Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI

[Hanna Reitz]

On 19.10.21 16:49, 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>
> ---
>   tests/qemu-iotests/linters.py | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)

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

Re: [PATCH v2 08/15] iotests/297: Change run_linter() to raise an exception on failure

[John Snow]

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

On Tue, Oct 26, 2021 at 6:10 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 19.10.21 16:49, John Snow wrote:
> > Instead of using a process return code as the python function return
> > value (or just not returning anything at all), allow run_linter() to
> > raise an exception instead.
> >
> > The responsibility for printing output on error shifts from the function
> > itself to the caller, who will know best how to present/format that
> > information. (Also, "suppress_output" is now a lot more accurate of a
> > parameter name.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297 | 24 ++++++++++++++----------
> >   1 file changed, 14 insertions(+), 10 deletions(-)
>
> Thanks! :)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
No problem. Thanks for taking the time to review it!

--js

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

Re: [PATCH v2 10/15] iotests/297: split test into sub-cases

[John Snow]

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

On Tue, Oct 26, 2021 at 6:29 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 19.10.21 16:49, John Snow wrote:
> > Take iotest 297's main() test function and split it into two sub-cases
> > that can be skipped individually. We can also drop custom environment
> > setup from the pylint test as it isn't needed.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297 | 63 ++++++++++++++++++++++++++----------------
> >   1 file changed, 39 insertions(+), 24 deletions(-)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
> (while heavily scratching myself to ease the itch to turn this into a
> unit test now)
>
>
(I strongly considered it, but didn't want to add yet-more-rewrites into
this series. If the ultimate fate is to sunset this particular iotest, I
didn't see a big benefit to doing it.)

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

Re: [PATCH v2 11/15] iotests: split linters.py out from 297

[John Snow]

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

On Tue, Oct 26, 2021 at 6:51 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 19.10.21 16:49, 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,
> > 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 | 76 +++++++++++++++++++++++++++++++++++
> >   2 files changed, 87 insertions(+), 61 deletions(-)
> >   create mode 100644 tests/qemu-iotests/linters.py
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
Thanks!


> I wonder about `check_linter()`, though.  By not moving it to
> linters.py, we can’t use it in its entry point, and so the Python test
> infrastructure will have a strong dependency on these linters. Though
> then again, it probably already does, and I suppose that’s one of the
> points hindering us from running this from make check?
>
>
That one is left behind because it uses iotests API to skip a test.
Environment setup that guarantees we won't *need* to skip the test is
handled by the virtual environment setup magic in qemu/python/Makefile.


> Hanna
>

More info than you require:

There's maybe about four ways you could run the python tests that might
make sense depending on who you are and what you're trying to accomplish;
they're documented in "make help" and again in qemu/python/README.rst;

(1) make check-dev -- makes a venv with whatever python you happen to have,
installs the latest packages, runs the tests
(2) make check-pipenv -- requires python 3.6 specifically, installs the
*oldest* packages, runs the tests
(3) make check-tox -- requires python 3.6 through 3.10, installs the newest
packages, runs the tests per each python version
(4) make check -- perform no setup at all, just run the tests in the
current environment. (Used directly by all three prior invocations)

In general, I assume that human beings that aren't working on Python stuff
actively will be using (1) at their interactive console, if they decide to
run any of these at all. It imposes the least pre-requirements while still
endeavoring to be a target that will "just work".
Options (2) and (3) are what power the CI tests 'check-python-pipenv' and
'check-python-tox', respectively; with(2) being the one that actually gates
the CI.
Option (4) is only really a convenience for bypassing the venv-setup stuff
if you want to construct your own (or not use one at all). So the tests
just assume that the tools they have available will work. It's kind of a
'power user' target.

The way the CI works at the moment is that it uses a "fedora:latest" base
image and installs python interpreters 3.6 through 3.10 inclusive, pipenv,
and tox. From there, it has enough juice to run the makefile targets which
take care of all of the actual linting dependencies and their different
versions to get a wider matrix on the version testing to ensure we're still
keeping compatibility with the 3.6 platform while also checking for new
problems that show up on the bleeding edge.

iotests 297 right now doesn't do any python environment setup at all, so we
can't guarantee that the linters are present, so we decide to allow the
test to be skipped. My big hesitation of adding this test directly into
'make check' is that I will need to do some environment probing to make
sure that the 'pylint' version isn't too old -- or otherwise set up a venv
in the build directory that can be used to run tests. I know we already set
one up for the acceptance tests, so maybe there's an opportunity to re-use
that venv, but I need to make sure that the dependencies between the two
sets of tests are aligned. I don't know if they agree, currently.

I think I probably want to minimize the number of different virtual python
environments we create during the build, so I will look into what problems
might exist in re-purposing the acceptance test venv. If that can get
squared away easily, there's likely no real big barrier to adding more
tests that rely on a python venv to get cooking during the normal
build/check process, including iotest 297.

--js

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

Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI

[John Snow]

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

On Tue, Oct 26, 2021 at 6:57 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 19.10.21 16:49, 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>
> > ---
> >   tests/qemu-iotests/linters.py | 27 +++++++++++++++++++++++++++
> >   1 file changed, 27 insertions(+)
>
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
Thanks! I'll endeavor to try and clean up the list of exempted files to
continue cleaning up this mess, but it's not a top priority right now. I'll
put it on the backburner after I finish typing the QAPI generator. A lot of
the weird compatibility goop will go away over time as I consolidate more
of the venv logic.

(I think this series is good to go, now? I think it could be applied in any
order vs my other series. If you want, if/when you give the go-ahead for
the other series, I could just stage them both myself and make sure they
work well together and save you the headache.)

--js

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

Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI

[John Snow]

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

On Tue, Oct 26, 2021 at 2:36 PM John Snow <jsnow@redhat.com> wrote:

>
>
> On Tue, Oct 26, 2021 at 6:57 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>> On 19.10.21 16:49, 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>
>> > ---
>> >   tests/qemu-iotests/linters.py | 27 +++++++++++++++++++++++++++
>> >   1 file changed, 27 insertions(+)
>>
>> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>>
>>
> Thanks! I'll endeavor to try and clean up the list of exempted files to
> continue cleaning up this mess, but it's not a top priority right now. I'll
> put it on the backburner after I finish typing the QAPI generator. A lot of
> the weird compatibility goop will go away over time as I consolidate more
> of the venv logic.
>
> (I think this series is good to go, now? I think it could be applied in
> any order vs my other series. If you want, if/when you give the go-ahead
> for the other series, I could just stage them both myself and make sure
> they work well together and save you the headache.)
>

Update: I pre-emptively staged both series (the iotests one first, followed
by the AQMP one) to jsnow/python and verified that all of the python tests
pass for each commit between:

[14] python-add-iotest-linters-to   # python: Add iotest linters to test
suite
...
[22] python-iotests-replace-qmp     # python, iotests: replace qmp with aqmp

and I'm running the CI on all of that now at
https://gitlab.com/jsnow/qemu/-/pipelines/396002744

(I just wanted to double-check they didn't conflict with each other in any
unanticipated ways. Let me know if I should send the PR or if that'll just
create hassle for you.)

--js

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

Re: [PATCH v2 11/15] iotests: split linters.py out from 297

[Hanna Reitz]

On 26.10.21 20:30, John Snow wrote:
>
>
> On Tue, Oct 26, 2021 at 6:51 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 19.10.21 16:49, 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,
>     > 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 | 76
>     +++++++++++++++++++++++++++++++++++
>     >   2 files changed, 87 insertions(+), 61 deletions(-)
>     >   create mode 100644 tests/qemu-iotests/linters.py
>
>     Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
> Thanks!
>
>     I wonder about `check_linter()`, though.  By not moving it to
>     linters.py, we can’t use it in its entry point, and so the Python
>     test
>     infrastructure will have a strong dependency on these linters. Though
>     then again, it probably already does, and I suppose that’s one of the
>     points hindering us from running this from make check?
>
>
> That one is left behind because it uses iotests API to skip a test. 
> Environment setup that guarantees we won't *need* to skip the test is 
> handled by the virtual environment setup magic in qemu/python/Makefile.
>
>     Hanna
>
>
> More info than you require:
>
> There's maybe about four ways you could run the python tests that 
> might make sense depending on who you are and what you're trying to 
> accomplish; they're documented in "make help" and again in 
> qemu/python/README.rst;
>
> (1) make check-dev -- makes a venv with whatever python you happen to 
> have, installs the latest packages, runs the tests
> (2) make check-pipenv -- requires python 3.6 specifically, installs 
> the *oldest* packages, runs the tests
> (3) make check-tox -- requires python 3.6 through 3.10, installs the 
> newest packages, runs the tests per each python version
> (4) make check -- perform no setup at all, just run the tests in the 
> current environment. (Used directly by all three prior invocations)

AFAIU these are all to be run in build/python?  Isn’t any of them hooked 
up to the global `make check` in build?  Because...

> In general, I assume that human beings that aren't working on Python 
> stuff actively will be using (1) at their interactive console, if they 
> decide to run any of these at all.

...I’m just running `make check` in the build directory.

I would have hoped that this is hooked up to something that won’t fail 
because I don’t have some necessary tools installed or perhaps even 
because I have the wrong version of some tools installed (although the 
latter would be kind of questionable...).  Would be nice if the global 
`make check` had a summary on what was skipped...

> It imposes the least pre-requirements while still endeavoring to be a 
> target that will "just work".
> Options (2) and (3) are what power the CI tests 'check-python-pipenv' 
> and 'check-python-tox', respectively; with(2) being the one that 
> actually gates the CI.
> Option (4) is only really a convenience for bypassing the venv-setup 
> stuff if you want to construct your own (or not use one at all). So 
> the tests just assume that the tools they have available will work. 
> It's kind of a 'power user' target.
>
> The way the CI works at the moment is that it uses a "fedora:latest" 
> base image and installs python interpreters 3.6 through 3.10 
> inclusive, pipenv, and tox. From there, it has enough juice to run the 
> makefile targets which take care of all of the actual linting 
> dependencies and their different versions to get a wider matrix on the 
> version testing to ensure we're still keeping compatibility with the 
> 3.6 platform while also checking for new problems that show up on the 
> bleeding edge.

Perhaps it’s unreasonable, but I’d prefer if a local `make check` would 
already run most tests in some form or another and that I don’t have to 
push to gitlab and wait for the CI there.

I mean, I can of course also integrate a `cd python && make check-dev` 
invocation into my test scripts, but it doesn’t feel right to 
special-case one test area.

> iotests 297 right now doesn't do any python environment setup at all, 
> so we can't guarantee that the linters are present, so we decide to 
> allow the test to be skipped. My big hesitation of adding this test 
> directly into 'make check' is that I will need to do some environment 
> probing to make sure that the 'pylint' version isn't too old -- or 
> otherwise set up a venv in the build directory that can be used to run 
> tests. I know we already set one up for the acceptance tests, so maybe 
> there's an opportunity to re-use that venv, but I need to make sure 
> that the dependencies between the two sets of tests are aligned. I 
> don't know if they agree, currently.

I see.

> I think I probably want to minimize the number of different virtual 
> python environments we create during the build, so I will look into 
> what problems might exist in re-purposing the acceptance test venv. If 
> that can get squared away easily, there's likely no real big barrier 
> to adding more tests that rely on a python venv to get cooking during 
> the normal build/check process, including iotest 297.

OK, thanks for the explanation!

Hanna

Re: [PATCH v2 12/15] iotests/linters: Add entry point for linting via Python CI

[Hanna Reitz]

On 26.10.21 21:45, John Snow wrote:
>
>
> On Tue, Oct 26, 2021 at 2:36 PM John Snow <jsnow@redhat.com> wrote:
>
>
>
>     On Tue, Oct 26, 2021 at 6:57 AM Hanna Reitz <hreitz@redhat.com> wrote:
>
>         On 19.10.21 16:49, 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>
>         > ---
>         >   tests/qemu-iotests/linters.py | 27 +++++++++++++++++++++++++++
>         >   1 file changed, 27 insertions(+)
>
>         Reviewed-by: Hanna Reitz <hreitz@redhat.com>
>
>
>     Thanks! I'll endeavor to try and clean up the list of exempted
>     files to continue cleaning up this mess, but it's not a top
>     priority right now. I'll put it on the backburner after I finish
>     typing the QAPI generator. A lot of the weird compatibility goop
>     will go away over time as I consolidate more of the venv logic.
>
>     (I think this series is good to go, now? I think it could be
>     applied in any order vs my other series. If you want, if/when you
>     give the go-ahead for the other series, I could just stage them
>     both myself and make sure they work well together and save you the
>     headache.)
>
>
> Update: I pre-emptively staged both series (the iotests one first, 
> followed by the AQMP one) to jsnow/python and verified that all of the 
> python tests pass for each commit between:
>
> [14] python-add-iotest-linters-to   # python: Add iotest linters to 
> test suite
> ...
> [22] python-iotests-replace-qmp     # python, iotests: replace qmp 
> with aqmp
>
> and I'm running the CI on all of that now at 
> https://gitlab.com/jsnow/qemu/-/pipelines/396002744
>
> (I just wanted to double-check they didn't conflict with each other in 
> any unanticipated ways. Let me know if I should send the PR or if 
> that'll just create hassle for you.)

No, I’m all good with you taking (the blame for) them. :)

Hanna

Re: [PATCH v2 11/15] iotests: split linters.py out from 297

[John Snow]

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

On Thu, Oct 28, 2021, 6:34 AM Hanna Reitz <hreitz@redhat.com> wrote:

> On 26.10.21 20:30, John Snow wrote:
> >
> >
> > On Tue, Oct 26, 2021 at 6:51 AM Hanna Reitz <hreitz@redhat.com> wrote:
> >
> >     On 19.10.21 16:49, 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,
> >     > 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 | 76
> >     +++++++++++++++++++++++++++++++++++
> >     >   2 files changed, 87 insertions(+), 61 deletions(-)
> >     >   create mode 100644 tests/qemu-iotests/linters.py
> >
> >     Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> >
> >
> > Thanks!
> >
> >     I wonder about `check_linter()`, though.  By not moving it to
> >     linters.py, we can’t use it in its entry point, and so the Python
> >     test
> >     infrastructure will have a strong dependency on these linters. Though
> >     then again, it probably already does, and I suppose that’s one of the
> >     points hindering us from running this from make check?
> >
> >
> > That one is left behind because it uses iotests API to skip a test.
> > Environment setup that guarantees we won't *need* to skip the test is
> > handled by the virtual environment setup magic in qemu/python/Makefile.
> >
> >     Hanna
> >
> >
> > More info than you require:
> >
> > There's maybe about four ways you could run the python tests that
> > might make sense depending on who you are and what you're trying to
> > accomplish; they're documented in "make help" and again in
> > qemu/python/README.rst;
> >
> > (1) make check-dev -- makes a venv with whatever python you happen to
> > have, installs the latest packages, runs the tests
> > (2) make check-pipenv -- requires python 3.6 specifically, installs
> > the *oldest* packages, runs the tests
> > (3) make check-tox -- requires python 3.6 through 3.10, installs the
> > newest packages, runs the tests per each python version
> > (4) make check -- perform no setup at all, just run the tests in the
> > current environment. (Used directly by all three prior invocations)
>
> AFAIU these are all to be run in build/python?  Isn’t any of them hooked
> up to the global `make check` in build?  Because...
>

None of them are on make check, correct. Not yet. I'll try to make that
happen soon to drop 297.

They run out of the source tree directly, since they're checks on the
source and aren't actually related to a build of QEMU in any way.

(Y'know, yet. I'm not saying never.)


> In general, I assume that human beings that aren't working on Python
> > stuff actively will be using (1) at their interactive console, if they
> > decide to run any of these at all.
>
> ...I’m just running `make check` in the build directory.
>

Yeah, that's OK. I mean, I don't expect you to run them unless you were
submitting a series to specifically me.

("If they decide to run any of these at all" - I'm aware that very few
people would or are doing so. I consider the CI to be mostly for me as the
maintainer to make sure nothing regressed.)


> I would have hoped that this is hooked up to something that won’t fail
> because I don’t have some necessary tools installed or perhaps even
> because I have the wrong version of some tools installed (although the
> latter would be kind of questionable...).  Would be nice if the global
> `make check` had a summary on what was skipped...
>


I mean. These targets shouldn't fail unless you're missing some really
basic requirements. They're just not hooked in to the big "make check" yet.

In terms of environment probing and skipping the tests, though, I do need
another layer somewhere to manage that. Stuff I'll need to put in
configure/meson etc. I have to look into it.


> > It imposes the least pre-requirements while still endeavoring to be a
> > target that will "just work".
> > Options (2) and (3) are what power the CI tests 'check-python-pipenv'
> > and 'check-python-tox', respectively; with(2) being the one that
> > actually gates the CI.
> > Option (4) is only really a convenience for bypassing the venv-setup
> > stuff if you want to construct your own (or not use one at all). So
> > the tests just assume that the tools they have available will work.
> > It's kind of a 'power user' target.
> >
> > The way the CI works at the moment is that it uses a "fedora:latest"
> > base image and installs python interpreters 3.6 through 3.10
> > inclusive, pipenv, and tox. From there, it has enough juice to run the
> > makefile targets which take care of all of the actual linting
> > dependencies and their different versions to get a wider matrix on the
> > version testing to ensure we're still keeping compatibility with the
> > 3.6 platform while also checking for new problems that show up on the
> > bleeding edge.
>
> Perhaps it’s unreasonable, but I’d prefer if a local `make check` would
> already run most tests in some form or another and that I don’t have to
> push to gitlab and wait for the CI there.
>

Yep, understand. That's a requirement for me as well in order to drop 297.
On the list, I promise.


> I mean, I can of course also integrate a `cd python && make check-dev`
> invocation into my test scripts, but it doesn’t feel right to
> special-case one test area.
>

Don't worry, I don't expect that. It just took a lot of work to standardize
even that much of the test, so I went for the smaller thing instead of the
perfect thing. I'm still inching along to the perfect thing, but considered
the iotest cleanups I've done a requisite on that path.



> > iotests 297 right now doesn't do any python environment setup at all,
> > so we can't guarantee that the linters are present, so we decide to
> > allow the test to be skipped. My big hesitation of adding this test
> > directly into 'make check' is that I will need to do some environment
> > probing to make sure that the 'pylint' version isn't too old -- or
> > otherwise set up a venv in the build directory that can be used to run
> > tests. I know we already set one up for the acceptance tests, so maybe
> > there's an opportunity to re-use that venv, but I need to make sure
> > that the dependencies between the two sets of tests are aligned. I
> > don't know if they agree, currently.
>
> I see.
>
> > I think I probably want to minimize the number of different virtual
> > python environments we create during the build, so I will look into
> > what problems might exist in re-purposing the acceptance test venv. If
> > that can get squared away easily, there's likely no real big barrier
> > to adding more tests that rely on a python venv to get cooking during
> > the normal build/check process, including iotest 297.
>
> OK, thanks for the explanation!
>
> Hanna
>

Yep. I'll start trying to integrate this into make check and see where the
problems are. It should be safe to do during soft freeze, I think, since
it's just testing ...

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