Re: [PATCH] qemu-iotests: fix pylint 2.8 consider-using-with error

[Max Reitz]

On 06.05.21 10:48, Emanuele Giuseppe Esposito wrote:
> pylint 2.8 introduces consider-using-with error, suggesting
> to use the 'with' block statement when possible.
> http://pylint.pycqa.org/en/latest/whatsnew/2.8.html
>
> Modify all subprocess.Popen calls to use the 'with' statement,
> except one in __init__ of QemuIoInteractive class, since
> the return value is assigned to a class field and used in other methods.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py    | 63 ++++++++++++++++----------------
>   tests/qemu-iotests/testrunner.py | 22 +++++------
>   2 files changed, 42 insertions(+), 43 deletions(-)

Thanks, looks good, functionally.  But I just can’t keep myself from 
nagging about indentation (I wouldn’t have, but flake8 says I may be 
justified):

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 5af0182895..e1117e8ae8 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -113,15 +113,14 @@ def qemu_tool_pipe_and_status(tool: str, args: 
> Sequence[str],
>       Run a tool and return both its output and its exit code
>       """
>       stderr = subprocess.STDOUT if connect_stderr else None
> -    subp = subprocess.Popen(args,
> -                            stdout=subprocess.PIPE,
> -                            stderr=stderr,
> -                            universal_newlines=True)
> -    output = subp.communicate()[0]
> -    if subp.returncode < 0:
> -        cmd = ' '.join(args)
> -        sys.stderr.write(f'{tool} received signal {-subp.returncode}: {cmd}\n')
> -    return (output, subp.returncode)
> +    with subprocess.Popen(args, stdout=subprocess.PIPE,
> +                        stderr=stderr, universal_newlines=True) as subp:

The second line’s indentation is not aligned to the opening parenthesis 
of the Popen call.  I’d like it better if it were.

> +        output = subp.communicate()[0]
> +        if subp.returncode < 0:
> +            cmd = ' '.join(args)
> +            sys.stderr.write(f'{tool} received signal \
> +                               {-subp.returncode}: {cmd}\n')
> +        return (output, subp.returncode)
>   
>   def qemu_img_pipe_and_status(*args: str) -> Tuple[str, int]:
>       """
> @@ -237,6 +236,7 @@ def qemu_io_silent_check(*args):
>   class QemuIoInteractive:
>       def __init__(self, *args):
>           self.args = qemu_io_args_no_fmt + list(args)
> +        # pylint: disable=consider-using-with

I think I would have added an additional comment why we need this 
statement (because we need to keep the Popen object around), but then 
again, I suppose it really is obvious.

(Wouldn’t have commented on this bit if I hadn’t replied because of the 
indentation.  So if you think it’s obvious, no need to add a comment.)

>           self._p = subprocess.Popen(self.args, stdin=subprocess.PIPE,
>                                      stdout=subprocess.PIPE,
>                                      stderr=subprocess.STDOUT,

[...]

> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> index 1fc61fcaa3..729fe9ee3b 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -258,17 +258,17 @@ def do_run_test(self, test: str) -> TestResult:
>   
>           t0 = time.time()
>           with f_bad.open('w', encoding="utf-8") as f:
> -            proc = subprocess.Popen(args, cwd=str(f_test.parent), env=env,
> -                                    stdout=f, stderr=subprocess.STDOUT)
> -            try:
> -                proc.wait()
> -            except KeyboardInterrupt:
> -                proc.terminate()
> -                proc.wait()
> -                return TestResult(status='not run',
> -                                  description='Interrupted by user',
> -                                  interrupted=True)
> -            ret = proc.returncode
> +            with subprocess.Popen(args, cwd=str(f_test.parent), env=env,
> +                                stdout=f, stderr=subprocess.STDOUT) as proc:

As above, the second line is (no longer) aligned to the opening parenthesis.

> +                try:
> +                    proc.wait()
> +                except KeyboardInterrupt:
> +                    proc.terminate()
> +                    proc.wait()
> +                    return TestResult(status='not run',
> +                                    description='Interrupted by user',
> +                                    interrupted=True)

And here, too.

Max

> +                ret = proc.returncode
>   
>           elapsed = round(time.time() - t0, 1)
>