[PATCH v3 0/5] qemu-iotests: quality of life improvements

[PATCH v3 0/5] qemu-iotests: quality of life improvements

[Paolo Bonzini]

This series adds a few usability improvements to qemu-iotests, in
particular:

- arguments can be passed to Python unittests scripts, for example
  to run only a subset of the test cases (patches 1-2)

- it is possible to do "./check -- ../../../tests/qemu-iotests/055 args..."
  and specify arbitrary arguments to be passed to a single test script.
  This allows to take advantage of the previous feature and ease debugging
  of Python tests.

Paolo

v2->v3: fix pylint/mypy [Max]
        fix patch 4 for shell-based tests [Emanuele]

Paolo Bonzini (5):
  qemu-iotests: do not buffer the test output
  qemu-iotests: allow passing unittest.main arguments to the test
    scripts
  qemu-iotests: move command line and environment handling from
    TestRunner to TestEnv
  qemu-iotests: let "check" spawn an arbitrary test command
  qemu-iotests: fix case of SOCK_DIR already in the environment

 tests/qemu-iotests/check         | 18 ++++++-
 tests/qemu-iotests/iotests.py    | 80 +++++++++++++++++++-------------
 tests/qemu-iotests/testenv.py    | 22 +++++++--
 tests/qemu-iotests/testrunner.py | 15 +-----
 4 files changed, 85 insertions(+), 50 deletions(-)

-- 
2.30.1

[PATCH v3 1/5] qemu-iotests: do not buffer the test output

[Paolo Bonzini]

Instead of buffering the test output into a StringIO, patch it on
the fly by wrapping sys.stdout's write method.  This can be
done unconditionally, even if using -d, which makes execute_unittest
a bit simpler.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Message-Id: <20210323181928.311862-2-pbonzini@redhat.com>
---
 tests/qemu-iotests/iotests.py | 70 ++++++++++++++++++++---------------
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 5af0182895..ccf3747ede 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -20,7 +20,6 @@
 import bz2
 from collections import OrderedDict
 import faulthandler
-import io
 import json
 import logging
 import os
@@ -32,7 +31,7 @@
 import sys
 import time
 from typing import (Any, Callable, Dict, Iterable,
-                    List, Optional, Sequence, Tuple, TypeVar)
+                    List, Optional, Sequence, TextIO, Tuple, Type, TypeVar)
 import unittest
 
 from contextlib import contextmanager
@@ -1271,37 +1270,50 @@ def func_wrapper(*args, **kwargs):
             return func(*args, **kwargs)
     return func_wrapper
 
+# We need to filter out the time taken from the output so that
+# qemu-iotest can reliably diff the results against master output,
+# and hide skipped tests from the reference output.
+
+class ReproducibleTestResult(unittest.TextTestResult):
+    def addSkip(self, test, reason):
+        # Same as TextTestResult, but print dot instead of "s"
+        unittest.TestResult.addSkip(self, test, reason)
+        if self.showAll:
+            self.stream.writeln("skipped {0!r}".format(reason))
+        elif self.dots:
+            self.stream.write(".")
+            self.stream.flush()
+
+class ReproducibleStreamWrapper:
+    def __init__(self, stream: TextIO):
+        self.stream = stream
+
+    def __getattr__(self, attr):
+        if attr in ('stream', '__getstate__'):
+            raise AttributeError(attr)
+        return getattr(self.stream, attr)
+
+    def write(self, arg=None):
+        arg = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', arg)
+        arg = re.sub(r' \(skipped=\d+\)', r'', arg)
+        self.stream.write(arg)
+
+class ReproducibleTestRunner(unittest.TextTestRunner):
+    def __init__(self, stream: Optional[TextIO] = None,
+             resultclass: Type[unittest.TestResult] = ReproducibleTestResult,
+             **kwargs: Any) -> None:
+        rstream = ReproducibleStreamWrapper(stream or sys.stdout)
+        super().__init__(stream=rstream,           # type: ignore
+                         descriptions=True,
+                         resultclass=resultclass,
+                         **kwargs)
+
 def execute_unittest(debug=False):
     """Executes unittests within the calling module."""
 
     verbosity = 2 if debug else 1
-
-    if debug:
-        output = sys.stdout
-    else:
-        # We need to filter out the time taken from the output so that
-        # qemu-iotest can reliably diff the results against master output.
-        output = io.StringIO()
-
-    runner = unittest.TextTestRunner(stream=output, descriptions=True,
-                                     verbosity=verbosity)
-    try:
-        # unittest.main() will use sys.exit(); so expect a SystemExit
-        # exception
-        unittest.main(testRunner=runner)
-    finally:
-        # We need to filter out the time taken from the output so that
-        # qemu-iotest can reliably diff the results against master output.
-        if not debug:
-            out = output.getvalue()
-            out = re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', out)
-
-            # Hide skipped tests from the reference output
-            out = re.sub(r'OK \(skipped=\d+\)', 'OK', out)
-            out_first_line, out_rest = out.split('\n', 1)
-            out = out_first_line.replace('s', '.') + '\n' + out_rest
-
-            sys.stderr.write(out)
+    runner = unittest.ReproducibleTestRunner(verbosity=verbosity)
+    unittest.main(testRunner=runner)
 
 def execute_setup_common(supported_fmts: Sequence[str] = (),
                          supported_platforms: Sequence[str] = (),
-- 
2.30.1

[PATCH v3 2/5] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Paolo Bonzini]

Python test scripts that use unittest consist of multiple tests.
unittest.main allows selecting which tests to run, but currently this
is not possible because the iotests wrapper ignores sys.argv.

unittest.main command line options also allow the user to pick the
desired options for verbosity, failfast mode, etc.  While "-d" is
currently translated to "-v", it also enables extra debug output,
and other options are not available at all.

These command line options only work if the unittest.main testRunner
argument is a type, rather than a TestRunner instance.  Therefore, pass
the class name and "verbosity" argument to unittest.main, and adjust for
the different default warnings between TextTestRunner and unittest.main.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Message-Id: <20210323181928.311862-3-pbonzini@redhat.com>
---
 tests/qemu-iotests/iotests.py | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ccf3747ede..5ead94229f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1308,12 +1308,16 @@ def __init__(self, stream: Optional[TextIO] = None,
                          resultclass=resultclass,
                          **kwargs)
 
-def execute_unittest(debug=False):
+def execute_unittest(argv: List[str], debug: bool = False) -> None:
     """Executes unittests within the calling module."""
 
-    verbosity = 2 if debug else 1
-    runner = unittest.ReproducibleTestRunner(verbosity=verbosity)
-    unittest.main(testRunner=runner)
+    # Some tests have warnings, especially ResourceWarnings for unclosed
+    # files and sockets.  Ignore them for now to ensure reproducibility of
+    # the test output.
+    unittest.main(argv=argv,
+                  testRunner=ReproducibleTestRunner,
+                  verbosity=2 if debug else 1,
+                  warnings=None if sys.warnoptions else 'ignore')
 
 def execute_setup_common(supported_fmts: Sequence[str] = (),
                          supported_platforms: Sequence[str] = (),
@@ -1350,7 +1354,7 @@ def execute_test(*args, test_function=None, **kwargs):
 
     debug = execute_setup_common(*args, **kwargs)
     if not test_function:
-        execute_unittest(debug)
+        execute_unittest(sys.argv, debug)
     else:
         test_function()
 
-- 
2.30.1

[PATCH v3 3/5] qemu-iotests: move command line and environment handling from TestRunner to TestEnv

[Paolo Bonzini]

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.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Message-Id: <20210323181928.311862-4-pbonzini@redhat.com>
---
 tests/qemu-iotests/testenv.py    | 17 ++++++++++++++++-
 tests/qemu-iotests/testrunner.py | 14 +-------------
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index 6d27712617..fca3a609e0 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:
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 1fc61fcaa3..519924dc81 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:
-- 
2.30.1

[PATCH v3 4/5] qemu-iotests: let "check" spawn an arbitrary test command

[Paolo Bonzini]

Right now there is no easy way for "check" to print a reproducer command.
Because such a reproducer command line would be huge, we can instead teach
check to start a command of our choice.  This can be for example a Python
unit test with arguments to only run a specific subtest.

Move the trailing empty line to print_env(), since it always looks better
and one caller was not adding it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Message-Id: <20210323181928.311862-5-pbonzini@redhat.com>
---
 tests/qemu-iotests/check         | 18 +++++++++++++++++-
 tests/qemu-iotests/testenv.py    |  3 ++-
 tests/qemu-iotests/testrunner.py |  1 -
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..df9fd733ff 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -19,6 +19,9 @@
 import os
 import sys
 import argparse
+import shutil
+from pathlib import Path
+
 from findtests import TestFinder
 from testenv import TestEnv
 from testrunner import TestRunner
@@ -101,7 +104,7 @@ def make_argparser() -> argparse.ArgumentParser:
                        'rerun failed ./check command, starting from the '
                        'middle of the process.')
     g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
-                       help='tests to run')
+                       help='tests to run, or "--" followed by a command')
 
     return p
 
@@ -114,6 +117,19 @@ if __name__ == '__main__':
                   imgopts=args.imgopts, misalign=args.misalign,
                   debug=args.debug, valgrind=args.valgrind)
 
+    if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
+        if not args.tests:
+            sys.exit("missing command after '--'")
+        cmd = args.tests
+        env.print_env()
+        exec_path = Path(shutil.which(cmd[0]))
+        if exec_path is None:
+            sys.exit('command not found: ' + cmd[0])
+        cmd[0] = exec_path.resolve()
+        full_env = env.prepare_subprocess(cmd)
+        os.chdir(Path(exec_path).parent)
+        os.execve(cmd[0], cmd, full_env)
+
     testfinder = TestFinder(test_dir=env.source_iotests)
 
     groups = args.groups.split(',') if args.groups else None
diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index fca3a609e0..cd0e39b789 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -284,7 +284,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}
+"""
 
         args = collections.defaultdict(str, self.get_env())
 
diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
index 519924dc81..2f56ac545d 100644
--- a/tests/qemu-iotests/testrunner.py
+++ b/tests/qemu-iotests/testrunner.py
@@ -316,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
 
-- 
2.30.1

[PATCH v3 5/5] qemu-iotests: fix case of SOCK_DIR already in the environment

[Paolo Bonzini]

Due to a typo, in this case the SOCK_DIR was not being created.

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Message-Id: <20210323181928.311862-6-pbonzini@redhat.com>
---
 tests/qemu-iotests/testenv.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
index cd0e39b789..0c3fe75636 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -120,7 +120,7 @@ def init_directories(self) -> None:
         try:
             self.sock_dir = os.environ['SOCK_DIR']
             self.tmp_sock_dir = False
-            Path(self.test_dir).mkdir(parents=True, exist_ok=True)
+            Path(self.sock_dir).mkdir(parents=True, exist_ok=True)
         except KeyError:
             self.sock_dir = tempfile.mkdtemp()
             self.tmp_sock_dir = True
-- 
2.30.1

Re: [PATCH v3 4/5] qemu-iotests: let "check" spawn an arbitrary test command

[Max Reitz]

On 26.03.21 15:23, Paolo Bonzini wrote:
> Right now there is no easy way for "check" to print a reproducer command.
> Because such a reproducer command line would be huge, we can instead teach
> check to start a command of our choice.  This can be for example a Python
> unit test with arguments to only run a specific subtest.
> 
> Move the trailing empty line to print_env(), since it always looks better
> and one caller was not adding it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Message-Id: <20210323181928.311862-5-pbonzini@redhat.com>
> ---
>   tests/qemu-iotests/check         | 18 +++++++++++++++++-
>   tests/qemu-iotests/testenv.py    |  3 ++-
>   tests/qemu-iotests/testrunner.py |  1 -
>   3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d1c87ceaf1..df9fd733ff 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -19,6 +19,9 @@
>   import os
>   import sys
>   import argparse
> +import shutil
> +from pathlib import Path
> +
>   from findtests import TestFinder
>   from testenv import TestEnv
>   from testrunner import TestRunner
> @@ -101,7 +104,7 @@ def make_argparser() -> argparse.ArgumentParser:
>                          'rerun failed ./check command, starting from the '
>                          'middle of the process.')
>       g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
> -                       help='tests to run')
> +                       help='tests to run, or "--" followed by a command')
>   
>       return p
>   
> @@ -114,6 +117,19 @@ if __name__ == '__main__':
>                     imgopts=args.imgopts, misalign=args.misalign,
>                     debug=args.debug, valgrind=args.valgrind)
>   
> +    if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
> +        if not args.tests:
> +            sys.exit("missing command after '--'")
> +        cmd = args.tests
> +        env.print_env()
> +        exec_path = Path(shutil.which(cmd[0]))

297 says:

check:125: error: Argument 1 to "Path" has incompatible type 
"Optional[str]"; expected "Union[str, _PathLike[str]]"
Found 1 error in 1 file (checked 1 source file)

Normally I’d assert this away, but actually I think the returned value 
should be checked and we should print an error if it’s None.  (Seems 
like shutil.which() doesn’t raise an exception if there is no such 
command, it just returns None.)

Max

> +        if exec_path is None:
> +            sys.exit('command not found: ' + cmd[0])
> +        cmd[0] = exec_path.resolve()
> +        full_env = env.prepare_subprocess(cmd)
> +        os.chdir(Path(exec_path).parent)
> +        os.execve(cmd[0], cmd, full_env)
> +
>       testfinder = TestFinder(test_dir=env.source_iotests)
>   
>       groups = args.groups.split(',') if args.groups else None
> diff --git a/tests/qemu-iotests/testenv.py b/tests/qemu-iotests/testenv.py
> index fca3a609e0..cd0e39b789 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -284,7 +284,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}
> +"""
>   
>           args = collections.defaultdict(str, self.get_env())
>   
> diff --git a/tests/qemu-iotests/testrunner.py b/tests/qemu-iotests/testrunner.py
> index 519924dc81..2f56ac545d 100644
> --- a/tests/qemu-iotests/testrunner.py
> +++ b/tests/qemu-iotests/testrunner.py
> @@ -316,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
>   
> 

Re: [PATCH v3 4/5] qemu-iotests: let "check" spawn an arbitrary test command

[Max Reitz]

On 26.03.21 16:05, Max Reitz wrote:
> On 26.03.21 15:23, Paolo Bonzini wrote:
>> Right now there is no easy way for "check" to print a reproducer command.
>> Because such a reproducer command line would be huge, we can instead 
>> teach
>> check to start a command of our choice.  This can be for example a Python
>> unit test with arguments to only run a specific subtest.
>>
>> Move the trailing empty line to print_env(), since it always looks better
>> and one caller was not adding it.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> Message-Id: <20210323181928.311862-5-pbonzini@redhat.com>
>> ---
>>   tests/qemu-iotests/check         | 18 +++++++++++++++++-
>>   tests/qemu-iotests/testenv.py    |  3 ++-
>>   tests/qemu-iotests/testrunner.py |  1 -
>>   3 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index d1c87ceaf1..df9fd733ff 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -19,6 +19,9 @@
>>   import os
>>   import sys
>>   import argparse
>> +import shutil
>> +from pathlib import Path
>> +
>>   from findtests import TestFinder
>>   from testenv import TestEnv
>>   from testrunner import TestRunner
>> @@ -101,7 +104,7 @@ def make_argparser() -> argparse.ArgumentParser:
>>                          'rerun failed ./check command, starting from 
>> the '
>>                          'middle of the process.')
>>       g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
>> -                       help='tests to run')
>> +                       help='tests to run, or "--" followed by a 
>> command')
>>       return p
>> @@ -114,6 +117,19 @@ if __name__ == '__main__':
>>                     imgopts=args.imgopts, misalign=args.misalign,
>>                     debug=args.debug, valgrind=args.valgrind)
>> +    if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
>> +        if not args.tests:
>> +            sys.exit("missing command after '--'")
>> +        cmd = args.tests
>> +        env.print_env()
>> +        exec_path = Path(shutil.which(cmd[0]))
> 
> 297 says:
> 
> check:125: error: Argument 1 to "Path" has incompatible type 
> "Optional[str]"; expected "Union[str, _PathLike[str]]"
> Found 1 error in 1 file (checked 1 source file)
> 
> Normally I’d assert this away, but actually I think the returned value 
> should be checked and we should print an error if it’s None.  (Seems 
> like shutil.which() doesn’t raise an exception if there is no such 
> command, it just returns None.)
> 
> Max
> 
>> +        if exec_path is None:
>> +            sys.exit('command not found: ' + cmd[0])

Oh, I see, the intent to print an error is actually there.  The problem 
is just that Path(None) throws an exception, so we must check 
shutil.which()’s return value.

I’ll squash this in if you don’t mind:

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index df9fd733ff..e2230f5612 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -122,9 +122,10 @@ if __name__ == '__main__':
              sys.exit("missing command after '--'")
          cmd = args.tests
          env.print_env()
-        exec_path = Path(shutil.which(cmd[0]))
-        if exec_path is None:
+        exec_pathstr = shutil.which(cmd[0])
+        if exec_pathstr is None:
              sys.exit('command not found: ' + cmd[0])
+        exec_path = Path(exec_pathstr)
          cmd[0] = exec_path.resolve()
          full_env = env.prepare_subprocess(cmd)
          os.chdir(Path(exec_path).parent)

>> +        cmd[0] = exec_path.resolve()
>> +        full_env = env.prepare_subprocess(cmd)
>> +        os.chdir(Path(exec_path).parent)
>> +        os.execve(cmd[0], cmd, full_env)
>> +
>>       testfinder = TestFinder(test_dir=env.source_iotests)
>>       groups = args.groups.split(',') if args.groups else None
>> diff --git a/tests/qemu-iotests/testenv.py 
>> b/tests/qemu-iotests/testenv.py
>> index fca3a609e0..cd0e39b789 100644
>> --- a/tests/qemu-iotests/testenv.py
>> +++ b/tests/qemu-iotests/testenv.py
>> @@ -284,7 +284,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}
>> +"""
>>           args = collections.defaultdict(str, self.get_env())
>> diff --git a/tests/qemu-iotests/testrunner.py 
>> b/tests/qemu-iotests/testrunner.py
>> index 519924dc81..2f56ac545d 100644
>> --- a/tests/qemu-iotests/testrunner.py
>> +++ b/tests/qemu-iotests/testrunner.py
>> @@ -316,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
>>
> 

Re: [PATCH v3 4/5] qemu-iotests: let "check" spawn an arbitrary test command

[Max Reitz]

On 30.03.21 12:38, Max Reitz wrote:
> On 26.03.21 16:05, Max Reitz wrote:
>> On 26.03.21 15:23, Paolo Bonzini wrote:
>>> Right now there is no easy way for "check" to print a reproducer 
>>> command.
>>> Because such a reproducer command line would be huge, we can instead 
>>> teach
>>> check to start a command of our choice.  This can be for example a 
>>> Python
>>> unit test with arguments to only run a specific subtest.
>>>
>>> Move the trailing empty line to print_env(), since it always looks 
>>> better
>>> and one caller was not adding it.
>>>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>> Message-Id: <20210323181928.311862-5-pbonzini@redhat.com>
>>> ---
>>>   tests/qemu-iotests/check         | 18 +++++++++++++++++-
>>>   tests/qemu-iotests/testenv.py    |  3 ++-
>>>   tests/qemu-iotests/testrunner.py |  1 -
>>>   3 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>> index d1c87ceaf1..df9fd733ff 100755
>>> --- a/tests/qemu-iotests/check
>>> +++ b/tests/qemu-iotests/check
>>> @@ -19,6 +19,9 @@
>>>   import os
>>>   import sys
>>>   import argparse
>>> +import shutil
>>> +from pathlib import Path
>>> +
>>>   from findtests import TestFinder
>>>   from testenv import TestEnv
>>>   from testrunner import TestRunner
>>> @@ -101,7 +104,7 @@ def make_argparser() -> argparse.ArgumentParser:
>>>                          'rerun failed ./check command, starting from 
>>> the '
>>>                          'middle of the process.')
>>>       g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
>>> -                       help='tests to run')
>>> +                       help='tests to run, or "--" followed by a 
>>> command')
>>>       return p
>>> @@ -114,6 +117,19 @@ if __name__ == '__main__':
>>>                     imgopts=args.imgopts, misalign=args.misalign,
>>>                     debug=args.debug, valgrind=args.valgrind)
>>> +    if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
>>> +        if not args.tests:
>>> +            sys.exit("missing command after '--'")
>>> +        cmd = args.tests
>>> +        env.print_env()
>>> +        exec_path = Path(shutil.which(cmd[0]))
>>
>> 297 says:
>>
>> check:125: error: Argument 1 to "Path" has incompatible type 
>> "Optional[str]"; expected "Union[str, _PathLike[str]]"
>> Found 1 error in 1 file (checked 1 source file)
>>
>> Normally I’d assert this away, but actually I think the returned value 
>> should be checked and we should print an error if it’s None.  (Seems 
>> like shutil.which() doesn’t raise an exception if there is no such 
>> command, it just returns None.)
>>
>> Max
>>
>>> +        if exec_path is None:
>>> +            sys.exit('command not found: ' + cmd[0])
> 
> Oh, I see, the intent to print an error is actually there.  The problem 
> is just that Path(None) throws an exception, so we must check 
> shutil.which()’s return value.
> 
> I’ll squash this in if you don’t mind:
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index df9fd733ff..e2230f5612 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -122,9 +122,10 @@ if __name__ == '__main__':
>               sys.exit("missing command after '--'")
>           cmd = args.tests
>           env.print_env()
> -        exec_path = Path(shutil.which(cmd[0]))
> -        if exec_path is None:
> +        exec_pathstr = shutil.which(cmd[0])
> +        if exec_pathstr is None:
>               sys.exit('command not found: ' + cmd[0])
> +        exec_path = Path(exec_pathstr)
>           cmd[0] = exec_path.resolve()
>           full_env = env.prepare_subprocess(cmd)
>           os.chdir(Path(exec_path).parent)
> 
>>> +        cmd[0] = exec_path.resolve()
>>> +        full_env = env.prepare_subprocess(cmd)
>>> +        os.chdir(Path(exec_path).parent)

Oh, and this Path() does nothing, I presume, so I’m going to replace it 
with just “exec_path”.

Max

>>> +        os.execve(cmd[0], cmd, full_env)
>>> +
>>>       testfinder = TestFinder(test_dir=env.source_iotests)
>>>       groups = args.groups.split(',') if args.groups else None
>>> diff --git a/tests/qemu-iotests/testenv.py 
>>> b/tests/qemu-iotests/testenv.py
>>> index fca3a609e0..cd0e39b789 100644
>>> --- a/tests/qemu-iotests/testenv.py
>>> +++ b/tests/qemu-iotests/testenv.py
>>> @@ -284,7 +284,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}
>>> +"""
>>>           args = collections.defaultdict(str, self.get_env())
>>> diff --git a/tests/qemu-iotests/testrunner.py 
>>> b/tests/qemu-iotests/testrunner.py
>>> index 519924dc81..2f56ac545d 100644
>>> --- a/tests/qemu-iotests/testrunner.py
>>> +++ b/tests/qemu-iotests/testrunner.py
>>> @@ -316,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
>>>
>>
> 

Re: [PATCH v3 4/5] qemu-iotests: let "check" spawn an arbitrary test command

[Max Reitz]

On 30.03.21 12:44, Max Reitz wrote:
> On 30.03.21 12:38, Max Reitz wrote:
>> On 26.03.21 16:05, Max Reitz wrote:
>>> On 26.03.21 15:23, Paolo Bonzini wrote:
>>>> Right now there is no easy way for "check" to print a reproducer 
>>>> command.
>>>> Because such a reproducer command line would be huge, we can instead 
>>>> teach
>>>> check to start a command of our choice.  This can be for example a 
>>>> Python
>>>> unit test with arguments to only run a specific subtest.
>>>>
>>>> Move the trailing empty line to print_env(), since it always looks 
>>>> better
>>>> and one caller was not adding it.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>>>> Message-Id: <20210323181928.311862-5-pbonzini@redhat.com>
>>>> ---
>>>>   tests/qemu-iotests/check         | 18 +++++++++++++++++-
>>>>   tests/qemu-iotests/testenv.py    |  3 ++-
>>>>   tests/qemu-iotests/testrunner.py |  1 -
>>>>   3 files changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>>>> index d1c87ceaf1..df9fd733ff 100755
>>>> --- a/tests/qemu-iotests/check
>>>> +++ b/tests/qemu-iotests/check
>>>> @@ -19,6 +19,9 @@
>>>>   import os
>>>>   import sys
>>>>   import argparse
>>>> +import shutil
>>>> +from pathlib import Path
>>>> +
>>>>   from findtests import TestFinder
>>>>   from testenv import TestEnv
>>>>   from testrunner import TestRunner
>>>> @@ -101,7 +104,7 @@ def make_argparser() -> argparse.ArgumentParser:
>>>>                          'rerun failed ./check command, starting 
>>>> from the '
>>>>                          'middle of the process.')
>>>>       g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
>>>> -                       help='tests to run')
>>>> +                       help='tests to run, or "--" followed by a 
>>>> command')
>>>>       return p
>>>> @@ -114,6 +117,19 @@ if __name__ == '__main__':
>>>>                     imgopts=args.imgopts, misalign=args.misalign,
>>>>                     debug=args.debug, valgrind=args.valgrind)
>>>> +    if len(sys.argv) > 1 and sys.argv[-len(args.tests)-1] == '--':
>>>> +        if not args.tests:
>>>> +            sys.exit("missing command after '--'")
>>>> +        cmd = args.tests
>>>> +        env.print_env()
>>>> +        exec_path = Path(shutil.which(cmd[0]))
>>>
>>> 297 says:
>>>
>>> check:125: error: Argument 1 to "Path" has incompatible type 
>>> "Optional[str]"; expected "Union[str, _PathLike[str]]"
>>> Found 1 error in 1 file (checked 1 source file)
>>>
>>> Normally I’d assert this away, but actually I think the returned 
>>> value should be checked and we should print an error if it’s None.  
>>> (Seems like shutil.which() doesn’t raise an exception if there is no 
>>> such command, it just returns None.)
>>>
>>> Max
>>>
>>>> +        if exec_path is None:
>>>> +            sys.exit('command not found: ' + cmd[0])
>>
>> Oh, I see, the intent to print an error is actually there.  The 
>> problem is just that Path(None) throws an exception, so we must check 
>> shutil.which()’s return value.
>>
>> I’ll squash this in if you don’t mind:
>>
>> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
>> index df9fd733ff..e2230f5612 100755
>> --- a/tests/qemu-iotests/check
>> +++ b/tests/qemu-iotests/check
>> @@ -122,9 +122,10 @@ if __name__ == '__main__':
>>               sys.exit("missing command after '--'")
>>           cmd = args.tests
>>           env.print_env()
>> -        exec_path = Path(shutil.which(cmd[0]))
>> -        if exec_path is None:
>> +        exec_pathstr = shutil.which(cmd[0])
>> +        if exec_pathstr is None:
>>               sys.exit('command not found: ' + cmd[0])
>> +        exec_path = Path(exec_pathstr)
>>           cmd[0] = exec_path.resolve()
>>           full_env = env.prepare_subprocess(cmd)
>>           os.chdir(Path(exec_path).parent)
>>
>>>> +        cmd[0] = exec_path.resolve()
>>>> +        full_env = env.prepare_subprocess(cmd)
>>>> +        os.chdir(Path(exec_path).parent)
> 
> Oh, and this Path() does nothing, I presume, so I’m going to replace it 
> with just “exec_path”.

On third thought, the pathlib doc says:

 > If you want to walk an arbitrary filesystem path upwards, it is
 > recommended to first call Path.resolve() so as to resolve symlinks and
 > eliminate “..” components.

So I guess the best would be to make it “exec_path = 
Path(exec_pathstr).resolve()”.

I’d also prefer if cmd[0] was a string and not a Path object 
(Path.resolve returns a Path).  os.execve() can work with Path objects 
as of 3.6 (which is the minimum version we require), but 
prepare_subprocess() expects a list of strings.  (I don’t know why mypy 
doesn’t complain.  I presume it just can’t resolve cmd's type.)

So here’s the full diff:

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index df9fd733ff..7c9d3a0852 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -122,12 +122,13 @@ if __name__ == '__main__':
              sys.exit("missing command after '--'")
          cmd = args.tests
          env.print_env()
-        exec_path = Path(shutil.which(cmd[0]))
-        if exec_path is None:
+        exec_pathstr = shutil.which(cmd[0])
+        if exec_pathstr is None:
              sys.exit('command not found: ' + cmd[0])
-        cmd[0] = exec_path.resolve()
+        exec_path = Path(exec_pathstr).resolve()
+        cmd[0] = str(exec_path)
          full_env = env.prepare_subprocess(cmd)
-        os.chdir(Path(exec_path).parent)
+        os.chdir(exec_path.parent)
          os.execve(cmd[0], cmd, full_env)

      testfinder = TestFinder(test_dir=env.source_iotests)


But now these are so many changes that I feel uncomfortable making this 
change myself.  This series only affects the iotests, so AFAIU we are in 
no hurry to get this into rc1, and it can still go into rc2.

Max

Re: [PATCH v3 4/5] qemu-iotests: let "check" spawn an arbitrary test command

[Paolo Bonzini]

On 30/03/21 12:57, Max Reitz wrote:
> 
> 297 says:
> 
> check:125: error: Argument 1 to "Path" has incompatible type "Optional[str]"; expected "Union[str, _PathLike[str]]"
> Found 1 error in 1 file (checked 1 source file)

Weird, I had tested this and I cannot reproduce it.

> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index df9fd733ff..7c9d3a0852 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -122,12 +122,13 @@ if __name__ == '__main__':
>               sys.exit("missing command after '--'")
>           cmd = args.tests
>           env.print_env()
> -        exec_path = Path(shutil.which(cmd[0]))
> -        if exec_path is None:
> +        exec_pathstr = shutil.which(cmd[0])
> +        if exec_pathstr is None:
>               sys.exit('command not found: ' + cmd[0])
> -        cmd[0] = exec_path.resolve()
> +        exec_path = Path(exec_pathstr).resolve()
> +        cmd[0] = str(exec_path)
>           full_env = env.prepare_subprocess(cmd)
> -        os.chdir(Path(exec_path).parent)
> +        os.chdir(exec_path.parent)
>           os.execve(cmd[0], cmd, full_env)
> 
>       testfinder = TestFinder(test_dir=env.source_iotests)
> 
> 
> But now these are so many changes that I feel uncomfortable making this 
> change myself.  This series only affects the iotests, so AFAIU we are in 
> no hurry to get this into rc1, and it can still go into rc2.

Go ahead and squash it.

Technically I think resolve() is not needed because we're basically just 
doing "dirname" and not going upwards in the directory tree.  That would 
leave the smaller change in message id 
51523e26-a184-9434-cb60-277c7b3c67d6@redhat.com.  However, it doesn't 
hurt either and others may have the same doubt as you.

Thanks Max!

Paolo

Re: [PATCH v3 0/5] qemu-iotests: quality of life improvements

[Max Reitz]

On 26.03.21 15:23, Paolo Bonzini wrote:
> This series adds a few usability improvements to qemu-iotests, in
> particular:
> 
> - arguments can be passed to Python unittests scripts, for example
>    to run only a subset of the test cases (patches 1-2)
> 
> - it is possible to do "./check -- ../../../tests/qemu-iotests/055 args..."
>    and specify arbitrary arguments to be passed to a single test script.
>    This allows to take advantage of the previous feature and ease debugging
>    of Python tests.
> 
> Paolo
Thanks, I’ve amended patch 4 and applied the series to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max

Re: [PATCH v3 0/5] qemu-iotests: quality of life improvements

[Max Reitz]

On 30.03.21 13:32, Max Reitz wrote:
> On 26.03.21 15:23, Paolo Bonzini wrote:
>> This series adds a few usability improvements to qemu-iotests, in
>> particular:
>>
>> - arguments can be passed to Python unittests scripts, for example
>>    to run only a subset of the test cases (patches 1-2)
>>
>> - it is possible to do "./check -- ../../../tests/qemu-iotests/055 
>> args..."
>>    and specify arbitrary arguments to be passed to a single test script.
>>    This allows to take advantage of the previous feature and ease 
>> debugging
>>    of Python tests.
>>
>> Paolo
> Thanks, I’ve amended patch 4 and applied the series to my block branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block

I’m sorry but I’ll have to drop it again.  At least iotests 245 und 295 
fail; I assume it has something to do with `iotests.activate_logging()`.

I don’t think that’s something that we’ll fix today, so I think we 
should postpone this series to rc2 after all.

Max

Re: [PATCH v3 0/5] qemu-iotests: quality of life improvements

[Paolo Bonzini]

On 30/03/21 13:44, Max Reitz wrote:
> On 30.03.21 13:32, Max Reitz wrote:
>> On 26.03.21 15:23, Paolo Bonzini wrote:
>>> This series adds a few usability improvements to qemu-iotests, in
>>> particular:
>>>
>>> - arguments can be passed to Python unittests scripts, for example
>>>    to run only a subset of the test cases (patches 1-2)
>>>
>>> - it is possible to do "./check -- ../../../tests/qemu-iotests/055 
>>> args..."
>>>    and specify arbitrary arguments to be passed to a single test script.
>>>    This allows to take advantage of the previous feature and ease 
>>> debugging
>>>    of Python tests.
>>>
>>> Paolo
>> Thanks, I’ve amended patch 4 and applied the series to my block branch:
>>
>> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 
> I’m sorry but I’ll have to drop it again.  At least iotests 245 und 295 
> fail; I assume it has something to do with `iotests.activate_logging()`.

Ok, will look into it.  Can you give me the exact set of ./check 
invocations that you use?

Paolo