Re: [PATCH v3 11/16] iotests/297: return error code from run_linters()

[Hanna Reitz <hreitz@redhat.com>]

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 <jsnow@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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'):