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