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

[PATCH 0/4] 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 (patch 1)

- 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

Paolo Bonzini (4):
  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         | 16 +++++++--
 tests/qemu-iotests/iotests.py    | 60 +++++++++++++++++++-------------
 tests/qemu-iotests/testenv.py    | 22 ++++++++++--
 tests/qemu-iotests/testrunner.py | 15 +-------
 4 files changed, 69 insertions(+), 44 deletions(-)

-- 
2.30.1

[PATCH 1/4] 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 also there are no other options available.  Unfortunately, these
command line options only work if the unittest.main testRunner argument
is a type, rather than a TestRunner instance, and right now iotests.py
is using a TextTestRunner instance in order to pass the output stream.
By moving the machinery to obtain reproducible unittest output into a
TextTestRunner subclass, we can just pass the class name to unittest.main.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qemu-iotests/iotests.py | 60 ++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 25 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 90d0b62523..b9f4edfc42 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -1271,38 +1271,49 @@ def func_wrapper(*args, **kwargs):
             return func(*args, **kwargs)
     return func_wrapper
 
-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()
+# We need to filter out the time taken from the output so that
+# qemu-iotest can reliably diff the results against master output.
+class ReproducibleTextTestRunner(unittest.TextTestRunner):
+    __output = None
+
+    @classmethod
+    @property
+    def output(cls):
+        if cls.__output is not None:
+            return cls.__output
+
+        cls.__output = io.StringIO()
+        def print_output():
+            out = cls.__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)
 
+        atexit.register(print_output)
+        return cls.__output
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(stream=ReproducibleTextTestRunner.output, *args, **kwargs)
+
+def execute_unittest(argv, debug=False):
+    """Executes unittests within the calling module."""
+
+    # If we see non-empty argv we must not be invoked as a test script,
+    # so do not bother making constant output; debuggability is more
+    # important.
+    if debug or len(argv) > 1:
+        argv += ['-v']
+        runner = unittest.TextTestRunner
+    else:
+        runner = ReproducibleTextTestRunner
+
+    unittest.main(argv=argv, testRunner=runner,
+                  warnings=None if sys.warnoptions else 'ignore')
+
 def execute_setup_common(supported_fmts: Sequence[str] = (),
                          supported_platforms: Sequence[str] = (),
                          supported_cache_modes: Sequence[str] = (),
@@ -1338,7 +1348,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

Re: [PATCH 1/4] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Vladimir Sementsov-Ogievskiy]

23.03.2021 16:06, 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 also there are no other options available.  Unfortunately, these
> command line options only work if the unittest.main testRunner argument
> is a type, rather than a TestRunner instance, and right now iotests.py
> is using a TextTestRunner instance in order to pass the output stream.
> By moving the machinery to obtain reproducible unittest output into a
> TextTestRunner subclass, we can just pass the class name to unittest.main.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Great that you are working on it!

I keep in mind the necessity of supporting running test-cases in separate, but never actually start doing it.

And in my dreams I usually just drop the
"""
...
----------------------------------------------------------------------
Ran 3 tests

OK
"""

output at all, as it gives NO information.

When unittest-based test fails we have a native backtrace, and nothing more needed. (OK, information about crashed process counts too).

But actually, we don't need all these .out for unittest-based tests.

So, I'd drop it. But this is more work, and includes updating one-two unittest-based iotests that still have reasonable output, so this patch is good anyway.

> ---
>   tests/qemu-iotests/iotests.py | 60 ++++++++++++++++++++---------------
>   1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 90d0b62523..b9f4edfc42 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -1271,38 +1271,49 @@ def func_wrapper(*args, **kwargs):
>               return func(*args, **kwargs)
>       return func_wrapper
>   
> -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()
> +# We need to filter out the time taken from the output so that
> +# qemu-iotest can reliably diff the results against master output.
> +class ReproducibleTextTestRunner(unittest.TextTestRunner):
> +    __output = None
> +
> +    @classmethod
> +    @property
> +    def output(cls):
> +        if cls.__output is not None:
> +            return cls.__output
> +
> +        cls.__output = io.StringIO()
> +        def print_output():
> +            out = cls.__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)
>   
> +        atexit.register(print_output)
> +        return cls.__output
> +
> +    def __init__(self, *args, **kwargs):
> +        super().__init__(stream=ReproducibleTextTestRunner.output, *args, **kwargs)

over-79 line (PEP8)

> +
> +def execute_unittest(argv, debug=False):
> +    """Executes unittests within the calling module."""
> +
> +    # If we see non-empty argv we must not be invoked as a test script,
> +    # so do not bother making constant output; debuggability is more
> +    # important.
> +    if debug or len(argv) > 1:

It's native to rely on converting sequences to bool. Empty sequence is False and non-empty is True, just like

   if debug or argv:

> +        argv += ['-v']
> +        runner = unittest.TextTestRunner
> +    else:
> +        runner = ReproducibleTextTestRunner
> +
> +    unittest.main(argv=argv, testRunner=runner,
> +                  warnings=None if sys.warnoptions else 'ignore')

This thing about warnings seems unrelated change and not described in the commit message

> +
>   def execute_setup_common(supported_fmts: Sequence[str] = (),
>                            supported_platforms: Sequence[str] = (),
>                            supported_cache_modes: Sequence[str] = (),
> @@ -1338,7 +1348,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()
>   
> 

Why isn't it simpler just to add argv argument to original function, and on "debug or argv" path just pass unittest.TextTestRunner instead of constructing the object? Why do we need new class and switching to atexit()?

(I think, I'm OK with it as is, just wonder)

-- 
Best regards,
Vladimir

Re: [PATCH 1/4] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Paolo Bonzini]

On 23/03/21 15:34, Vladimir Sementsov-Ogievskiy wrote:
>> +    def __init__(self, *args, **kwargs):
>> +        super().__init__(stream=ReproducibleTextTestRunner.output, 
>> *args, **kwargs)
> 
> over-79 line (PEP8)

Ok, thanks.

>> +
>> +def execute_unittest(argv, debug=False):
>> +    """Executes unittests within the calling module."""
>> +
>> +    # If we see non-empty argv we must not be invoked as a test script,
>> +    # so do not bother making constant output; debuggability is more
>> +    # important.
>> +    if debug or len(argv) > 1:
> 
> It's native to rely on converting sequences to bool. Empty sequence is 
> False and non-empty is True, just like
> 
>    if debug or argv:

No, this is checking if there is *more than one* element in argv, 
because argv contains the program name as argv[0].  It's trying to catch 
the case of "./run testclass.TestMethod" or "./run -v" and not buffer 
the output, but it sucks.  Really this patchset should have been marked 
as RFC.

A better implementation is to create a class that wraps sys.stdout and 
overrides write() to ensure reproducibility.  There is no need to buffer 
the output in the StringIO at all.

>> +        argv += ['-v']
>> +        runner = unittest.TextTestRunner
>> +    else:
>> +        runner = ReproducibleTextTestRunner
>> +
>> +    unittest.main(argv=argv, testRunner=runner,
>> +                  warnings=None if sys.warnoptions else 'ignore')
> 
> This thing about warnings seems unrelated change and not described in 
> the commit message

The default settings for warnings is different when you instantiate 
TextTestRunner and when you use unittest.main.  Currently the tests have 
some warnings, they need to be disabled otherwise the tests fail. 
Honestly, I don't have time to debug the warnings and they are 
pre-existing anyway.  But you're right, at least I should have a comment 
describing the purpose of the warnings keyword argument.

>> +
>>   def execute_setup_common(supported_fmts: Sequence[str] = (),
>>                            supported_platforms: Sequence[str] = (),
>>                            supported_cache_modes: Sequence[str] = (),
>> @@ -1338,7 +1348,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()
>>
> 
> Why isn't it simpler just to add argv argument to original function, and 
> on "debug or argv" path just pass unittest.TextTestRunner instead of 
> constructing the object? Why do we need new class and switching to 
> atexit()?

mypy complains because you set the variable to two different types on 
the two branches.  So I decided it was cleaner to write a new runner 
class.  I think this is the only remaining part of the patch that I like. :)

Thanks,

Paolo

Re: [PATCH 1/4] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Vladimir Sementsov-Ogievskiy]

23.03.2021 18:29, Paolo Bonzini wrote:
> On 23/03/21 15:34, Vladimir Sementsov-Ogievskiy wrote:
>>> +    def __init__(self, *args, **kwargs):
>>> +        super().__init__(stream=ReproducibleTextTestRunner.output, *args, **kwargs)
>>
>> over-79 line (PEP8)
> 
> Ok, thanks.
> 
>>> +
>>> +def execute_unittest(argv, debug=False):
>>> +    """Executes unittests within the calling module."""
>>> +
>>> +    # If we see non-empty argv we must not be invoked as a test script,
>>> +    # so do not bother making constant output; debuggability is more
>>> +    # important.
>>> +    if debug or len(argv) > 1:
>>
>> It's native to rely on converting sequences to bool. Empty sequence is False and non-empty is True, just like
>>
>>    if debug or argv:
> 
> No, this is checking if there is *more than one*

Ah, oops, yes, right :\

element in argv, because argv contains the program name as argv[0].  It's trying to catch the case of "./run testclass.TestMethod" or "./run -v" and not buffer the output, but it sucks.  Really this patchset should have been marked as RFC.
> 
> A better implementation is to create a class that wraps sys.stdout and overrides write() to ensure reproducibility.  There is no need to buffer the output in the StringIO at all.
> 
>>> +        argv += ['-v']
>>> +        runner = unittest.TextTestRunner
>>> +    else:
>>> +        runner = ReproducibleTextTestRunner
>>> +
>>> +    unittest.main(argv=argv, testRunner=runner,
>>> +                  warnings=None if sys.warnoptions else 'ignore')
>>
>> This thing about warnings seems unrelated change and not described in the commit message
> 
> The default settings for warnings is different when you instantiate TextTestRunner and when you use unittest.main.  Currently the tests have some warnings, they need to be disabled otherwise the tests fail. Honestly, I don't have time to debug the warnings and they are pre-existing anyway.  But you're right, at least I should have a comment describing the purpose of the warnings keyword argument.
> 
>>> +
>>>   def execute_setup_common(supported_fmts: Sequence[str] = (),
>>>                            supported_platforms: Sequence[str] = (),
>>>                            supported_cache_modes: Sequence[str] = (),
>>> @@ -1338,7 +1348,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()
>>>
>>
>> Why isn't it simpler just to add argv argument to original function, and on "debug or argv" path just pass unittest.TextTestRunner instead of constructing the object? Why do we need new class and switching to atexit()?
> 
> mypy complains because you set the variable to two different types on the two branches.  So I decided it was cleaner to write a new runner class.  I think this is the only remaining part of the patch that I like. :)
> 
> Thanks,
> 
> Paolo
> 


-- 
Best regards,
Vladimir

Re: [PATCH 1/4] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Vladimir Sementsov-Ogievskiy]

23.03.2021 18:29, Paolo Bonzini wrote:
> On 23/03/21 15:34, Vladimir Sementsov-Ogievskiy wrote:
>>> +    def __init__(self, *args, **kwargs):
>>> +        super().__init__(stream=ReproducibleTextTestRunner.output, *args, **kwargs)
>>
>> over-79 line (PEP8)
> 
> Ok, thanks.
> 
>>> +
>>> +def execute_unittest(argv, debug=False):
>>> +    """Executes unittests within the calling module."""
>>> +
>>> +    # If we see non-empty argv we must not be invoked as a test script,
>>> +    # so do not bother making constant output; debuggability is more
>>> +    # important.
>>> +    if debug or len(argv) > 1:
>>
>> It's native to rely on converting sequences to bool. Empty sequence is False and non-empty is True, just like
>>
>>    if debug or argv:
> 
> No, this is checking if there is *more than one* element in argv, because argv contains the program name as argv[0].  It's trying to catch the case of "./run testclass.TestMethod" or "./run -v" and not buffer the output, but it sucks.  Really this patchset should have been marked as RFC.
> 
> A better implementation is to create a class that wraps sys.stdout and overrides write() to ensure reproducibility.  There is no need to buffer the output in the StringIO at all.
> 
>>> +        argv += ['-v']
>>> +        runner = unittest.TextTestRunner
>>> +    else:
>>> +        runner = ReproducibleTextTestRunner
>>> +
>>> +    unittest.main(argv=argv, testRunner=runner,
>>> +                  warnings=None if sys.warnoptions else 'ignore')
>>
>> This thing about warnings seems unrelated change and not described in the commit message
> 
> The default settings for warnings is different when you instantiate TextTestRunner and when you use unittest.main.  Currently the tests have some warnings, they need to be disabled otherwise the tests fail. Honestly, I don't have time to debug the warnings and they are pre-existing anyway.  But you're right, at least I should have a comment describing the purpose of the warnings keyword argument.
> 
>>> +
>>>   def execute_setup_common(supported_fmts: Sequence[str] = (),
>>>                            supported_platforms: Sequence[str] = (),
>>>                            supported_cache_modes: Sequence[str] = (),
>>> @@ -1338,7 +1348,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()
>>>
>>
>> Why isn't it simpler just to add argv argument to original function, and on "debug or argv" path just pass unittest.TextTestRunner instead of constructing the object? Why do we need new class and switching to atexit()?
> 
> mypy complains because you set the variable to two different types on the two branches. 

hmm, just use a separate call of unittest.main, or honestly define a varaible as Union[] or just Any ?

> So I decided it was cleaner to write a new runner class.  I think this is the only remaining part of the patch that I like. :)
> 

For me normal try-finally is a lot more clean than calling atexit() in a class method. It's just a strange interface. Prior to the patch user can call execute_unittest several times and expect that output will be printed during the call. After the patch outputs of all calls to execute_unittest() will be printed at program exit..


-- 
Best regards,
Vladimir

Re: [PATCH 1/4] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Paolo Bonzini]

On 23/03/21 17:56, Vladimir Sementsov-Ogievskiy wrote:
> hmm, just use a separate call of unittest.main, or honestly define a 
> varaible as Union[] or just Any ?

All the ugliness goes away if the implementation is done properly. :)

> For me normal try-finally is a lot more clean than calling atexit() in a 
> class method. It's just a strange interface. Prior to the patch user can 
> call execute_unittest several times and expect that output will be 
> printed during the call. After the patch outputs of all calls to 
> execute_unittest() will be printed at program exit..

Yeah, I agree.  I didn't like the finally, but I really didn't like it
because of the *behavior* of buffering the whole output.  I have changed
it to drop the buffering altogether, that's much better code and more usable:

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 90d0b62523..a74f4f9488 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=ReproducibleTestRunner)
  
  def execute_setup_common(supported_fmts: Sequence[str] = (),
                           supported_platforms: Sequence[str] = (),

Re: [PATCH 1/4] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Vladimir Sementsov-Ogievskiy]

23.03.2021 20:04, Paolo Bonzini wrote:
> On 23/03/21 17:56, Vladimir Sementsov-Ogievskiy wrote:
>> hmm, just use a separate call of unittest.main, or honestly define a varaible as Union[] or just Any ?
> 
> All the ugliness goes away if the implementation is done properly. :)
> 
>> For me normal try-finally is a lot more clean than calling atexit() in a class method. It's just a strange interface. Prior to the patch user can call execute_unittest several times and expect that output will be printed during the call. After the patch outputs of all calls to execute_unittest() will be printed at program exit..
> 
> Yeah, I agree.  I didn't like the finally, but I really didn't like it
> because of the *behavior* of buffering the whole output.  I have changed
> it to drop the buffering altogether, that's much better code and more usable:

Me too. Never liked buffering of test output.

> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 90d0b62523..a74f4f9488 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=ReproducibleTestRunner)
> 
>   def execute_setup_common(supported_fmts: Sequence[str] = (),
>                            supported_platforms: Sequence[str] = (),
> 


Sounds good :) We'll see dots appearing dynamically during test run, yes?

[anyway, I'd drop "...." test outputs for unittest-based tests at some moment... But after that patch I'll have to keep some kind of "progress bar" :]

-- 
Best regards,
Vladimir

Re: [PATCH 1/4] qemu-iotests: allow passing unittest.main arguments to the test scripts

[Paolo Bonzini]

On 23/03/21 18:27, Vladimir Sementsov-Ogievskiy wrote:
> Sounds good :) We'll see dots appearing dynamically during test run, yes?

Yes, exactly! :)

Paolo

[PATCH 2/4] 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.

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}
+"""
 
         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
 
-- 
2.30.1

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

[Vladimir Sementsov-Ogievskiy]

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

[PATCH 3/4] 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.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/qemu-iotests/check | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index d1c87ceaf1..aab25dac6a 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
@@ -100,8 +101,8 @@ def make_argparser() -> argparse.ArgumentParser:
                        'one to TEST (not inclusive). This may be used to '
                        'rerun failed ./check command, starting from the '
                        'middle of the process.')
-    g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
-                       help='tests to run')
+    g_sel.add_argument('tests', nargs=argparse.REMAINDER,
+                       help='tests to run, or "--" followed by a command')
 
     return p
 
@@ -114,6 +115,17 @@ if __name__ == '__main__':
                   imgopts=args.imgopts, misalign=args.misalign,
                   debug=args.debug, valgrind=args.valgrind)
 
+    if args.tests[0] == '--':
+        del args.tests[0]
+        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
-- 
2.30.1

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

[Vladimir Sementsov-Ogievskiy]

23.03.2021 16:06, 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   tests/qemu-iotests/check | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index d1c87ceaf1..aab25dac6a 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
> @@ -100,8 +101,8 @@ def make_argparser() -> argparse.ArgumentParser:
>                          'one to TEST (not inclusive). This may be used to '
>                          'rerun failed ./check command, starting from the '
>                          'middle of the process.')
> -    g_sel.add_argument('tests', metavar='TEST_FILES', nargs='*',
> -                       help='tests to run')
> +    g_sel.add_argument('tests', nargs=argparse.REMAINDER,

Interesting that REMAINDER documentation disappeared from latest (3.9) python documentation https://docs.python.org/3.9/library/argparse.html , but exists here https://docs.python.org/3.8/library/argparse.html  (and no mark of deprecation)

> +                       help='tests to run, or "--" followed by a command')
>   
>       return p
>   
> @@ -114,6 +115,17 @@ if __name__ == '__main__':
>                     imgopts=args.imgopts, misalign=args.misalign,
>                     debug=args.debug, valgrind=args.valgrind)
>   
> +    if args.tests[0] == '--':
> +        del args.tests[0]
> +        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
> 

I hope at some moment we'll run testcases with syntax like

./check -qcow2 test_name.ClassName.method_name

But a possibility to run any command under check script defined environment is a good thing anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

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

[Paolo Bonzini]

On 23/03/21 17:43, Vladimir Sementsov-Ogievskiy wrote:
> 
> Interesting that REMAINDER documentation disappeared from latest (3.9) 
> python documentation https://docs.python.org/3.9/library/argparse.html , 
> but exists here https://docs.python.org/3.8/library/argparse.html  (and 
> no mark of deprecation)

Whoa.  https://bugs.python.org/issue17050 says:

---
Since this feature is buggy, and there isn't an easy fix, we should 
probably remove any mention of it from the docs.  We can still leave it 
as an undocumented legacy feature.

There is precedent for leaving `nargs` constants undocumented. 
`argparse.PARSER` ('+...') is used by the subparser mechanism, but is 
not documented.  https://bugs.python.org/issue16988
---

The problematic case appears to be when you have more than one 
positional argument, which is exactly the case with the 3.8 documented 
use of REMAINDER.  Hence the decision to drop the documentation.

However, "check" works fine because the REMAINDER argument is the only 
positional argument:

     $ ./check 001 -d
     Test "tests/-d" is not found

Another possibility is to pre-process sys.argv like this:

     if '--' in sys.argv:
         cmd = True
         args = sys.argv[0:sys.argv.index('--')]
         posargs = sys.argv[len(args)+1:]
     else:
         cmd = False
         args = list(x for x in sys.argv if x.startswith('-'))
         posargs = list(x for x in sys.argv if not x.startswith('-'))

But getting the help message right etc. would be messy.

Paolo

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

[Vladimir Sementsov-Ogievskiy]

23.03.2021 20:00, Paolo Bonzini wrote:
> On 23/03/21 17:43, Vladimir Sementsov-Ogievskiy wrote:
>>
>> Interesting that REMAINDER documentation disappeared from latest (3.9) python documentation https://docs.python.org/3.9/library/argparse.html , but exists here https://docs.python.org/3.8/library/argparse.html  (and no mark of deprecation)
> 
> Whoa.  https://bugs.python.org/issue17050 says:
> 
> ---
> Since this feature is buggy, and there isn't an easy fix, we should probably remove any mention of it from the docs.  We can still leave it as an undocumented legacy feature.
> 
> There is precedent for leaving `nargs` constants undocumented. `argparse.PARSER` ('+...') is used by the subparser mechanism, but is not documented.  https://bugs.python.org/issue16988
> ---
> 
> The problematic case appears to be when you have more than one positional argument, which is exactly the case with the 3.8 documented use of REMAINDER.  Hence the decision to drop the documentation.
> 
> However, "check" works fine because the REMAINDER argument is the only positional argument:
> 
>      $ ./check 001 -d
>      Test "tests/-d" is not found
> 
> Another possibility is to pre-process sys.argv like this:
> 
>      if '--' in sys.argv:
>          cmd = True
>          args = sys.argv[0:sys.argv.index('--')]
>          posargs = sys.argv[len(args)+1:]
>      else:
>          cmd = False
>          args = list(x for x in sys.argv if x.startswith('-'))
>          posargs = list(x for x in sys.argv if not x.startswith('-'))
> 
> But getting the help message right etc. would be messy.
> 
> Paolo
> 


Hmm:

> If you have positional arguments that must begin with - and don’t look like negative numbers, you can insert the pseudo-argument '--' which tells parse_args() that everything after that is a positional argument:

So, as I understand argparse supports '--' feature out of the box. So, we can keep '*' as is, and it would parse all remaining positional arguments which are either tests or the command, and '--' will be automatically dropped. So, we only need to check existing of '--' in original sys.argv to chose our behavior.

-- 
Best regards,
Vladimir

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

[Paolo Bonzini]

On 23/03/21 18:11, Vladimir Sementsov-Ogievskiy wrote:
>> If you have positional arguments that must begin with - and don’t look 
>> like negative numbers, you can insert the pseudo-argument '--' which 
>> tells parse_args() that everything after that is a positional argument:
> 
> So, as I understand argparse supports '--' feature out of the box. So, 
> we can keep '*' as is, and it would parse all remaining positional 
> arguments which are either tests or the command, and '--' will be 
> automatically dropped. So, we only need to check existing of '--' in 
> original sys.argv to chose our behavior.

There is still a difference with REMAINDER:

./check aa -- bb
=> REMAINDER: error because ./-- is not a test
=> look for '--':  invoke "aa -- bb"

So I think REMAINDER provides the best behavior overall.

Paolo

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

[Vladimir Sementsov-Ogievskiy]

23.03.2021 20:22, Paolo Bonzini wrote:
> On 23/03/21 18:11, Vladimir Sementsov-Ogievskiy wrote:
>>> If you have positional arguments that must begin with - and don’t look like negative numbers, you can insert the pseudo-argument '--' which tells parse_args() that everything after that is a positional argument:
>>
>> So, as I understand argparse supports '--' feature out of the box. So, we can keep '*' as is, and it would parse all remaining positional arguments which are either tests or the command, and '--' will be automatically dropped. So, we only need to check existing of '--' in original sys.argv to chose our behavior.
> 
> There is still a difference with REMAINDER:
> 
> ./check aa -- bb
> => REMAINDER: error because ./-- is not a test
> => look for '--':  invoke "aa -- bb"
> 
> So I think REMAINDER provides the best behavior overall.
> 

Ok, with '*' you need to check that exactly sys.argv[-len(agrs.tests)-1] == '--', which gives the same behavior as REMAINDER.

I'm OK with REMAINDER too, still with we probably also need some comment to not embarrass next person who go into documentation and not find it.


-- 
Best regards,
Vladimir

[PATCH 4/4] 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.

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 4/4] qemu-iotests: fix case of SOCK_DIR already in the environment

[Vladimir Sementsov-Ogievskiy]

23.03.2021 16:06, Paolo Bonzini wrote:
> Due to a typo, in this case the SOCK_DIR was not being created.
> 
> 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
> 

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

-- 
Best regards,
Vladimir