qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 1/4] qemu-iotests: allow passing unittest.main arguments to the test scripts
Date: Tue, 23 Mar 2021 19:04:05 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]