[Qemu-devel] [PATCH v2 0/5] iotests: Selfish patches

[Qemu-devel] [PATCH v2 0/5] iotests: Selfish patches

[Max Reitz]

Hi,

These are some rather selfish iotests patches.  The first patch helps me
personally because I tend to run the tests over SSH and forget to set
$DISPLAY.  That makes test 139 skip the tests annotated with
skip_if_unsupprted(), because iotests.py can no longer determine the
list of whitelisted formats.

Patches 2 through 5 are specifically for RHEL.  We have not whitelisted
null-aio, so it would be nice if tests didn’t require it.  Sorry, I
don’t have a better reason to give.
In all seriousness, null-co is used widely in many tests, it basically
is our standard null driver.  Tests should prefer it over null-aio, just
for consistency alone.  It is not completely unreasonable to treat
null-aio as optional.  I guess.


v2:
- Allow tests to use the unittest module’s skipTest() so it is possible
  to skip a test case in the setUp() method (new patch 3).  Then use
  this in patches 4 and 5 to skip all null-aio cases instead of falling
  back to null-co and thus running tests twice.

- Patch 1 needed to be rebased on
  4a715461c8eab628e79b1e6889d650455e043b88
  (“tests/qemu-iotests/check: Pick a default machine if necessary”)


git backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/5:[0008] [FC] 'iotests: Add -display none to the qemu options'
002/5:[----] [--] 'iotests: Prefer null-co over null-aio'
003/5:[down] 'iotests: Allow skipping test cases'
004/5:[0007] [FC] 'iotests: Test driver whitelisting in 093'
005/5:[0007] [FC] 'iotests: Test driver whitelisting in 136'


Max Reitz (5):
  iotests: Add -display none to the qemu options
  iotests: Prefer null-co over null-aio
  iotests: Allow skipping test cases
  iotests: Test driver whitelisting in 093
  iotests: Test driver whitelisting in 136

 tests/qemu-iotests/093        | 21 ++++++++++++++-------
 tests/qemu-iotests/136        | 16 ++++++++++++----
 tests/qemu-iotests/245        |  2 +-
 tests/qemu-iotests/check      |  6 +++---
 tests/qemu-iotests/iotests.py | 20 ++++++++++++++++++--
 5 files changed, 48 insertions(+), 17 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/5] iotests: Add -display none to the qemu options

[Max Reitz]

Without this argument, qemu will print an angry message about not being
able to connect to a display server if $DISPLAY is not set.  For me,
that breaks iotests.supported_formats() because it thus only sees
["Could", "not", "connect"] as the supported formats.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/check | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index f925606cc5..f2a0df8af2 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -581,13 +581,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
 
 case "$QEMU_PROG" in
     *qemu-system-arm|*qemu-system-aarch64)
-        export QEMU_OPTIONS="-nodefaults -machine virt,accel=qtest"
+        export QEMU_OPTIONS="-nodefaults -display none -machine virt,accel=qtest"
         ;;
     *qemu-system-tricore)
-        export QEMU_OPTIONS="-nodefaults -machine tricore_testboard,accel=qtest"
+        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard,accel=qtest"
         ;;
     *)
-        export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
+        export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
         ;;
 esac
 
-- 
2.21.0

[Qemu-devel] [PATCH v2 2/5] iotests: Prefer null-co over null-aio

[Max Reitz]

We use null-co basically everywhere in the iotests.  Unless we want to
test null-aio specifically, we should use it instead (for consistency).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/093 | 7 +++----
 tests/qemu-iotests/245 | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index d88fbc182e..bd56c94708 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -267,13 +267,12 @@ class ThrottleTestCoroutine(ThrottleTestCase):
     test_img = "null-co://"
 
 class ThrottleTestGroupNames(iotests.QMPTestCase):
-    test_img = "null-aio://"
     max_drives = 3
 
     def setUp(self):
         self.vm = iotests.VM()
         for i in range(0, self.max_drives):
-            self.vm.add_drive(self.test_img, "throttling.iops-total=100")
+            self.vm.add_drive("null-co://", "throttling.iops-total=100")
         self.vm.launch()
 
     def tearDown(self):
@@ -377,10 +376,10 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
     def test_removable_media(self):
         # Add a couple of dummy nodes named cd0 and cd1
-        result = self.vm.qmp("blockdev-add", driver="null-aio",
+        result = self.vm.qmp("blockdev-add", driver="null-co",
                              node_name="cd0")
         self.assert_qmp(result, 'return', {})
-        result = self.vm.qmp("blockdev-add", driver="null-aio",
+        result = self.vm.qmp("blockdev-add", driver="null-co",
                              node_name="cd1")
         self.assert_qmp(result, 'return', {})
 
diff --git a/tests/qemu-iotests/245 b/tests/qemu-iotests/245
index bc1ceb9792..ae169778b0 100644
--- a/tests/qemu-iotests/245
+++ b/tests/qemu-iotests/245
@@ -598,7 +598,7 @@ class TestBlockdevReopen(iotests.QMPTestCase):
         ##################
         ###### null ######
         ##################
-        opts = {'driver': 'null-aio', 'node-name': 'root', 'size': 1024}
+        opts = {'driver': 'null-co', 'node-name': 'root', 'size': 1024}
 
         result = self.vm.qmp('blockdev-add', conv_keys = False, **opts)
         self.assert_qmp(result, 'return', {})
-- 
2.21.0

[Qemu-devel] [PATCH v2 3/5] iotests: Allow skipping test cases

[Max Reitz]

case_notrun() does not actually skip the current test case.  It just
adds a "notrun" note and then returns to the caller, who manually has to
skip the test.  Generally, skipping a test case is as simple as
returning from the current function, but not always: For example, this
model does not allow skipping tests already in the setUp() function.

Thus, add a QMPTestCase.case_skip() function that invokes case_notrun()
and then self.skipTest().  To make this work, we need to filter the
information on how many test cases were skipped from the unittest
output.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3ecef5bc90..1b0ac50153 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -741,6 +741,11 @@ class QMPTestCase(unittest.TestCase):
             return self.pause_wait(job_id)
         return result
 
+    def case_skip(self, reason):
+        '''Skip this test case'''
+        case_notrun(reason)
+        self.skipTest(reason)
+
 
 def notrun(reason):
     '''Skip this test suite'''
@@ -752,7 +757,10 @@ def notrun(reason):
     sys.exit(0)
 
 def case_notrun(reason):
-    '''Skip this test case'''
+    '''Mark this test case as not having been run, but do not actually
+    skip it; that is left to the caller.  See QMPTestCase.case_skip()
+    for a variant that actually skips the current test case.'''
+
     # Each test in qemu-iotests has a number ("seq")
     seq = os.path.basename(sys.argv[0])
 
@@ -879,4 +887,12 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[],
         unittest.main(testRunner=MyTestRunner)
     finally:
         if not debug:
-            sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', output.getvalue()))
+            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)
-- 
2.21.0

[Qemu-devel] [PATCH v2 4/5] iotests: Test driver whitelisting in 093

[Max Reitz]

null-aio may not be whitelisted.  Skip all test cases that require it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/093 | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index bd56c94708..806bdcaa24 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -22,9 +22,11 @@
 import iotests
 
 nsec_per_sec = 1000000000
+supported_null_drivers = [ f for f in iotests.supported_formats()
+                           if f.startswith('null-') ]
 
 class ThrottleTestCase(iotests.QMPTestCase):
-    test_img = "null-aio://"
+    test_driver = "null-aio"
     max_drives = 3
 
     def blockstats(self, device):
@@ -36,9 +38,13 @@ class ThrottleTestCase(iotests.QMPTestCase):
         raise Exception("Device not found for blockstats: %s" % device)
 
     def setUp(self):
+        global supported_null_drivers
+        if self.test_driver not in supported_null_drivers:
+            self.case_skip('%s support missing' % self.test_driver)
+
         self.vm = iotests.VM()
         for i in range(0, self.max_drives):
-            self.vm.add_drive(self.test_img)
+            self.vm.add_drive(self.test_driver + "://")
         self.vm.launch()
 
     def tearDown(self):
@@ -264,7 +270,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
         self.assertEqual(self.blockstats('drive1')[0], 4096)
 
 class ThrottleTestCoroutine(ThrottleTestCase):
-    test_img = "null-co://"
+    test_driver = "null-co"
 
 class ThrottleTestGroupNames(iotests.QMPTestCase):
     max_drives = 3
@@ -426,4 +432,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
 
 if __name__ == '__main__':
+    if 'null-co' not in supported_null_drivers:
+        iotests.notrun('null-co driver support missing')
     iotests.main(supported_fmts=["raw"])
-- 
2.21.0

[Qemu-devel] [PATCH v2 5/5] iotests: Test driver whitelisting in 136

[Max Reitz]

null-aio may not be whitelisted.  Skip all test cases that require it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/136 | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index af7ffa4540..4ef95517a1 100755
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -28,9 +28,11 @@ op_latency = nsec_per_sec // 1000 # See qtest_latency_ns in accounting.c
 bad_sector = 8192
 bad_offset = bad_sector * 512
 blkdebug_file = os.path.join(iotests.test_dir, 'blkdebug.conf')
+supported_null_drivers = [ f for f in iotests.supported_formats()
+                           if f.startswith('null-') ]
 
 class BlockDeviceStatsTestCase(iotests.QMPTestCase):
-    test_img = "null-aio://"
+    test_driver = "null-aio"
     total_rd_bytes = 0
     total_rd_ops = 0
     total_wr_bytes = 0
@@ -68,6 +70,10 @@ sector = "%d"
         file.close()
 
     def setUp(self):
+        global supported_null_drivers
+        if self.test_driver not in supported_null_drivers:
+            self.case_skip('%s support missing' % self.test_driver)
+
         drive_args = []
         drive_args.append("stats-intervals.0=%d" % interval_length)
         drive_args.append("stats-account-invalid=%s" %
@@ -75,8 +81,8 @@ sector = "%d"
         drive_args.append("stats-account-failed=%s" %
                           (self.account_failed and "on" or "off"))
         self.create_blkdebug_file()
-        self.vm = iotests.VM().add_drive('blkdebug:%s:%s' %
-                                         (blkdebug_file, self.test_img),
+        self.vm = iotests.VM().add_drive('blkdebug:%s:%s://' %
+                                         (blkdebug_file, self.test_driver),
                                          ','.join(drive_args))
         self.vm.launch()
         # Set an initial value for the clock
@@ -336,7 +342,9 @@ class BlockDeviceStatsTestAccountBoth(BlockDeviceStatsTestCase):
     account_failed = True
 
 class BlockDeviceStatsTestCoroutine(BlockDeviceStatsTestCase):
-    test_img = "null-co://"
+    test_driver = "null-co"
 
 if __name__ == '__main__':
+    if 'null-co' not in supported_null_drivers:
+        iotests.notrun('null-co driver support missing')
     iotests.main(supported_fmts=["raw"])
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 1/5] iotests: Add -display none to the qemu options

[Philippe Mathieu-Daudé]

On 6/25/19 11:19 PM, Max Reitz wrote:
> Without this argument, qemu will print an angry message about not being
> able to connect to a display server if $DISPLAY is not set.  For me,
> that breaks iotests.supported_formats() because it thus only sees
> ["Could", "not", "connect"] as the supported formats.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  tests/qemu-iotests/check | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index f925606cc5..f2a0df8af2 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -581,13 +581,13 @@ export QEMU_PROG="$(type -p "$QEMU_PROG")"
>  
>  case "$QEMU_PROG" in
>      *qemu-system-arm|*qemu-system-aarch64)
> -        export QEMU_OPTIONS="-nodefaults -machine virt,accel=qtest"
> +        export QEMU_OPTIONS="-nodefaults -display none -machine virt,accel=qtest"
>          ;;
>      *qemu-system-tricore)
> -        export QEMU_OPTIONS="-nodefaults -machine tricore_testboard,accel=qtest"
> +        export QEMU_OPTIONS="-nodefaults -display none -machine tricore_testboard,accel=qtest"
>          ;;
>      *)
> -        export QEMU_OPTIONS="-nodefaults -machine accel=qtest"
> +        export QEMU_OPTIONS="-nodefaults -display none -machine accel=qtest"
>          ;;
>  esac
>  
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/5] iotests: Prefer null-co over null-aio

[John Snow]

On 6/25/19 5:19 PM, Max Reitz wrote:
> We use null-co basically everywhere in the iotests.  Unless we want to
> test null-aio specifically, we should use it instead (for consistency).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Probably fine.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/5] iotests: Allow skipping test cases

[John Snow]

On 6/25/19 5:19 PM, Max Reitz wrote:
> case_notrun() does not actually skip the current test case.  It just
> adds a "notrun" note and then returns to the caller, who manually has to
> skip the test.  Generally, skipping a test case is as simple as
> returning from the current function, but not always: For example, this
> model does not allow skipping tests already in the setUp() function.
> 
> Thus, add a QMPTestCase.case_skip() function that invokes case_notrun()
> and then self.skipTest().  To make this work, we need to filter the
> information on how many test cases were skipped from the unittest
> output.
> 

I feel like skipping sub-tests has been a point of contention before,
but I don't recall the outcome.

Cleber, can you help jog my memory?

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 3ecef5bc90..1b0ac50153 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -741,6 +741,11 @@ class QMPTestCase(unittest.TestCase):
>              return self.pause_wait(job_id)
>          return result
>  
> +    def case_skip(self, reason):
> +        '''Skip this test case'''
> +        case_notrun(reason)
> +        self.skipTest(reason)
> +
>  
>  def notrun(reason):
>      '''Skip this test suite'''
> @@ -752,7 +757,10 @@ def notrun(reason):
>      sys.exit(0)
>  
>  def case_notrun(reason):
> -    '''Skip this test case'''
> +    '''Mark this test case as not having been run, but do not actually
> +    skip it; that is left to the caller.  See QMPTestCase.case_skip()
> +    for a variant that actually skips the current test case.'''
> +
>      # Each test in qemu-iotests has a number ("seq")
>      seq = os.path.basename(sys.argv[0])
>  
> @@ -879,4 +887,12 @@ def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[],
>          unittest.main(testRunner=MyTestRunner)
>      finally:
>          if not debug:
> -            sys.stderr.write(re.sub(r'Ran (\d+) tests? in [\d.]+s', r'Ran \1 tests', output.getvalue()))
> +            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)
> 

-- 
—js