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.)
Haha. It's fine, Vladimir didn't like the smell of this one either. I just didn't want to prematurely optimize or overcomplicate.
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}`
Splitting the test apart into two sub-tests is completely reasonable. Python CI right now has individual tests for pylint, mypy, etc.
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.
Yeh. Ideally, we'd just have a mypy.ini and a pylintrc that configures the tests adequately, and then 297 (or whomever else) would just call the linters which would in turn read the same configuration. This series is somewhat of a stop-gap to measure the temperature of the room to see how important it was to leave 297 inside of iotests. So, I did the conservative thing that's faster to review even if it now looks *slightly* fishy.
As for things being archaic or not ... possibly, but why involve extra complexity if it isn't warranted? a simple pass/fail works perfectly well. (And the human can read the output to understand WHY it failed.) If you want more rigorous analytics for some reason, we can discuss the use cases and figure out how to represent that information, but that's definitely a bit beyond scope here.
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.
Right, the point of this series is effectively to split out the linting configuration and separate it from the "test" which executes the linters with that configuration. Simplest possible thing was to just take the configuration as it exists in its current form -- hardcoded in a python script -- and isolate it. To my delight, it worked quite well!
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.
Well, I certainly don't want to bother parsing output from python tools and trying to make sure that it works sensibly cross-version and cross-distro. The return code being 0/non-zero is vastly simpler. Let the human read the output log on failure cases to get a sense of what exactly went wrong. Is there some reason why parsing the output would be beneficial to anyone?
(The Python GitLab CI hooks don't even bother printing output to the console unless it returns non-zero, and then it will just print whatever it saw. Seems to work well in practice.)
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.
I can definitely drop 297 if you'd like, and work on making the linter configuration more declarative. I erred on the side of less movement instead of more so that disruption would be minimal. It might take me some extra time to work out how to un-scriptify the test, though. I'd like to get a quick temperature check from kwolf on this before I put the work in.
Hanna
PS: Also, summing up processes’ return codes makes me feel not good.
That's the part Vladimir didn't like. There was no real reason for it, other than it was "easy". I can make it a binary 0/1 return instead, if that'd grease the wheels.
(I'll sit on respinning the series for now until we've had some time to discuss it. I would rather like a chance to involve kwolf as the other major user of this subsystem.)
> 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'):