Re: [PATCH 2/4] qemu-iotests: move command line and environment handling from TestRunner to TestEnv

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

23.03.2021 16:06, Paolo Bonzini wrote:
> In the next patch, "check" will learn how to execute a test script without
> going through TestRunner.  To enable this, keep only the text output
> and subprocess handling in the TestRunner; move into TestEnv the logic
> to prepare for running a subprocess.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/qemu-iotests/testenv.py    | 20 ++++++++++++++++++--
>   tests/qemu-iotests/testrunner.py | 15 +--------------
>   2 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index 1fbec854c1..6767eeeb25 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -25,7 +25,7 @@
>   import random
>   import subprocess
>   import glob
> -from typing import Dict, Any, Optional, ContextManager
> +from typing import List, Dict, Any, Optional, ContextManager
>   
>   
>   def isxfile(path: str) -> bool:
> @@ -74,6 +74,21 @@ class TestEnv(ContextManager['TestEnv']):
>                        'CACHEMODE_IS_DEFAULT', 'IMGFMT_GENERIC', 'IMGOPTSSYNTAX',
>                        'IMGKEYSECRET', 'QEMU_DEFAULT_MACHINE', 'MALLOC_PERTURB_']
>   
> +    def prepare_subprocess(self, args: List[str]) -> Dict[str, str]:
> +        if self.debug:
> +            args.append('-d')
> +
> +        with open(args[0], encoding="utf-8") as f:
> +            try:
> +                if f.readline().rstrip() == '#!/usr/bin/env python3':
> +                    args.insert(0, self.python)
> +            except UnicodeDecodeError:  # binary test? for future.
> +                pass
> +
> +        os_env = os.environ.copy()
> +        os_env.update(self.get_env())
> +        return os_env
> +
>       def get_env(self) -> Dict[str, str]:
>           env = {}
>           for v in self.env_variables:
> @@ -268,7 +283,8 @@ def print_env(self) -> None:
>   PLATFORM      -- {platform}
>   TEST_DIR      -- {TEST_DIR}
>   SOCK_DIR      -- {SOCK_DIR}
> -SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}"""
> +SOCKET_SCM_HELPER -- {SOCKET_SCM_HELPER}
> +"""

Unrelated change.. Better be in another commit or at least noted in commit msg.

You also updated only one of two callers, so output will change after this patch. Seems it should become better..

>   
>           args = collections.defaultdict(str, self.get_env())
>   
> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> index 1fc61fcaa3..2f56ac545d 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -129,7 +129,6 @@ class TestRunner(ContextManager['TestRunner']):
>       def __init__(self, env: TestEnv, makecheck: bool = False,
>                    color: str = 'auto') -> None:
>           self.env = env
> -        self.test_run_env = self.env.get_env()
>           self.makecheck = makecheck
>           self.last_elapsed = LastElapsedTime('.last-elapsed-cache', env)
>   
> @@ -243,18 +242,7 @@ def do_run_test(self, test: str) -> TestResult:
>               silent_unlink(p)
>   
>           args = [str(f_test.resolve())]
> -        if self.env.debug:
> -            args.append('-d')
> -
> -        with f_test.open(encoding="utf-8") as f:
> -            try:
> -                if f.readline().rstrip() == '#!/usr/bin/env python3':
> -                    args.insert(0, self.env.python)
> -            except UnicodeDecodeError:  # binary test? for future.
> -                pass
> -
> -        env = os.environ.copy()
> -        env.update(self.test_run_env)
> +        env = self.env.prepare_subprocess(args)
>   
>           t0 = time.time()
>           with f_bad.open('w', encoding="utf-8") as f:
> @@ -328,7 +316,6 @@ def run_tests(self, tests: List[str]) -> bool:
>   
>           if not self.makecheck:
>               self.env.print_env()
> -            print()
>   
>           test_field_width = max(len(os.path.basename(t)) for t in tests) + 2
>   
> 

Better without changing empty line, but still I'm OK with the patch as is:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir