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

[PATCH v2 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

v1->v2: patches 1-2 are a rewrite of v1's patch 1
        moved print_env change to patch 4
        do not use argparse.REMAINDER

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         | 15 +++++-
 tests/qemu-iotests/iotests.py    | 78 +++++++++++++++++++-------------
 tests/qemu-iotests/testenv.py    | 22 +++++++--
 tests/qemu-iotests/testrunner.py | 15 +-----
 4 files changed, 81 insertions(+), 49 deletions(-)

-- 
2.30.1

[PATCH v2 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>
---
 tests/qemu-iotests/iotests.py | 68 ++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 28 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 90d0b62523..0521235030 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -32,7 +32,7 @@
 import sys
 import time
 from typing import (Any, Callable, Dict, Iterable,
-                    List, Optional, Sequence, Tuple, TypeVar)
+                    List, Optional, Sequence, Tuple, Type, TypeVar)
 import unittest
 
 from contextlib import contextmanager
@@ -1271,37 +1271,49 @@ 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(object):
+    def __init__(self, stream):
+        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[io.TextIOBase] = None,
+                 resultclass: Type = ReproducibleTestResult, *args, **kwargs):
+        rstream = ReproducibleStreamWrapper(stream or sys.stdout)
+        super().__init__(stream=rstream,           # type: ignore
+                         descriptions=True,
+                         resultclass=resultclass,
+                         *args, **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 v2 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>
---
 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 0521235030..c7915684ba 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1308,12 +1308,16 @@ def __init__(self, stream: Optional[io.TextIOBase] = None,
                          resultclass=resultclass,
                          *args, **kwargs)
 
-def execute_unittest(debug=False):
+def execute_unittest(argv: List[str], debug: bool= False):
     """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 v2 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>
---
 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 1fbec854c1..341a4af4e9 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 v2 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>
---
 tests/qemu-iotests/check         | 15 ++++++++++++++-
 tests/qemu-iotests/testenv.py    |  3 ++-
 tests/qemu-iotests/testrunner.py |  1 -
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..478d74e509 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -19,6 +19,7 @@
 import os
 import sys
 import argparse
+import shutil
 from findtests import TestFinder
 from testenv import TestEnv
 from testrunner import TestRunner
@@ -101,7 +102,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 +115,18 @@ 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 = shutil.which(cmd[0])
+        if exec_path is None:
+            sys.exit('command not found: ' + cmd[0])
+        cmd[0] = exec_path
+        full_env = env.prepare_subprocess(cmd)
+        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 341a4af4e9..6767eeeb25 100644
--- a/tests/qemu-iotests/testenv.py
+++ b/tests/qemu-iotests/testenv.py
@@ -283,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}
+"""
 
         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 v2 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>
---
 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 6767eeeb25..169268f61a 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 v2 1/5] qemu-iotests: do not buffer the test output

[Vladimir Sementsov-Ogievskiy]

23.03.2021 21:19, Paolo Bonzini wrote:
> 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>

some non-critical notes below

> ---
>   tests/qemu-iotests/iotests.py | 68 ++++++++++++++++++++---------------
>   1 file changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 90d0b62523..0521235030 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -32,7 +32,7 @@
>   import sys
>   import time
>   from typing import (Any, Callable, Dict, Iterable,
> -                    List, Optional, Sequence, Tuple, TypeVar)
> +                    List, Optional, Sequence, Tuple, Type, TypeVar)
>   import unittest
>   
>   from contextlib import contextmanager
> @@ -1271,37 +1271,49 @@ def func_wrapper(*args, **kwargs):
>               return func(*args, **kwargs)
>       return func_wrapper
>   

You don't use typing everywhere.. But iotests.py doesn't use it everywhere as well

> +# 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(object):
> +    def __init__(self, stream):
> +        self.stream = stream
> +
> +    def __getattr__(self, attr):
> +        if attr in ('stream', '__getstate__'):

why __getstate__ is here? It's something about pickle..

> +            raise AttributeError(attr)
> +        return getattr(self.stream,attr)

s/,/, /

> +
> +    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[io.TextIOBase] = None,
> +                 resultclass: Type = ReproducibleTestResult, *args, **kwargs):

actually, neither stream nor resultclass arguments are used in the code, so it seems we don't need them?

> +        rstream = ReproducibleStreamWrapper(stream or sys.stdout)
> +        super().__init__(stream=rstream,           # type: ignore
> +                         descriptions=True,
> +                         resultclass=resultclass,
> +                         *args, **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] = (),
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

23.03.2021 21:54, Vladimir Sementsov-Ogievskiy wrote:
>> +class ReproducibleTestRunner(unittest.TextTestRunner):
>> +    def __init__(self, stream: Optional[io.TextIOBase] = None,
>> +                 resultclass: Type = ReproducibleTestResult, *args, **kwargs):
> 
> actually, neither stream nor resultclass arguments are used in the code, so it seems we don't need them?

Ah, we probably should support them to be prepared for the following patch.

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

23.03.2021 21:19, Paolo Bonzini wrote:
> 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>

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

23.03.2021 21:19, 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.

Seems you've moved this fix from one unrelated commit to another.. And it touches two extra files. I'd just make it a separate commit. Nitpicking. Separate or as is:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/qemu-iotests/check         | 15 ++++++++++++++-
>   tests/qemu-iotests/testenv.py    |  3 ++-
>   tests/qemu-iotests/testrunner.py |  1 -
>   3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d1c87ceaf1..478d74e509 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -19,6 +19,7 @@
>   import os
>   import sys
>   import argparse
> +import shutil
>   from findtests import TestFinder
>   from testenv import TestEnv
>   from testrunner import TestRunner
> @@ -101,7 +102,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 +115,18 @@ 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 = shutil.which(cmd[0])
> +        if exec_path is None:
> +            sys.exit('command not found: ' + cmd[0])
> +        cmd[0] = exec_path
> +        full_env = env.prepare_subprocess(cmd)
> +        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 341a4af4e9..6767eeeb25 100644
> --- a/tests/qemu-iotests/testenv.py
> +++ b/tests/qemu-iotests/testenv.py
> @@ -283,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}
> +"""
>   
>           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
>   
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

23.03.2021 21:19, Paolo Bonzini wrote:
> 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>
> ---
>   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 0521235030..c7915684ba 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -1308,12 +1308,16 @@ def __init__(self, stream: Optional[io.TextIOBase] = None,
>                            resultclass=resultclass,
>                            *args, **kwargs)
>   
> -def execute_unittest(debug=False):
> +def execute_unittest(argv: List[str], debug: bool= False):
>       """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()
>   
> 

If you decide to resend for some of my comments (or due to another reviewer be more careful), I think it would be nicer to merge part of this commit which moves us from passing object to passing ReproducibleTestRunner to the previous commit, to not remove line that we've added in the previous commit. And here only add argv argument.


-- 
Best regards,
Vladimir

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

[Paolo Bonzini]

On 23/03/21 20:12, Vladimir Sementsov-Ogievskiy wrote:
>>
>>
>> Move the trailing empty line to print_env(), since it always looks better
>> and one caller was not adding it.
> 
> Seems you've moved this fix from one unrelated commit to another.. And 
> it touches two extra files. I'd just make it a separate commit. 
> Nitpicking. Separate or as is:

Well, now I add the third caller so it's time to make up our mind on the 
trailing line.  A two-line commit didn't seem worth it.

Paolo

> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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

[Paolo Bonzini]

On 23/03/21 20:17, Vladimir Sementsov-Ogievskiy wrote:
>>
>> +    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()
>>
> 
> If you decide to resend for some of my comments (or due to another 
> reviewer be more careful), I think it would be nicer to merge part of 
> this commit which moves us from passing object to passing 
> ReproducibleTestRunner to the previous commit, to not remove line that 
> we've added in the previous commit. And here only add argv argument.

Well, it's the price to pay to make the previous commit as independent 
as possible.  In particular in the previous patch there's no reason to 
add the complexity of warnings.

I could make it three commits, but at some point too much splitting adds 
clutter, the patches are already pretty small.

Paolo

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

[Vladimir Sementsov-Ogievskiy]

24.03.2021 00:22, Paolo Bonzini wrote:
> On 23/03/21 20:17, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>> +    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()
>>>
>>
>> If you decide to resend for some of my comments (or due to another reviewer be more careful), I think it would be nicer to merge part of this commit which moves us from passing object to passing ReproducibleTestRunner to the previous commit, to not remove line that we've added in the previous commit. And here only add argv argument.
> 
> Well, it's the price to pay to make the previous commit as independent as possible.  In particular in the previous patch there's no reason to add the complexity of warnings.
> 
> I could make it three commits, but at some point too much splitting adds clutter, the patches are already pretty small.
> 

OK, they are good as is



-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

24.03.2021 00:20, Paolo Bonzini wrote:
> On 23/03/21 20:12, Vladimir Sementsov-Ogievskiy wrote:
>>>
>>>
>>> Move the trailing empty line to print_env(), since it always looks better
>>> and one caller was not adding it.
>>
>> Seems you've moved this fix from one unrelated commit to another.. And it touches two extra files. I'd just make it a separate commit. Nitpicking. Separate or as is:
> 
> Well, now I add the third caller

Ah yes. So it's related.

> 
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 


-- 
Best regards,
Vladimir

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

[Emanuele Giuseppe Esposito]

On 23/03/2021 19:19, 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
> 
> v1->v2: patches 1-2 are a rewrite of v1's patch 1
>          moved print_env change to patch 4
>          do not use argparse.REMAINDER
> 
> 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         | 15 +++++-
>   tests/qemu-iotests/iotests.py    | 78 +++++++++++++++++++-------------
>   tests/qemu-iotests/testenv.py    | 22 +++++++--
>   tests/qemu-iotests/testrunner.py | 15 +-----
>   4 files changed, 81 insertions(+), 49 deletions(-)
> 

I can confirm that this helps a lot when debugging tests.

Tested-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

[Max Reitz]

On 23.03.21 19:19, 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

I’ve applied an s/,attr/, attr/ to patch 1 (as suggested by Vladimir) 
and an s/debug: bool= False/debug: bool = False/ to patch 2.  I hope 
that’s OK for you.

With that I’ve applied the series to my block branch:

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

Thanks!

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

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

Il gio 25 mar 2021, 19:15 Max Reitz <mreitz@redhat.com> ha scritto:

> I’ve applied an s/,attr/, attr/ to patch 1 (as suggested by Vladimir)
> and an s/debug: bool= False/debug: bool = False/ to patch 2.  I hope
> that’s OK for you.
>
> With that I’ve applied the series to my block branch:
>
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block


Of course, thanks Max!

Paolo

[-- Attachment #2: Type: text/html, Size: 956 bytes --]

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

[Max Reitz]

On 23.03.21 19:19, 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
> 
> v1->v2: patches 1-2 are a rewrite of v1's patch 1
>          moved print_env change to patch 4
>          do not use argparse.REMAINDER

I’m afraid the changes to iotests.py don’t pass the scrutiny of iotest 
297, so I’m going to have to remove the series again. :/


=== pylint ===
************* Module iotests
iotests.py:1288:0: R0205: Class 'ReproducibleStreamWrapper' inherits 
from object, can be safely removed from bases in python3 
(useless-object-inheritance)
iotests.py:1303:4: W1113: Keyword argument before variable positional 
arguments list in the definition of __init__ function 
(keyword-arg-before-vararg)
=== mypy ===
iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments
iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments
iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments
iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments
iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments
iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments
iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments
iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)

iotests.py:1303: error: Function is missing a type annotation for one or 
more arguments
iotests.py:1304: error: Missing type parameters for generic type "Type"
iotests.py:1311: error: Function is missing a return type annotation
Found 3 errors in 1 file (checked 1 source file)