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

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

[John Snow]

Based-on: <20210625154540.783306-1-jsnow@redhat.com>
GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-package-iotest
CI: https://gitlab.com/jsnow/qemu/-/pipelines/327413868

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

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

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

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

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

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

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

Here are example outputs from each of the three different local
execution methods, in order as outlined above:

(1)

jsnow@scv ~/s/q/python (python-package-iotest)> make check-venv
VENV ~/.cache/qemu-pyvenv
ACTIVATE ~/.cache/qemu-pyvenv
INSTALL qemu[devel] ~/.cache/qemu-pyvenv
ACTIVATE ~/.cache/qemu-pyvenv
make[1]: Entering directory '/home/jsnow/src/qemu/python'
JOB ID     : 645eacd5ed7f4d5a76ea5f494984a55403a11081
JOB LOG    : /home/jsnow/avocado/job-results/job-2021-06-25T12.01-645eacd/job.log
 (1/5) tests/flake8.sh: PASS (0.28 s)
 (2/5) tests/iotests.sh: PASS (9.61 s)
 (3/5) tests/isort.sh: PASS (0.09 s)
 (4/5) tests/mypy.sh: PASS (0.25 s)
 (5/5) tests/pylint.sh: PASS (4.13 s)
RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 14.73 s
make[1]: Leaving directory '/home/jsnow/src/qemu/python'

(2)

jsnow@scv ~/s/q/python (python-package-iotest)> make check-pipenv
Creating a virtualenv for this project...
Pipfile: /home/jsnow/src/qemu/python/Pipfile
Using /usr/bin/python3.6m (3.6.13) to create virtualenv...
⠹ Creating virtual environment...created virtual environment CPython3.6.13.final.0-64 in 104ms
  creator CPython3Posix(dest=/home/jsnow/src/qemu/python/.venv, clear=False, no_vcs_ignore=False, global=False)
  seeder FromAppData(download=False, pip=bundle, setuptools=bundle, wheel=bundle, via=copy, app_data_dir=/home/jsnow/.local/share/virtualenv)
    added seed packages: pip==21.1.2, setuptools==57.0.0, wheel==0.36.2
  activators BashActivator,CShellActivator,FishActivator,PowerShellActivator,PythonActivator,XonshActivator

✔ Successfully created virtual environment!
Virtualenv location: /home/jsnow/src/qemu/python/.venv
Installing dependencies from Pipfile.lock (c13e91)...
  🐍   ▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉ 30/30 — 00:00:08
To activate this project's virtualenv, run pipenv shell.
Alternatively, run a command inside the virtualenv with pipenv run.
All dependencies are now up-to-date!
rm -f pyproject.toml
make[1]: Entering directory '/home/jsnow/src/qemu/python'
JOB ID     : a56f53f3c5ef0058d341917228362db8ff24075f
JOB LOG    : /home/jsnow/avocado/job-results/job-2021-06-25T11.58-a56f53f/job.log
 (1/5) tests/flake8.sh: PASS (0.42 s)
 (2/5) tests/iotests.sh: PASS (8.89 s)
 (3/5) tests/isort.sh: PASS (0.24 s)
 (4/5) tests/mypy.sh: PASS (0.30 s)
 (5/5) tests/pylint.sh: PASS (3.96 s)
RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 14.16 s
make[1]: Leaving directory '/home/jsnow/src/qemu/python'

(3)

jsnow@scv ~/s/q/python (python-package-iotest)> make check-tox
GLOB sdist-make: /home/jsnow/src/qemu/python/setup.py
py36 create: /home/jsnow/src/qemu/python/.tox/py36
py36 installdeps: .[devel], .[fuse]
py36 inst: /home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip
py36 installed: appdirs==1.4.4,astroid==2.5.6,avocado-framework==89.0,distlib==0.3.2,filelock==3.0.12,flake8==3.9.2,fusepy==3.0.1,importlib-metadata==4.5.0,importlib-resources==5.1.4,isort==5.9.1,lazy-object-proxy==1.6.0,mccabe==0.6.1,mypy==0.910,mypy-extensions==0.4.3,packaging==20.9,pluggy==0.13.1,py==1.10.0,pycodestyle==2.7.0,pyflakes==2.3.1,pylint==2.8.3,pyparsing==2.4.7,qemu @ file:///home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip,six==1.16.0,toml==0.10.2,tox==3.23.1,typed-ast==1.4.3,typing-extensions==3.10.0.0,virtualenv==20.4.7,wrapt==1.12.1,zipp==3.4.1
py36 run-test-pre: PYTHONHASHSEED='835116428'
py36 run-test: commands[0] | make check
JOB ID     : c37bc201d0ec4d22a9f8b9347e3848e47fe88ba8
JOB LOG    : /home/jsnow/avocado/job-results/job-2021-06-25T11.59-c37bc20/job.log
 (1/5) tests/flake8.sh:  PASS (0.36 s)
 (2/5) tests/iotests.sh:  PASS (10.64 s)
 (3/5) tests/isort.sh:  PASS (0.11 s)
 (4/5) tests/mypy.sh:  PASS (1.31 s)
 (5/5) tests/pylint.sh:  PASS (4.45 s)
RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 17.28 s
py37 create: /home/jsnow/src/qemu/python/.tox/py37
py37 installdeps: .[devel], .[fuse]
py37 inst: /home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip
py37 installed: appdirs==1.4.4,astroid==2.5.6,avocado-framework==89.0,distlib==0.3.2,filelock==3.0.12,flake8==3.9.2,fusepy==3.0.1,importlib-metadata==4.5.0,isort==5.9.1,lazy-object-proxy==1.6.0,mccabe==0.6.1,mypy==0.910,mypy-extensions==0.4.3,packaging==20.9,pluggy==0.13.1,py==1.10.0,pycodestyle==2.7.0,pyflakes==2.3.1,pylint==2.8.3,pyparsing==2.4.7,qemu @ file:///home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip,six==1.16.0,toml==0.10.2,tox==3.23.1,typed-ast==1.4.3,typing-extensions==3.10.0.0,virtualenv==20.4.7,wrapt==1.12.1,zipp==3.4.1
py37 run-test-pre: PYTHONHASHSEED='835116428'
py37 run-test: commands[0] | make check
JOB ID     : 5fd0aaadb8557f941d773dd28f0017bed7b36526
JOB LOG    : /home/jsnow/avocado/job-results/job-2021-06-25T11.59-5fd0aaa/job.log
 (1/5) tests/flake8.sh:  PASS (0.32 s)
 (2/5) tests/iotests.sh:  PASS (11.52 s)
 (3/5) tests/isort.sh:  PASS (0.09 s)
 (4/5) tests/mypy.sh:  PASS (0.27 s)
 (5/5) tests/pylint.sh:  PASS (4.44 s)
RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 17.03 s
py38 create: /home/jsnow/src/qemu/python/.tox/py38
py38 installdeps: .[devel], .[fuse]
py38 inst: /home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip
py38 installed: appdirs==1.4.4,astroid==2.5.6,avocado-framework==89.0,distlib==0.3.2,filelock==3.0.12,flake8==3.9.2,fusepy==3.0.1,isort==5.9.1,lazy-object-proxy==1.6.0,mccabe==0.6.1,mypy==0.910,mypy-extensions==0.4.3,packaging==20.9,pluggy==0.13.1,py==1.10.0,pycodestyle==2.7.0,pyflakes==2.3.1,pylint==2.8.3,pyparsing==2.4.7,qemu @ file:///home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip,six==1.16.0,toml==0.10.2,tox==3.23.1,typing-extensions==3.10.0.0,virtualenv==20.4.7,wrapt==1.12.1
py38 run-test-pre: PYTHONHASHSEED='835116428'
py38 run-test: commands[0] | make check
JOB ID     : dde7217f6afa2cb7496b8b8da3dee57783fa6377
JOB LOG    : /home/jsnow/avocado/job-results/job-2021-06-25T11.59-dde7217/job.log
 (1/5) tests/flake8.sh:  PASS (0.25 s)
 (2/5) tests/iotests.sh:  PASS (10.17 s)
 (3/5) tests/isort.sh:  PASS (0.08 s)
 (4/5) tests/mypy.sh:  PASS (0.25 s)
 (5/5) tests/pylint.sh:  PASS (3.96 s)
RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 15.08 s
py39 create: /home/jsnow/src/qemu/python/.tox/py39
py39 installdeps: .[devel], .[fuse]
py39 inst: /home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip
py39 installed: appdirs==1.4.4,astroid==2.5.6,avocado-framework==89.0,distlib==0.3.2,filelock==3.0.12,flake8==3.9.2,fusepy==3.0.1,isort==5.9.1,lazy-object-proxy==1.6.0,mccabe==0.6.1,mypy==0.910,mypy-extensions==0.4.3,packaging==20.9,pluggy==0.13.1,py==1.10.0,pycodestyle==2.7.0,pyflakes==2.3.1,pylint==2.8.3,pyparsing==2.4.7,qemu @ file:///home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip,six==1.16.0,toml==0.10.2,tox==3.23.1,typing-extensions==3.10.0.0,virtualenv==20.4.7,wrapt==1.12.1
py39 run-test-pre: PYTHONHASHSEED='835116428'
py39 run-test: commands[0] | make check
JOB ID     : 9c30aa6af6aece73acd995603a11df6f6d609b43
JOB LOG    : /home/jsnow/avocado/job-results/job-2021-06-25T12.00-9c30aa6/job.log
 (1/5) tests/flake8.sh:  PASS (0.31 s)
 (2/5) tests/iotests.sh:  PASS (10.97 s)
 (3/5) tests/isort.sh:  PASS (0.09 s)
 (4/5) tests/mypy.sh:  PASS (0.24 s)
 (5/5) tests/pylint.sh:  PASS (4.14 s)
RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 16.11 s
py310 create: /home/jsnow/src/qemu/python/.tox/py310
py310 installdeps: .[devel], .[fuse]
py310 inst: /home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip
py310 installed: appdirs==1.4.4,astroid==2.5.6,avocado-framework==89.0,distlib==0.3.2,filelock==3.0.12,flake8==3.9.2,fusepy==3.0.1,isort==5.9.1,lazy-object-proxy==1.6.0,mccabe==0.6.1,mypy==0.910,mypy-extensions==0.4.3,packaging==20.9,pluggy==0.13.1,py==1.10.0,pycodestyle==2.7.0,pyflakes==2.3.1,pylint==2.8.3,pyparsing==2.4.7,qemu @ file:///home/jsnow/src/qemu/python/.tox/.tmp/package/1/qemu-0.6.1.0a1.zip,six==1.16.0,toml==0.10.2,tox==3.23.1,typing-extensions==3.10.0.0,virtualenv==20.4.7,wrapt==1.12.1
py310 run-test-pre: PYTHONHASHSEED='835116428'
py310 run-test: commands[0] | make check
JOB ID     : 24de320dd6b9130a6986c67ea3bf9aff6af7b134
JOB LOG    : /home/jsnow/avocado/job-results/job-2021-06-25T12.00-24de320/job.log
 (1/5) tests/flake8.sh:  PASS (0.29 s)
 (2/5) tests/iotests.sh:  PASS (18.38 s)
 (3/5) tests/isort.sh:  PASS (0.09 s)
 (4/5) tests/mypy.sh:  PASS (0.54 s)
 (5/5) tests/pylint.sh:  PASS (4.10 s)
RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 23.70 s
___________________________________ summary ___________________________________
  py36: commands succeeded
  py37: commands succeeded
  py38: commands succeeded
  py39: commands succeeded
  py310: commands succeeded
  congratulations :)

John Snow (10):
  iotests/297: modify is_python_file to work from any CWD
  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: Add 'directory' argument to run_linters
  iotests/297: return error code from run_linters()
  iotests/297: split linters.py off from 297
  iotests/linters: Add entry point for Python CI linters
  python: Add iotest linters to test suite

 python/tests/iotests.sh       |   2 +
 tests/qemu-iotests/297        |  80 +++------------------
 tests/qemu-iotests/linters.py | 130 ++++++++++++++++++++++++++++++++++
 3 files changed, 141 insertions(+), 71 deletions(-)
 create mode 100755 python/tests/iotests.sh
 create mode 100755 tests/qemu-iotests/linters.py

-- 
2.31.1

[PATCH 01/10] iotests/297: modify is_python_file to work from any CWD

[John Snow]

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

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

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

Re: [PATCH 01/10] iotests/297: modify is_python_file to work from any CWD

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> Add a directory argument to is_python_file to allow it to work correctly
> no matter what CWD we happen to run it from. This is done in
> anticipation of running the iotests from another directory (./python/).
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 02/10] iotests/297: Add get_files() function

[John Snow]

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

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

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

Re: [PATCH 02/10] iotests/297: Add get_files() function

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> Split out file discovery into its own method to begin separating out the
> "environment setup" and "test execution" phases.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/297 | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 493dda17fb..0bc1195805 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -21,6 +21,7 @@ import re
>   import shutil
>   import subprocess
>   import sys
> +from typing import List
>   
>   import iotests
>   
> @@ -56,9 +57,15 @@ def is_python_file(filename: str, directory: str = '.') -> bool:
>               return False
>   
>   
> +def get_test_files(directory: str = '.') -> List[str]:
> +    return [
> +        f for f in (set(os.listdir(directory)) - set(SKIP_FILES))
> +        if is_python_file(f, directory)
> +    ]
> +
> +
>   def run_linters():
> -    files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
> -             if is_python_file(filename)]
> +    files = get_test_files()

Hmm. It looks like files in tests/qemu-iotests/tests are ignored now.. That's bad

>   
>       iotests.logger.debug('Files to be checked:')
>       iotests.logger.debug(', '.join(sorted(files)))
> 


-- 
Best regards,
Vladimir

Re: [PATCH 02/10] iotests/297: Add get_files() function

[John Snow]

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

On Tue, Jul 6, 2021 at 5:02 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 25.06.2021 21:20, John Snow wrote:
> > Split out file discovery into its own method to begin separating out the
> > "environment setup" and "test execution" phases.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> > ---
> >   tests/qemu-iotests/297 | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 493dda17fb..0bc1195805 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -21,6 +21,7 @@ import re
> >   import shutil
> >   import subprocess
> >   import sys
> > +from typing import List
> >
> >   import iotests
> >
> > @@ -56,9 +57,15 @@ def is_python_file(filename: str, directory: str =
> '.') -> bool:
> >               return False
> >
> >
> > +def get_test_files(directory: str = '.') -> List[str]:
> > +    return [
> > +        f for f in (set(os.listdir(directory)) - set(SKIP_FILES))
> > +        if is_python_file(f, directory)
> > +    ]
> > +
> > +
> >   def run_linters():
> > -    files = [filename for filename in (set(os.listdir('.')) -
> set(SKIP_FILES))
> > -             if is_python_file(filename)]
> > +    files = get_test_files()
>
> Hmm. It looks like files in tests/qemu-iotests/tests are ignored now..
> That's bad
>
>
Oh, it seems likely we were never checking them -- listdir doesn't recurse
before *or* after this patch. OK, I can fix that. It'll be in a new patch.


> >
> >       iotests.logger.debug('Files to be checked:')
> >       iotests.logger.debug(', '.join(sorted(files)))
> >
>
>
> --
> Best regards,
> Vladimir
>
>

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

[PATCH 03/10] 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>
---
 tests/qemu-iotests/297 | 45 ++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 0bc1195805..43af361622 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -82,8 +82,11 @@ def run_linters():
         env['PYTHONPATH'] += os.pathsep + qemu_module_path
     except KeyError:
         env['PYTHONPATH'] = qemu_module_path
-    subprocess.run(('pylint-3', '--score=n', '--notes=FIXME,XXX', *files),
-                   env=env, check=False)
+    subprocess.run(
+        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        env=env,
+        check=False,
+    )
 
     print('=== mypy ===')
     sys.stdout.flush()
@@ -94,23 +97,27 @@ def run_linters():
     # must not both define the __main__ module).
     env['MYPYPATH'] = env['PYTHONPATH']
     for filename in files:
-        p = subprocess.run(('mypy',
-                            '--warn-unused-configs',
-                            '--disallow-subclassing-any',
-                            '--disallow-any-generics',
-                            '--disallow-incomplete-defs',
-                            '--disallow-untyped-decorators',
-                            '--no-implicit-optional',
-                            '--warn-redundant-casts',
-                            '--warn-unused-ignores',
-                            '--no-implicit-reexport',
-                            '--namespace-packages',
-                            filename),
-                           env=env,
-                           check=False,
-                           stdout=subprocess.PIPE,
-                           stderr=subprocess.STDOUT,
-                           universal_newlines=True)
+        p = subprocess.run(
+            (
+                'python3', '-m', 'mypy',
+                '--warn-unused-configs',
+                '--disallow-subclassing-any',
+                '--disallow-any-generics',
+                '--disallow-incomplete-defs',
+                '--disallow-untyped-decorators',
+                '--no-implicit-optional',
+                '--warn-redundant-casts',
+                '--warn-unused-ignores',
+                '--no-implicit-reexport',
+                '--namespace-packages',
+                filename,
+            ),
+            env=env,
+            check=False,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+            universal_newlines=True
+        )
 
         if p.returncode != 0:
             print(p.stdout)
-- 
2.31.1

Re: [PATCH 03/10] iotests/297: Don't rely on distro-specific linter binaries

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> 'pylint-3' is another Fedora-ism. Use "python3 -m pylint" or "python3 -m
> mypy" to access these scripts instead. This style of invocation will
> prefer the "correct" tool when run in a virtual environment.
> 
> Note that we still check for "pylint-3" before the test begins -- this
> check is now "overly strict", but shouldn't cause anything that was
> already running correctly to start failing.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 04/10] 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>
---
 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 43af361622..f35687d021 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -123,8 +123,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

Re: [PATCH 04/10] iotests/297: Create main() function

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, 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>

-- 
Best regards,
Vladimir

[PATCH 05/10] 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 | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index f35687d021..c7428af901 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
 
@@ -64,24 +64,16 @@ def get_test_files(directory: str = '.') -> List[str]:
     ]
 
 
-def run_linters():
-    files = get_test_files()
-
-    iotests.logger.debug('Files to be checked:')
-    iotests.logger.debug(', '.join(sorted(files)))
+def run_linters(
+    files: List[str],
+    env: Optional[Mapping[str, str]] = None,
+) -> None:
 
     print('=== pylint ===')
     sys.stdout.flush()
 
     # Todo notes are fine, but fixme's or xxx's should probably just be
     # fixed (in tests, at least)
-    env = os.environ.copy()
-    qemu_module_path = os.path.join(os.path.dirname(__file__),
-                                    '..', '..', 'python')
-    try:
-        env['PYTHONPATH'] += os.pathsep + qemu_module_path
-    except KeyError:
-        env['PYTHONPATH'] = qemu_module_path
     subprocess.run(
         ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
         env=env,
@@ -95,7 +87,6 @@ def run_linters():
     # will interpret all given files as belonging together (i.e., they
     # may not both define the same classes, etc.; most notably, they
     # must not both define the __main__ module).
-    env['MYPYPATH'] = env['PYTHONPATH']
     for filename in files:
         p = subprocess.run(
             (
@@ -128,7 +119,22 @@ def main() -> None:
         if shutil.which(linter) is None:
             iotests.notrun(f'{linter} not found')
 
-    run_linters()
+    files = get_test_files()
+
+    iotests.logger.debug('Files to be checked:')
+    iotests.logger.debug(', '.join(sorted(files)))
+
+    env = os.environ.copy()
+    qemu_module_path = os.path.join(os.path.dirname(__file__),
+                                    '..', '..', 'python')
+    try:
+        env['PYTHONPATH'] += os.pathsep + qemu_module_path
+    except KeyError:
+        env['PYTHONPATH'] = qemu_module_path
+
+    env['MYPYPATH'] = env['PYTHONPATH']
+
+    run_linters(files, env=env)
 
 
 iotests.script_main(main)
-- 
2.31.1

Re: [PATCH 05/10] iotests/297: Separate environment setup from test execution

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> Move environment setup into main(), leaving pure test execution behind
> in run_linters().
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 06/10] iotests/297: Add 'directory' argument to run_linters

[John Snow]

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

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index c7428af901..1e8334d1d4 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -66,6 +66,7 @@ def get_test_files(directory: str = '.') -> List[str]:
 
 def run_linters(
     files: List[str],
+    directory: str = '.',
     env: Optional[Mapping[str, str]] = None,
 ) -> None:
 
@@ -76,6 +77,7 @@ def run_linters(
     # fixed (in tests, at least)
     subprocess.run(
         ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        cwd=directory,
         env=env,
         check=False,
     )
@@ -103,6 +105,7 @@ def run_linters(
                 '--namespace-packages',
                 filename,
             ),
+            cwd=directory,
             env=env,
             check=False,
             stdout=subprocess.PIPE,
-- 
2.31.1

Re: [PATCH 06/10] iotests/297: Add 'directory' argument to run_linters

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> Allow run_linters to work well if it's executed from a different
> directory.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 07/10] iotests/297: return error code from run_linters()

[John Snow]

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

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

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

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

Re: [PATCH 07/10] iotests/297: return error code from run_linters()

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> This turns run_linters() into a bit of a hybrid test; returning non-zero
> on failed execution while also printing diffable information. This is
> done for the benefit of the avocado simple test runner, which will soon
> be attempting to execute this test from a different environment.
> 
> (Note: universal_newlines is added to the pylint invocation for type
> consistency with the mypy run -- it's not strictly necessary, but it
> avoids some typing errors caused by our re-use of the 'p' variable.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 1e8334d1d4..7db1f9ed45 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -68,19 +68,22 @@ def run_linters(
>       files: List[str],
>       directory: str = '.',
>       env: Optional[Mapping[str, str]] = None,
> -) -> None:
> +) -> int:
> +    ret = 0
>   
>       print('=== pylint ===')
>       sys.stdout.flush()
>   
>       # Todo notes are fine, but fixme's or xxx's should probably just be
>       # fixed (in tests, at least)
> -    subprocess.run(
> +    p = subprocess.run(
>           ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
>           cwd=directory,
>           env=env,
>           check=False,
> +        universal_newlines=True,
>       )
> +    ret += p.returncode
>   
>       print('=== mypy ===')
>       sys.stdout.flush()
> @@ -113,9 +116,12 @@ def run_linters(
>               universal_newlines=True
>           )
>   
> +        ret += p.returncode
>           if p.returncode != 0:
>               print(p.stdout)
>   
> +    return ret
> +
>   
>   def main() -> None:
>       for linter in ('pylint-3', 'mypy'):
> 

Hmm..

1. Rather unusual for a function in python to return int error-code, more usual is raising exceptions..

2. making a sum of return codes looks odd to me

3. Do we really want to run mypy if pylint failed? Maybe better not doing it, and just switch s/check=False/check=True/ ? This way:

3.1 the function becomes native wrapper for subprocess.run, and raise same exceptions
3.2 we don't waste CI time by running mypy when pylint failed anyway


-- 
Best regards,
Vladimir

Re: [PATCH 07/10] iotests/297: return error code from run_linters()

[John Snow]

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

On Tue, Jul 6, 2021 at 5:49 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 25.06.2021 21:20, John Snow wrote:
> > This turns run_linters() into a bit of a hybrid test; returning non-zero
> > on failed execution while also printing diffable information. This is
> > done for the benefit of the avocado simple test runner, which will soon
> > be attempting to execute this test from a different environment.
> >
> > (Note: universal_newlines is added to the pylint invocation for type
> > consistency with the mypy run -- it's not strictly necessary, but it
> > avoids some typing errors caused by our re-use of the 'p' variable.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297 | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 1e8334d1d4..7db1f9ed45 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -68,19 +68,22 @@ def run_linters(
> >       files: List[str],
> >       directory: str = '.',
> >       env: Optional[Mapping[str, str]] = None,
> > -) -> None:
> > +) -> int:
> > +    ret = 0
> >
> >       print('=== pylint ===')
> >       sys.stdout.flush()
> >
> >       # Todo notes are fine, but fixme's or xxx's should probably just be
> >       # fixed (in tests, at least)
> > -    subprocess.run(
> > +    p = subprocess.run(
> >           ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX',
> *files),
> >           cwd=directory,
> >           env=env,
> >           check=False,
> > +        universal_newlines=True,
> >       )
> > +    ret += p.returncode
> >
> >       print('=== mypy ===')
> >       sys.stdout.flush()
> > @@ -113,9 +116,12 @@ def run_linters(
> >               universal_newlines=True
> >           )
> >
> > +        ret += p.returncode
> >           if p.returncode != 0:
> >               print(p.stdout)
> >
> > +    return ret
> > +
> >
> >   def main() -> None:
> >       for linter in ('pylint-3', 'mypy'):
> >
>
> Hmm..
>
> 1. Rather unusual for a function in python to return int error-code, more
> usual is raising exceptions..
>
>
It is strange, but I felt that if these tests were going to run in "two
contexts" that I would avoid raising Exceptions and trying to understand
how it would affect either call stack.


> 2. making a sum of return codes looks odd to me
>
>
Just a cheap way to state that a 0 return is good, and a non-zero return
code is failure.


> 3. Do we really want to run mypy if pylint failed? Maybe better not doing
> it, and just switch s/check=False/check=True/ ? This way:
>
>
I suppose we could. For the sake of CI, I like seeing more output instead
of less so that you can save yourself the trouble and fix everything before
re-submitting the CI job. What do you think?


> 3.1 the function becomes native wrapper for subprocess.run, and raise same
> exceptions
> 3.2 we don't waste CI time by running mypy when pylint failed anyway
>
>
> --
> Best regards,
> Vladimir
>
>

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

Re: [PATCH 07/10] iotests/297: return error code from run_linters()

[Vladimir Sementsov-Ogievskiy]

13.07.2021 02:56, John Snow wrote:
> 
> 
> On Tue, Jul 6, 2021 at 5:49 AM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com <mailto:vsementsov@virtuozzo.com>> wrote:
> 
>     25.06.2021 21:20, John Snow wrote:
>      > This turns run_linters() into a bit of a hybrid test; returning non-zero
>      > on failed execution while also printing diffable information. This is
>      > done for the benefit of the avocado simple test runner, which will soon
>      > be attempting to execute this test from a different environment.
>      >
>      > (Note: universal_newlines is added to the pylint invocation for type
>      > consistency with the mypy run -- it's not strictly necessary, but it
>      > avoids some typing errors caused by our re-use of the 'p' variable.)
>      >
>      > Signed-off-by: John Snow <jsnow@redhat.com <mailto:jsnow@redhat.com>>
>      > ---
>      >   tests/qemu-iotests/297 | 10 ++++++++--
>      >   1 file changed, 8 insertions(+), 2 deletions(-)
>      >
>      > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
>      > index 1e8334d1d4..7db1f9ed45 100755
>      > --- a/tests/qemu-iotests/297
>      > +++ b/tests/qemu-iotests/297
>      > @@ -68,19 +68,22 @@ def run_linters(
>      >       files: List[str],
>      >       directory: str = '.',
>      >       env: Optional[Mapping[str, str]] = None,
>      > -) -> None:
>      > +) -> int:
>      > +    ret = 0
>      >
>      >       print('=== pylint ===')
>      >       sys.stdout.flush()
>      >
>      >       # Todo notes are fine, but fixme's or xxx's should probably just be
>      >       # fixed (in tests, at least)
>      > -    subprocess.run(
>      > +    p = subprocess.run(
>      >           ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
>      >           cwd=directory,
>      >           env=env,
>      >           check=False,
>      > +        universal_newlines=True,
>      >       )
>      > +    ret += p.returncode
>      >
>      >       print('=== mypy ===')
>      >       sys.stdout.flush()
>      > @@ -113,9 +116,12 @@ def run_linters(
>      >               universal_newlines=True
>      >           )
>      >
>      > +        ret += p.returncode
>      >           if p.returncode != 0:
>      >               print(p.stdout)
>      >
>      > +    return ret
>      > +
>      >
>      >   def main() -> None:
>      >       for linter in ('pylint-3', 'mypy'):
>      >
> 
>     Hmm..
> 
>     1. Rather unusual for a function in python to return int error-code, more usual is raising exceptions..
> 
> 
> It is strange, but I felt that if these tests were going to run in "two contexts" that I would avoid raising Exceptions and trying to understand how it would affect either call stack.
> 
>     2. making a sum of return codes looks odd to me
> 
> 
> Just a cheap way to state that a 0 return is good, and a non-zero return code is failure.
> 
>     3. Do we really want to run mypy if pylint failed? Maybe better not doing it, and just switch s/check=False/check=True/ ? This way:
> 
> 
> I suppose we could. For the sake of CI, I like seeing more output instead of less so that you can save yourself the trouble and fix everything before re-submitting the CI job. What do you think?

Hmm, that's reasonable.

> 
>     3.1 the function becomes native wrapper for subprocess.run, and raise same exceptions
>     3.2 we don't waste CI time by running mypy when pylint failed anyway
> 
> 
>     -- 
>     Best regards,
>     Vladimir
> 


-- 
Best regards,
Vladimir

Re: [PATCH 07/10] iotests/297: return error code from run_linters()

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> This turns run_linters() into a bit of a hybrid test; returning non-zero
> on failed execution while also printing diffable information. This is
> done for the benefit of the avocado simple test runner, which will soon
> be attempting to execute this test from a different environment.
> 
> (Note: universal_newlines is added to the pylint invocation for type
> consistency with the mypy run -- it's not strictly necessary, but it
> avoids some typing errors caused by our re-use of the 'p' variable.)
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/297 | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> index 1e8334d1d4..7db1f9ed45 100755
> --- a/tests/qemu-iotests/297
> +++ b/tests/qemu-iotests/297
> @@ -68,19 +68,22 @@ def run_linters(
>       files: List[str],
>       directory: str = '.',
>       env: Optional[Mapping[str, str]] = None,
> -) -> None:
> +) -> int:
> +    ret = 0
>   
>       print('=== pylint ===')
>       sys.stdout.flush()
>   
>       # Todo notes are fine, but fixme's or xxx's should probably just be
>       # fixed (in tests, at least)
> -    subprocess.run(
> +    p = subprocess.run(
>           ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
>           cwd=directory,
>           env=env,
>           check=False,
> +        universal_newlines=True,

Why you need this universal_newlines=True, if you don't handle output?

with or without it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>       )
> +    ret += p.returncode
>   
>       print('=== mypy ===')
>       sys.stdout.flush()
> @@ -113,9 +116,12 @@ def run_linters(
>               universal_newlines=True
>           )
>   
> +        ret += p.returncode
>           if p.returncode != 0:
>               print(p.stdout)
>   
> +    return ret
> +
>   
>   def main() -> None:
>       for linter in ('pylint-3', 'mypy'):
> 


-- 
Best regards,
Vladimir

Re: [PATCH 07/10] iotests/297: return error code from run_linters()

[John Snow]

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

On Tue, Jul 13, 2021 at 5:44 AM Vladimir Sementsov-Ogievskiy <
vsementsov@virtuozzo.com> wrote:

> 25.06.2021 21:20, John Snow wrote:
> > This turns run_linters() into a bit of a hybrid test; returning non-zero
> > on failed execution while also printing diffable information. This is
> > done for the benefit of the avocado simple test runner, which will soon
> > be attempting to execute this test from a different environment.
> >
> > (Note: universal_newlines is added to the pylint invocation for type
> > consistency with the mypy run -- it's not strictly necessary, but it
> > avoids some typing errors caused by our re-use of the 'p' variable.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   tests/qemu-iotests/297 | 10 ++++++++--
> >   1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
> > index 1e8334d1d4..7db1f9ed45 100755
> > --- a/tests/qemu-iotests/297
> > +++ b/tests/qemu-iotests/297
> > @@ -68,19 +68,22 @@ def run_linters(
> >       files: List[str],
> >       directory: str = '.',
> >       env: Optional[Mapping[str, str]] = None,
> > -) -> None:
> > +) -> int:
> > +    ret = 0
> >
> >       print('=== pylint ===')
> >       sys.stdout.flush()
> >
> >       # Todo notes are fine, but fixme's or xxx's should probably just be
> >       # fixed (in tests, at least)
> > -    subprocess.run(
> > +    p = subprocess.run(
> >           ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX',
> *files),
> >           cwd=directory,
> >           env=env,
> >           check=False,
> > +        universal_newlines=True,
>
> Why you need this universal_newlines=True, if you don't handle output?
>
>
Typing consistency. It changes the type of `p`, which we re-use. Could also
be solved by using two different local binding names.

--js


> with or without it:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>
Tentatively, I'll take this RB.


> >       )
> > +    ret += p.returncode
> >
> >       print('=== mypy ===')
> >       sys.stdout.flush()
> > @@ -113,9 +116,12 @@ def run_linters(
> >               universal_newlines=True
> >           )
> >
> > +        ret += p.returncode
> >           if p.returncode != 0:
> >               print(p.stdout)
> >
> > +    return ret
> > +
> >
> >   def main() -> None:
> >       for linter in ('pylint-3', 'mypy'):
> >
>
>
> --
> Best regards,
> Vladimir
>
>

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

[PATCH 08/10] iotests/297: split linters.py off from 297

[John Snow]

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

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

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index 7db1f9ed45..3d29af5b78 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -17,110 +17,13 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import re
 import shutil
-import subprocess
-import sys
-from typing import List, Mapping, Optional
 
 import iotests
+import linters
 
 
-# TODO: Empty this list!
-SKIP_FILES = (
-    '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
-    '096', '118', '124', '132', '136', '139', '147', '148', '149',
-    '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
-    '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
-    '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
-    '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-    '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
-    '299', '302', '303', '304', '307',
-    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
-)
-
-
-def is_python_file(filename: str, directory: str = '.') -> bool:
-    filepath = os.path.join(directory, filename)
-
-    if not os.path.isfile(filepath):
-        return False
-
-    if filename.endswith('.py'):
-        return True
-
-    with open(filepath) as f:
-        try:
-            first_line = f.readline()
-            return re.match('^#!.*python', first_line) is not None
-        except UnicodeDecodeError:  # Ignore binary files
-            return False
-
-
-def get_test_files(directory: str = '.') -> List[str]:
-    return [
-        f for f in (set(os.listdir(directory)) - set(SKIP_FILES))
-        if is_python_file(f, directory)
-    ]
-
-
-def run_linters(
-    files: List[str],
-    directory: str = '.',
-    env: Optional[Mapping[str, str]] = None,
-) -> int:
-    ret = 0
-
-    print('=== pylint ===')
-    sys.stdout.flush()
-
-    # Todo notes are fine, but fixme's or xxx's should probably just be
-    # fixed (in tests, at least)
-    p = subprocess.run(
-        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
-        cwd=directory,
-        env=env,
-        check=False,
-        universal_newlines=True,
-    )
-    ret += p.returncode
-
-    print('=== mypy ===')
-    sys.stdout.flush()
-
-    # We have to call mypy separately for each file.  Otherwise, it
-    # will interpret all given files as belonging together (i.e., they
-    # may not both define the same classes, etc.; most notably, they
-    # must not both define the __main__ module).
-    for filename in files:
-        p = subprocess.run(
-            (
-                'python3', '-m', 'mypy',
-                '--warn-unused-configs',
-                '--disallow-subclassing-any',
-                '--disallow-any-generics',
-                '--disallow-incomplete-defs',
-                '--disallow-untyped-decorators',
-                '--no-implicit-optional',
-                '--warn-redundant-casts',
-                '--warn-unused-ignores',
-                '--no-implicit-reexport',
-                '--namespace-packages',
-                filename,
-            ),
-            cwd=directory,
-            env=env,
-            check=False,
-            stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT,
-            universal_newlines=True
-        )
-
-        ret += p.returncode
-        if p.returncode != 0:
-            print(p.stdout)
-
-    return ret
+# Looking for the list of excluded tests? See linters.py !
 
 
 def main() -> None:
@@ -128,7 +31,7 @@ def main() -> None:
         if shutil.which(linter) is None:
             iotests.notrun(f'{linter} not found')
 
-    files = get_test_files()
+    files = linters.get_test_files()
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
@@ -143,7 +46,7 @@ def main() -> None:
 
     env['MYPYPATH'] = env['PYTHONPATH']
 
-    run_linters(files, env=env)
+    linters.run_linters(files, env=env)
 
 
 iotests.script_main(main)
diff --git a/tests/qemu-iotests/linters.py b/tests/qemu-iotests/linters.py
new file mode 100755
index 0000000000..6fa7ba2d22
--- /dev/null
+++ b/tests/qemu-iotests/linters.py
@@ -0,0 +1,117 @@
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import os
+import re
+import subprocess
+import sys
+from typing import List, Mapping, Optional
+
+
+# TODO: Empty this list!
+SKIP_FILES = (
+    '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
+    '096', '118', '124', '132', '136', '139', '147', '148', '149',
+    '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
+    '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
+    '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
+    '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
+    '262', '264', '266', '274', '277', '280', '281', '295', '296', '298',
+    '299', '302', '303', '304', '307',
+    'nbd-fault-injector.py', 'qcow2.py', 'qcow2_format.py', 'qed.py'
+)
+
+
+def is_python_file(filename: str, directory: str = '.') -> bool:
+    filepath = os.path.join(directory, filename)
+
+    if not os.path.isfile(filepath):
+        return False
+
+    if filename.endswith('.py'):
+        return True
+
+    with open(filepath) as f:
+        try:
+            first_line = f.readline()
+            return re.match('^#!.*python', first_line) is not None
+        except UnicodeDecodeError:  # Ignore binary files
+            return False
+
+
+def get_test_files(directory: str = '.') -> List[str]:
+    return [
+        f for f in (set(os.listdir(directory)) - set(SKIP_FILES))
+        if is_python_file(f, directory)
+    ]
+
+
+def run_linters(
+    files: List[str],
+    directory: str = '.',
+    env: Optional[Mapping[str, str]] = None,
+) -> int:
+    ret = 0
+
+    print('=== pylint ===')
+    sys.stdout.flush()
+
+    # Todo notes are fine, but fixme's or xxx's should probably just be
+    # fixed (in tests, at least)
+    p = subprocess.run(
+        ('python3', '-m', 'pylint', '--score=n', '--notes=FIXME,XXX', *files),
+        cwd=directory,
+        env=env,
+        check=False,
+        universal_newlines=True,
+    )
+    ret += p.returncode
+
+    print('=== mypy ===')
+    sys.stdout.flush()
+
+    # We have to call mypy separately for each file.  Otherwise, it
+    # will interpret all given files as belonging together (i.e., they
+    # may not both define the same classes, etc.; most notably, they
+    # must not both define the __main__ module).
+    for filename in files:
+        p = subprocess.run(
+            (
+                'python3', '-m', 'mypy',
+                '--warn-unused-configs',
+                '--disallow-subclassing-any',
+                '--disallow-any-generics',
+                '--disallow-incomplete-defs',
+                '--disallow-untyped-decorators',
+                '--no-implicit-optional',
+                '--warn-redundant-casts',
+                '--warn-unused-ignores',
+                '--no-implicit-reexport',
+                '--namespace-packages',
+                filename,
+            ),
+            cwd=directory,
+            env=env,
+            check=False,
+            stdout=subprocess.PIPE,
+            stderr=subprocess.STDOUT,
+            universal_newlines=True
+        )
+
+        ret += p.returncode
+        if p.returncode != 0:
+            print(p.stdout)
+
+    return ret
-- 
2.31.1

Re: [PATCH 08/10] iotests/297: split linters.py off from 297

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> Split the linter execution itself out from 297, leaving just environment
> setup in 297. This is done so that non-iotest code can invoke the
> linters without needing to worry about imports of unpackaged iotest
> code.
> 
> Eventually, it should be possible to replace linters.py with mypy.ini
> and pylintrc files that instruct these tools how to run properly in this
> directory, but ... not yet, and not in this series.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

[PATCH 09/10] iotests/linters: Add entry point for Python CI linters

[John Snow]

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

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

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

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

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

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

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

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

Re: [PATCH 09/10] iotests/linters: Add entry point for Python CI linters

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> Add a main() function to linters.py so that the Python CI infrastructure
> has something it can run.
> 
> Now, linters.py represents an invocation of the linting scripts that
> more resembles a "normal" execution of pylint/mypy, like you'd expect to
> use if 'qemu' was a bona-fide package you obtained from PyPI.
> 
> 297, by contrast, now represents the iotests-specific configuration bits
> you need to get it to function correctly as a part of iotests, and with
> 'qemu' as a namespace package that isn't "installed" to the current
> environment, but just lives elsewhere in our source tree.
> 
> By doing this, we will able to run the same linting configuration from
> the Python CI tests without calling iotest logging functions or messing
> around with PYTHONPATH / MYPYPATH.
> 
> iotest 297 continues to operate in a standalone fashion for now --
> presumably, it's convenient for block maintainers and contributors to
> run in this manner.
> 
> See the following commit for how this is used from the Python packaging side.
> 
> Signed-off-by: John Snow<jsnow@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[John Snow]

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

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

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

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

[Vladimir Sementsov-Ogievskiy]

25.06.2021 21:20, John Snow wrote:
> As a convenience, since iotests is an extremely prominent user of the
> qemu.qmp and qemu.machine packages and already implements a linting
> regime, run those tests as well so that it's very hard to miss
> regressions caused by changes to the python library.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   python/tests/iotests.sh | 2 ++
>   1 file changed, 2 insertions(+)
>   create mode 100755 python/tests/iotests.sh
> 
> diff --git a/python/tests/iotests.sh b/python/tests/iotests.sh
> new file mode 100755
> index 0000000000..ec2fc58066
> --- /dev/null
> +++ b/python/tests/iotests.sh
> @@ -0,0 +1,2 @@
> +#!/bin/sh -e
> +PYTHONPATH=../tests/qemu-iotests/ python3 -m linters
> 


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir