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