On 16.09.21 06:09, 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 <js...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
tests/qemu-iotests/297 | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
I don’t think I like this very much. Returning an integer error code
seems archaic.
(You can perhaps already see that this is going to be one of these
reviews of mine where I won’t say anything is really wrong, but where I
will just lament subjectively missing beauty.)
As you say, run_linters() to me seems very iotests-specific still: It
emits a specific output that is compared against a reference output.
Fine for 297, but not fine for a function provided by a linters.py, I’d say.
I’d prefer run_linters() to return something like a Map[str,
Optional[str]], that would map a tool to its output in case of error,
i.e. ideally:
`{'pylint': None, 'mypy': None}`
297 could format it something like
```
for tool, output in run_linters().items():
print(f'=== {tool} ===')
if output is not None:
print(output)
```
Or something.
To check for error, you could put a Python script in python/tests that
checks `any(output is not None for output in run_linters().values())` or
something (and on error print the output).
Pulling out run_linters() into an external file and having it print
something to stdout just seems too iotests-centric to me. I suppose as
long as the return code is right (which this patch is for) it should
work for Avocado’s simple tests, too (which I don’t like very much
either, by the way, because they too seem archaic to me), but, well. It
almost seems like the Avocado test should just run ./check then.
Come to think of it, to be absolutely blasphemous, why not. I could say
all of this seems like quite some work that could be done by a
python/tests script that does this:
```
#!/bin/sh
set -e
cat >/tmp/qemu-parrot.sh <<EOF
#!/bin/sh
echo ': qcow2'
echo ': qcow2'
echo 'virtio-blk'
EOF
cd $QEMU_DIR/tests/qemu-iotests
QEMU_PROG="/tmp/qemu-parrot.sh" \
QEMU_IMG_PROG=$(which false) \
QEMU_IO_PROG=$(which false) \
QEMU_NBD_PROG=$(which false) \
QSD_PROG=$(which false) \
./check 297
```
And, no, I don’t want that! But the point of this series seems to just
be to rip the core of 297 out so it can run without ./check, because
./check requires some environment variables to be set. Doing that seems
just seems wrong to me.
Like, can’t we have a Python script in python/tests that imports
linters.py, invokes run_linters() and sensibly checks the output? Or,
you know, at the very least not have run_linters() print anything to
stdout and not have it return an integer code. linters.py:main() can do
that conversion.
Or, something completely different, perhaps my problem is that you put
linters.py as a fully standalone test into the iotests directory,
without it being an iotest. So, I think I could also agree on putting
linters.py into python/tests, and then having 297 execute that. Or you
know, we just drop 297 altogether, as you suggest in patch 13 – if
that’s what it takes, then so be it.
Hanna
PS: Also, summing up processes’ return codes makes me feel not good.
diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index e05c99972e..f9ddfb53a0 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'):
