[Qemu-devel] [PATCH v3 0/8] iotests: Selfish patches

[Qemu-devel] [PATCH v3 0/8] iotests: Selfish patches

[Max Reitz]

Hi,

Nothing has changed too much since the previous version, thus I’ll start
with a link to its cover letter:

https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg01102.html


What has changed is this:

v3:
- Patches 2 and 3: Resolved rebase conflicts

- Added patch 4.  It seems useful to support the new case-skipping
  method in @iotests.skip_if_unsupported so that annotation works even
  with setUp().

- Added patch 5.  This has been proposed by Kevin back in v1 (for a
  slightly different reason, though).
  Now that I added patch 4, it would be nice if I could make use of it
  for patches 6 and 7, but the problem is that both tests store the
  driver name in a class attribute.  Therefore, we need patch 5 that
  allows us to query that attribute’s value in skip_if_unsupported().

- Patches 6 and 7: Make use of the fact that the annotation works now
  (Also, I dropped supported_null_drivers, because there is no point in
  having it – especially with patch 8.)
  ((Also, resolved rebase conflicts.))

- Added patch 8.  It just itches me that we call qemu every time we
  inquire the list of supported formats, and this patch eases that itch.


git-backport-diff against v2:

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/8:[----] [--] 'iotests: Add -display none to the qemu options'
002/8:[0004] [FC] 'iotests: Prefer null-co over null-aio'
003/8:[0003] [FC] 'iotests: Allow skipping test cases'
004/8:[down] 'iotests: Use case_skip() in skip_if_unsupported()'
005/8:[down] 'iotests: Let skip_if_unsupported() accept a method'
006/8:[0014] [FC] 'iotests: Test driver whitelisting in 093'
007/8:[0010] [FC] 'iotests: Test driver whitelisting in 136'
008/8:[down] 'iotests: Cache supported_formats()'


Max Reitz (8):
  iotests: Add -display none to the qemu options
  iotests: Prefer null-co over null-aio
  iotests: Allow skipping test cases
  iotests: Use case_skip() in skip_if_unsupported()
  iotests: Let skip_if_unsupported() accept a method
  iotests: Test driver whitelisting in 093
  iotests: Test driver whitelisting in 136
  iotests: Cache supported_formats()

 tests/qemu-iotests/093        | 19 +++++++++------
 tests/qemu-iotests/136        | 14 +++++++----
 tests/qemu-iotests/245        |  2 +-
 tests/qemu-iotests/check      |  6 ++---
 tests/qemu-iotests/iotests.py | 45 ++++++++++++++++++++++++++++-------
 5 files changed, 62 insertions(+), 24 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v3 1/8] 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>
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 c24874ff4a..a58232eefb 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 v3 2/8] 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>
Reviewed-by: John Snow <jsnow@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 3c4f5173ce..50c1e7f2ec 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,
+            self.vm.add_drive("null-co://",
                               "throttling.iops-total=100,file.read-zeroes=on")
         self.vm.launch()
 
@@ -376,10 +375,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",
                              read_zeroes=True, 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",
                              read_zeroes=True, 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 v3 3/8] 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 | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 84438e837c..2f53baf633 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -802,6 +802,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'''
@@ -813,7 +818,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])
 
@@ -904,8 +912,15 @@ def execute_unittest(output, verbosity, debug):
         unittest.main(testRunner=runner)
     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)
 
 def execute_test(test_function=None,
                  supported_fmts=[], supported_oses=['linux'],
-- 
2.21.0

[Qemu-devel] [PATCH v3 4/8] iotests: Use case_skip() in skip_if_unsupported()

[Max Reitz]

skip_if_unsupported() should use the stronger variant case_skip(),
because this allows it to be used even with setUp() (in a meaningful
way).

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 2f53baf633..726f904f50 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -896,7 +896,7 @@ def skip_if_unsupported(required_formats=[], read_only=False):
             usf_list = list(set(required_formats) -
                             set(supported_formats(read_only)))
             if usf_list:
-                case_notrun('{}: formats {} are not whitelisted'.format(
+                args[0].case_skip('{}: formats {} are not whitelisted'.format(
                     args[0], usf_list))
             else:
                 return func(*args, **kwargs)
-- 
2.21.0

[Qemu-devel] [PATCH v3 5/8] iotests: Let skip_if_unsupported() accept a method

[Max Reitz]

This lets tests use skip_if_unsupported() with a potentially variable
list of required formats.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 726f904f50..8f315538e9 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -893,8 +893,12 @@ def skip_if_unsupported(required_formats=[], read_only=False):
        Runs the test if all the required formats are whitelisted'''
     def skip_test_decorator(func):
         def func_wrapper(*args, **kwargs):
-            usf_list = list(set(required_formats) -
-                            set(supported_formats(read_only)))
+            if callable(required_formats):
+                fmts = required_formats(args[0])
+            else:
+                fmts = required_formats
+
+            usf_list = list(set(fmts) - set(supported_formats(read_only)))
             if usf_list:
                 args[0].case_skip('{}: formats {} are not whitelisted'.format(
                     args[0], usf_list))
-- 
2.21.0

[Qemu-devel] [PATCH v3 6/8] 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 | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index 50c1e7f2ec..f03fa24a07 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -24,7 +24,7 @@ import iotests
 nsec_per_sec = 1000000000
 
 class ThrottleTestCase(iotests.QMPTestCase):
-    test_img = "null-aio://"
+    test_driver = "null-aio"
     max_drives = 3
 
     def blockstats(self, device):
@@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
                 return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
         raise Exception("Device not found for blockstats: %s" % device)
 
+    def required_drivers(self):
+        return [self.test_driver]
+
+    @iotests.skip_if_unsupported(required_drivers)
     def setUp(self):
         self.vm = iotests.VM()
         for i in range(0, self.max_drives):
-            self.vm.add_drive(self.test_img, "file.read-zeroes=on")
+            self.vm.add_drive(self.test_driver + "://", "file.read-zeroes=on")
         self.vm.launch()
 
     def tearDown(self):
@@ -264,7 +268,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
@@ -425,4 +429,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
 
 if __name__ == '__main__':
+    if 'null-co' not in iotests.supported_formats():
+        iotests.notrun('null-co driver support missing')
     iotests.main(supported_fmts=["raw"])
-- 
2.21.0

[Qemu-devel] [PATCH v3 7/8] 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 | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/136 b/tests/qemu-iotests/136
index a46a7b7630..012ea111ac 100755
--- a/tests/qemu-iotests/136
+++ b/tests/qemu-iotests/136
@@ -30,7 +30,7 @@ bad_offset = bad_sector * 512
 blkdebug_file = os.path.join(iotests.test_dir, 'blkdebug.conf')
 
 class BlockDeviceStatsTestCase(iotests.QMPTestCase):
-    test_img = "null-aio://"
+    test_driver = "null-aio"
     total_rd_bytes = 0
     total_rd_ops = 0
     total_wr_bytes = 0
@@ -67,6 +67,10 @@ sector = "%d"
 ''' % (bad_sector, bad_sector))
         file.close()
 
+    def required_drivers(self):
+        return [self.test_driver]
+
+    @iotests.skip_if_unsupported(required_drivers)
     def setUp(self):
         drive_args = []
         drive_args.append("stats-intervals.0=%d" % interval_length)
@@ -76,8 +80,8 @@ sector = "%d"
                           (self.account_failed and "on" or "off"))
         drive_args.append("file.image.read-zeroes=on")
         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
@@ -337,7 +341,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 iotests.supported_formats():
+        iotests.notrun('null-co driver support missing')
     iotests.main(supported_fmts=["raw"])
-- 
2.21.0

[Qemu-devel] [PATCH v3 8/8] iotests: Cache supported_formats()

[Max Reitz]

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8f315538e9..f6492b355f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -884,9 +884,17 @@ def qemu_pipe(*args):
 def supported_formats(read_only=False):
     '''Set 'read_only' to True to check ro-whitelist
        Otherwise, rw-whitelist is checked'''
-    format_message = qemu_pipe("-drive", "format=help")
-    line = 1 if read_only else 0
-    return format_message.splitlines()[line].split(":")[1].split()
+
+    if not hasattr(supported_formats, "formats"):
+        supported_formats.formats = {}
+
+    if read_only not in supported_formats.formats:
+        format_message = qemu_pipe("-drive", "format=help")
+        line = 1 if read_only else 0
+        supported_formats.formats[read_only] = \
+            format_message.splitlines()[line].split(":")[1].split()
+
+    return supported_formats.formats[read_only]
 
 def skip_if_unsupported(required_formats=[], read_only=False):
     '''Skip Test Decorator
-- 
2.21.0

Re: [Qemu-devel] [PATCH v3 2/8] iotests: Prefer null-co over null-aio

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2617 bytes --]

On 19.08.19 22:18, 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>
> Reviewed-by: John Snow <jsnow@redhat.com>

Hm, sorry, I just noticed that I probably should have dropped this R-b. :-/

(I mean, apart from the rebase conflict, nothing has changed, but still.)

Max

> ---
>  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 3c4f5173ce..50c1e7f2ec 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,
> +            self.vm.add_drive("null-co://",
>                                "throttling.iops-total=100,file.read-zeroes=on")
>          self.vm.launch()
>  
> @@ -376,10 +375,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",
>                               read_zeroes=True, 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",
>                               read_zeroes=True, 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', {})
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Thomas Huth]

On 8/19/19 10:18 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 c24874ff4a..a58232eefb 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

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 2/8] iotests: Prefer null-co over null-aio

[Thomas Huth]

On 8/19/19 10:18 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>
> Reviewed-by: John Snow <jsnow@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 3c4f5173ce..50c1e7f2ec 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,
> +            self.vm.add_drive("null-co://",
>                                "throttling.iops-total=100,file.read-zeroes=on")
>          self.vm.launch()
>  
> @@ -376,10 +375,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",
>                               read_zeroes=True, 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",
>                               read_zeroes=True, 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', {})
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Thomas Huth]

On 8/19/19 10:18 PM, Max Reitz wrote:
> 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 | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> index 50c1e7f2ec..f03fa24a07 100755
> --- a/tests/qemu-iotests/093
> +++ b/tests/qemu-iotests/093
> @@ -24,7 +24,7 @@ import iotests
>  nsec_per_sec = 1000000000
>  
>  class ThrottleTestCase(iotests.QMPTestCase):
> -    test_img = "null-aio://"
> +    test_driver = "null-aio"
>      max_drives = 3
>  
>      def blockstats(self, device):
> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>          raise Exception("Device not found for blockstats: %s" % device)
>  
> +    def required_drivers(self):
> +        return [self.test_driver]
> +
> +    @iotests.skip_if_unsupported(required_drivers)
>      def setUp(self):
>          self.vm = iotests.VM()
>          for i in range(0, self.max_drives):
> -            self.vm.add_drive(self.test_img, "file.read-zeroes=on")
> +            self.vm.add_drive(self.test_driver + "://", "file.read-zeroes=on")
>          self.vm.launch()
>  
>      def tearDown(self):
> @@ -264,7 +268,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
> @@ -425,4 +429,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
>  
>  
>  if __name__ == '__main__':
> +    if 'null-co' not in iotests.supported_formats():
> +        iotests.notrun('null-co driver support missing')
>      iotests.main(supported_fmts=["raw"])

Maybe also mention null-co in the patch description?

 Thomas

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2468 bytes --]

On 20.08.19 08:40, Thomas Huth wrote:
> On 8/19/19 10:18 PM, Max Reitz wrote:
>> 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 | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>> index 50c1e7f2ec..f03fa24a07 100755
>> --- a/tests/qemu-iotests/093
>> +++ b/tests/qemu-iotests/093
>> @@ -24,7 +24,7 @@ import iotests
>>  nsec_per_sec = 1000000000
>>  
>>  class ThrottleTestCase(iotests.QMPTestCase):
>> -    test_img = "null-aio://"
>> +    test_driver = "null-aio"
>>      max_drives = 3
>>  
>>      def blockstats(self, device):
>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>>          raise Exception("Device not found for blockstats: %s" % device)
>>  
>> +    def required_drivers(self):
>> +        return [self.test_driver]
>> +
>> +    @iotests.skip_if_unsupported(required_drivers)
>>      def setUp(self):
>>          self.vm = iotests.VM()
>>          for i in range(0, self.max_drives):
>> -            self.vm.add_drive(self.test_img, "file.read-zeroes=on")
>> +            self.vm.add_drive(self.test_driver + "://", "file.read-zeroes=on")
>>          self.vm.launch()
>>  
>>      def tearDown(self):
>> @@ -264,7 +268,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
>> @@ -425,4 +429,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
>>  
>>  
>>  if __name__ == '__main__':
>> +    if 'null-co' not in iotests.supported_formats():
>> +        iotests.notrun('null-co driver support missing')
>>      iotests.main(supported_fmts=["raw"])
> 
> Maybe also mention null-co in the patch description?

I probably didn’t because I felt bad that maybe I should add a null-co
check to all tests that require it...  But two wrongs don’t make a
right, so I’ll leave it at one wrong and put “Skip the whole test if
null-co is not whitelisted.” into the commit message, yes.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 2/8] iotests: Prefer null-co over null-aio

[John Snow]

On 8/19/19 4:22 PM, Max Reitz wrote:
> On 19.08.19 22:18, 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>
>> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> Hm, sorry, I just noticed that I probably should have dropped this R-b. :-/
> 
> (I mean, apart from the rebase conflict, nothing has changed, but still.)
> 
> Max
> 

It's fine.

Re: [Qemu-devel] [PATCH v3 8/8] iotests: Cache supported_formats()

[John Snow]

Any particular reason? I guess an "itch."

On 8/19/19 4:18 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 8f315538e9..f6492b355f 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -884,9 +884,17 @@ def qemu_pipe(*args):
>  def supported_formats(read_only=False):
>      '''Set 'read_only' to True to check ro-whitelist
>         Otherwise, rw-whitelist is checked'''
> -    format_message = qemu_pipe("-drive", "format=help")
> -    line = 1 if read_only else 0
> -    return format_message.splitlines()[line].split(":")[1].split()
> +
> +    if not hasattr(supported_formats, "formats"):
> +        supported_formats.formats = {}
> +
> +    if read_only not in supported_formats.formats:
> +        format_message = qemu_pipe("-drive", "format=help")
> +        line = 1 if read_only else 0
> +        supported_formats.formats[read_only] = \
> +            format_message.splitlines()[line].split(":")[1].split()
> +

Seems a little odd to use a boolean as a dict key, but I guess it works
and is fine.

> +    return supported_formats.formats[read_only]
>  
>  def skip_if_unsupported(required_formats=[], read_only=False):
>      '''Skip Test Decorator
> 

Eh, why not.

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

Re: [Qemu-devel] [PATCH v3 3/8] iotests: Allow skipping test cases

[John Snow]

On 8/19/19 4:18 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.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Hm, didn't someone else send a patch like this recently?

Ah, yes:
[Qemu-block] [PATCH v5 3/6] iotests: Add casenotrun report to bash tests

Oh, theirs is for bash. Moving along.

> ---
>  tests/qemu-iotests/iotests.py | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 84438e837c..2f53baf633 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -802,6 +802,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'''
> @@ -813,7 +818,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])
>  
> @@ -904,8 +912,15 @@ def execute_unittest(output, verbosity, debug):
>          unittest.main(testRunner=runner)
>      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)
>  
>  def execute_test(test_function=None,
>                   supported_fmts=[], supported_oses=['linux'],
> 

okey dokey.

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

Re: [Qemu-devel] [PATCH v3 4/8] iotests: Use case_skip() in skip_if_unsupported()

[John Snow]

On 8/19/19 4:18 PM, Max Reitz wrote:
> skip_if_unsupported() should use the stronger variant case_skip(),
> because this allows it to be used even with setUp() (in a meaningful
> way).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 2f53baf633..726f904f50 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -896,7 +896,7 @@ def skip_if_unsupported(required_formats=[], read_only=False):
>              usf_list = list(set(required_formats) -
>                              set(supported_formats(read_only)))
>              if usf_list:
> -                case_notrun('{}: formats {} are not whitelisted'.format(
> +                args[0].case_skip('{}: formats {} are not whitelisted'.format(
>                      args[0], usf_list))
>              else:
>                  return func(*args, **kwargs)
> 

Should we promote args[0] to a named argument here, because we depend on
it having a specific type? It's not truly as polymorphic as we're making
it appear.

That type here is iotests.QMPTestCase because we're relying on case_skip
being present.

def test_wrapper(test_case, *args, **kwargs):
    ...
        return func(test_case, *args, **kwargs)

--js

Re: [Qemu-devel] [PATCH v3 5/8] iotests: Let skip_if_unsupported() accept a method

[John Snow]

On 8/19/19 4:18 PM, Max Reitz wrote:
> This lets tests use skip_if_unsupported() with a potentially variable
> list of required formats.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/iotests.py | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 726f904f50..8f315538e9 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -893,8 +893,12 @@ def skip_if_unsupported(required_formats=[], read_only=False):
>         Runs the test if all the required formats are whitelisted'''
>      def skip_test_decorator(func):
>          def func_wrapper(*args, **kwargs):
> -            usf_list = list(set(required_formats) -
> -                            set(supported_formats(read_only)))
> +            if callable(required_formats):
> +                fmts = required_formats(args[0])
> +            else:
> +                fmts = required_formats
> +
> +            usf_list = list(set(fmts) - set(supported_formats(read_only)))
>              if usf_list:
>                  args[0].case_skip('{}: formats {} are not whitelisted'.format(
>                      args[0], usf_list))
> 

I am required to inform you that this is in direct violation of the
pythonista treaty of 2007; which mandates that you try to call and fail
instead of attempting to gracefully check ahead of time.

Luckily, I am not fond of such rules.

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

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[John Snow]

On 8/19/19 4:18 PM, Max Reitz wrote:
> 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 | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> index 50c1e7f2ec..f03fa24a07 100755
> --- a/tests/qemu-iotests/093
> +++ b/tests/qemu-iotests/093
> @@ -24,7 +24,7 @@ import iotests
>  nsec_per_sec = 1000000000
>  
>  class ThrottleTestCase(iotests.QMPTestCase):
> -    test_img = "null-aio://"
> +    test_driver = "null-aio"
>      max_drives = 3
>  
>      def blockstats(self, device):
> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>          raise Exception("Device not found for blockstats: %s" % device)
>  
> +    def required_drivers(self):
> +        return [self.test_driver]
> +
> +    @iotests.skip_if_unsupported(required_drivers)

Oh, I see why you're passing args[0] back to the callback now. Why not
just pass self.required_drivers and call it with no arguments instead?

You can get a bound version that way that doesn't need additional
arguments, and then the callback is free to take generic callables of
any kind.

>      def setUp(self):
>          self.vm = iotests.VM()
>          for i in range(0, self.max_drives):
> -            self.vm.add_drive(self.test_img, "file.read-zeroes=on")
> +            self.vm.add_drive(self.test_driver + "://", "file.read-zeroes=on")
>          self.vm.launch()
>  
>      def tearDown(self):
> @@ -264,7 +268,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
> @@ -425,4 +429,6 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
>  
>  
>  if __name__ == '__main__':
> +    if 'null-co' not in iotests.supported_formats():
> +        iotests.notrun('null-co driver support missing')
>      iotests.main(supported_fmts=["raw"])
> 

Re: [Qemu-devel] [PATCH v3 4/8] iotests: Use case_skip() in skip_if_unsupported()

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1675 bytes --]

On 20.08.19 23:27, John Snow wrote:
> 
> 
> On 8/19/19 4:18 PM, Max Reitz wrote:
>> skip_if_unsupported() should use the stronger variant case_skip(),
>> because this allows it to be used even with setUp() (in a meaningful
>> way).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/iotests.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 2f53baf633..726f904f50 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -896,7 +896,7 @@ def skip_if_unsupported(required_formats=[], read_only=False):
>>              usf_list = list(set(required_formats) -
>>                              set(supported_formats(read_only)))
>>              if usf_list:
>> -                case_notrun('{}: formats {} are not whitelisted'.format(
>> +                args[0].case_skip('{}: formats {} are not whitelisted'.format(
>>                      args[0], usf_list))
>>              else:
>>                  return func(*args, **kwargs)
>>
> 
> Should we promote args[0] to a named argument here, because we depend on
> it having a specific type? It's not truly as polymorphic as we're making
> it appear.
> 
> That type here is iotests.QMPTestCase because we're relying on case_skip
> being present.
> 
> def test_wrapper(test_case, *args, **kwargs):
>     ...
>         return func(test_case, *args, **kwargs)

That sounds good to me indeed.

(I didn’t feel too bad about it because we already use args[0] for the
skip reason, but it really should be named, yes.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 5/8] iotests: Let skip_if_unsupported() accept a method

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1790 bytes --]

On 20.08.19 23:31, John Snow wrote:
> 
> 
> On 8/19/19 4:18 PM, Max Reitz wrote:
>> This lets tests use skip_if_unsupported() with a potentially variable
>> list of required formats.
>>
>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  tests/qemu-iotests/iotests.py | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 726f904f50..8f315538e9 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -893,8 +893,12 @@ def skip_if_unsupported(required_formats=[], read_only=False):
>>         Runs the test if all the required formats are whitelisted'''
>>      def skip_test_decorator(func):
>>          def func_wrapper(*args, **kwargs):
>> -            usf_list = list(set(required_formats) -
>> -                            set(supported_formats(read_only)))
>> +            if callable(required_formats):
>> +                fmts = required_formats(args[0])
>> +            else:
>> +                fmts = required_formats
>> +
>> +            usf_list = list(set(fmts) - set(supported_formats(read_only)))
>>              if usf_list:
>>                  args[0].case_skip('{}: formats {} are not whitelisted'.format(
>>                      args[0], usf_list))
>>
> 
> I am required to inform you that this is in direct violation of the
> pythonista treaty of 2007; which mandates that you try to call and fail
> instead of attempting to gracefully check ahead of time.
> 
> Luckily, I am not fond of such rules.

:-)

I blame Kevin’s proposal.  (We should always have someone on PTO to
blame for everything.)


Thanks for reviewing.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1506 bytes --]

On 20.08.19 23:32, John Snow wrote:
> 
> 
> On 8/19/19 4:18 PM, Max Reitz wrote:
>> 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 | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>> index 50c1e7f2ec..f03fa24a07 100755
>> --- a/tests/qemu-iotests/093
>> +++ b/tests/qemu-iotests/093
>> @@ -24,7 +24,7 @@ import iotests
>>  nsec_per_sec = 1000000000
>>  
>>  class ThrottleTestCase(iotests.QMPTestCase):
>> -    test_img = "null-aio://"
>> +    test_driver = "null-aio"
>>      max_drives = 3
>>  
>>      def blockstats(self, device):
>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>>          raise Exception("Device not found for blockstats: %s" % device)
>>  
>> +    def required_drivers(self):
>> +        return [self.test_driver]
>> +
>> +    @iotests.skip_if_unsupported(required_drivers)
> 
> Oh, I see why you're passing args[0] back to the callback now. Why not
> just pass self.required_drivers and call it with no arguments instead?
> 
> You can get a bound version that way that doesn't need additional
> arguments, and then the callback is free to take generic callables of
> any kind.

That would be nicer indeed.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 3/8] iotests: Allow skipping test cases

[Andrey Shinkevich]

On 19/08/2019 23:18, 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.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 21 ++++++++++++++++++---
>   1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 84438e837c..2f53baf633 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -802,6 +802,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'''
> @@ -813,7 +818,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()

The clause "do not actually skip it" sounds like a prescription. I would 
like the comment to be clearer for a reader that the method is a 
notifier only.

Andrey

> +    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])
>   
> @@ -904,8 +912,15 @@ def execute_unittest(output, verbosity, debug):
>           unittest.main(testRunner=runner)
>       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)
>   
>   def execute_test(test_function=None,
>                    supported_fmts=[], supported_oses=['linux'],
> 

Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1803 bytes --]

On 20.08.19 23:32, John Snow wrote:
> 
> 
> On 8/19/19 4:18 PM, Max Reitz wrote:
>> 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 | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>> index 50c1e7f2ec..f03fa24a07 100755
>> --- a/tests/qemu-iotests/093
>> +++ b/tests/qemu-iotests/093
>> @@ -24,7 +24,7 @@ import iotests
>>  nsec_per_sec = 1000000000
>>  
>>  class ThrottleTestCase(iotests.QMPTestCase):
>> -    test_img = "null-aio://"
>> +    test_driver = "null-aio"
>>      max_drives = 3
>>  
>>      def blockstats(self, device):
>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>>          raise Exception("Device not found for blockstats: %s" % device)
>>  
>> +    def required_drivers(self):
>> +        return [self.test_driver]
>> +
>> +    @iotests.skip_if_unsupported(required_drivers)
> 
> Oh, I see why you're passing args[0] back to the callback now. Why not
> just pass self.required_drivers and call it with no arguments instead?
> 
> You can get a bound version that way that doesn't need additional
> arguments, and then the callback is free to take generic callables of
> any kind.

Am I doing something wrong?

I just get

+Traceback (most recent call last):
+  File "093", line 26, in <module>
+    class ThrottleTestCase(iotests.QMPTestCase):
+  File "093", line 41, in ThrottleTestCase
+    @iotests.skip_if_unsupported(self.required_drivers)
+NameError: name 'self' is not defined

this way.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[John Snow]

On 9/13/19 8:47 AM, Max Reitz wrote:
> On 20.08.19 23:32, John Snow wrote:
>>
>>
>> On 8/19/19 4:18 PM, Max Reitz wrote:
>>> 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 | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>>> index 50c1e7f2ec..f03fa24a07 100755
>>> --- a/tests/qemu-iotests/093
>>> +++ b/tests/qemu-iotests/093
>>> @@ -24,7 +24,7 @@ import iotests
>>>  nsec_per_sec = 1000000000
>>>  
>>>  class ThrottleTestCase(iotests.QMPTestCase):
>>> -    test_img = "null-aio://"
>>> +    test_driver = "null-aio"
>>>      max_drives = 3
>>>  
>>>      def blockstats(self, device):
>>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>>>          raise Exception("Device not found for blockstats: %s" % device)
>>>  
>>> +    def required_drivers(self):
>>> +        return [self.test_driver]
>>> +
>>> +    @iotests.skip_if_unsupported(required_drivers)
>>
>> Oh, I see why you're passing args[0] back to the callback now. Why not
>> just pass self.required_drivers and call it with no arguments instead?
>>
>> You can get a bound version that way that doesn't need additional
>> arguments, and then the callback is free to take generic callables of
>> any kind.
> 
> Am I doing something wrong?
> 
> I just get
> 
> +Traceback (most recent call last):
> +  File "093", line 26, in <module>
> +    class ThrottleTestCase(iotests.QMPTestCase):
> +  File "093", line 41, in ThrottleTestCase
> +    @iotests.skip_if_unsupported(self.required_drivers)
> +NameError: name 'self' is not defined
> 
> this way.
> 
> Max
> 
What was I even talking about? :\ Well.

I'd still like to define func_wrapper with a nod to the type constraint
it has:

def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
    [...]


Then, you'd write:

if callable(required_formats):
    fmts = required_formats(instance)
else:
    fmts = required_formats


And:

> +    def required_drivers(self):
> +        return [self.test_driver]
> +
> +    @iotests.skip_if_unsupported(required_drivers)
>      def setUp(self):

The problem is that 'self' isn't defined at the class level, so I was
mistaken about being able to use it :( Python does not have a notion of
a lexical constant to refer to the class being defined; and of course we
do not have an instance variable at definition time.

Sorry for the wild goose chase.

It's fine as-is.

(I wanted a way to define the required_formats callback without forcing
it to be a class method, but the decorator code runs at definition time
and we just don't HAVE the instance; so the way you wrote it is I think
the only way it CAN be written without some much nastier trickery.)

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2531 bytes --]

On 13.09.19 20:30, John Snow wrote:
> 
> 
> On 9/13/19 8:47 AM, Max Reitz wrote:
>> On 20.08.19 23:32, John Snow wrote:
>>>
>>>
>>> On 8/19/19 4:18 PM, Max Reitz wrote:
>>>> 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 | 12 +++++++++---
>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>>>> index 50c1e7f2ec..f03fa24a07 100755
>>>> --- a/tests/qemu-iotests/093
>>>> +++ b/tests/qemu-iotests/093
>>>> @@ -24,7 +24,7 @@ import iotests
>>>>  nsec_per_sec = 1000000000
>>>>  
>>>>  class ThrottleTestCase(iotests.QMPTestCase):
>>>> -    test_img = "null-aio://"
>>>> +    test_driver = "null-aio"
>>>>      max_drives = 3
>>>>  
>>>>      def blockstats(self, device):
>>>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>>>>          raise Exception("Device not found for blockstats: %s" % device)
>>>>  
>>>> +    def required_drivers(self):
>>>> +        return [self.test_driver]
>>>> +
>>>> +    @iotests.skip_if_unsupported(required_drivers)
>>>
>>> Oh, I see why you're passing args[0] back to the callback now. Why not
>>> just pass self.required_drivers and call it with no arguments instead?
>>>
>>> You can get a bound version that way that doesn't need additional
>>> arguments, and then the callback is free to take generic callables of
>>> any kind.
>>
>> Am I doing something wrong?
>>
>> I just get
>>
>> +Traceback (most recent call last):
>> +  File "093", line 26, in <module>
>> +    class ThrottleTestCase(iotests.QMPTestCase):
>> +  File "093", line 41, in ThrottleTestCase
>> +    @iotests.skip_if_unsupported(self.required_drivers)
>> +NameError: name 'self' is not defined
>>
>> this way.
>>
>> Max
>>
> What was I even talking about? :\ Well.
> 
> I'd still like to define func_wrapper with a nod to the type constraint
> it has:
> 
> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
>     [...]
> 
> 
> Then, you'd write:
> 
> if callable(required_formats):
>     fmts = required_formats(instance)
> else:
>     fmts = required_formats

Yep, that anyway.  (Although I didn’t know about the “param: type”
syntax and put that constraint in a comment instead.  Thanks again :-))

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2800 bytes --]

On 17.09.19 10:18, Max Reitz wrote:
> On 13.09.19 20:30, John Snow wrote:
>>
>>
>> On 9/13/19 8:47 AM, Max Reitz wrote:
>>> On 20.08.19 23:32, John Snow wrote:
>>>>
>>>>
>>>> On 8/19/19 4:18 PM, Max Reitz wrote:
>>>>> 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 | 12 +++++++++---
>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>>>>> index 50c1e7f2ec..f03fa24a07 100755
>>>>> --- a/tests/qemu-iotests/093
>>>>> +++ b/tests/qemu-iotests/093
>>>>> @@ -24,7 +24,7 @@ import iotests
>>>>>  nsec_per_sec = 1000000000
>>>>>  
>>>>>  class ThrottleTestCase(iotests.QMPTestCase):
>>>>> -    test_img = "null-aio://"
>>>>> +    test_driver = "null-aio"
>>>>>      max_drives = 3
>>>>>  
>>>>>      def blockstats(self, device):
>>>>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>>>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>>>>>          raise Exception("Device not found for blockstats: %s" % device)
>>>>>  
>>>>> +    def required_drivers(self):
>>>>> +        return [self.test_driver]
>>>>> +
>>>>> +    @iotests.skip_if_unsupported(required_drivers)
>>>>
>>>> Oh, I see why you're passing args[0] back to the callback now. Why not
>>>> just pass self.required_drivers and call it with no arguments instead?
>>>>
>>>> You can get a bound version that way that doesn't need additional
>>>> arguments, and then the callback is free to take generic callables of
>>>> any kind.
>>>
>>> Am I doing something wrong?
>>>
>>> I just get
>>>
>>> +Traceback (most recent call last):
>>> +  File "093", line 26, in <module>
>>> +    class ThrottleTestCase(iotests.QMPTestCase):
>>> +  File "093", line 41, in ThrottleTestCase
>>> +    @iotests.skip_if_unsupported(self.required_drivers)
>>> +NameError: name 'self' is not defined
>>>
>>> this way.
>>>
>>> Max
>>>
>> What was I even talking about? :\ Well.
>>
>> I'd still like to define func_wrapper with a nod to the type constraint
>> it has:
>>
>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
>>     [...]
>>
>>
>> Then, you'd write:
>>
>> if callable(required_formats):
>>     fmts = required_formats(instance)
>> else:
>>     fmts = required_formats
> 
> Yep, that anyway.  (Although I didn’t know about the “param: type”
> syntax and put that constraint in a comment instead.  Thanks again :-))

Ah, but it isn’t a constraint, but just a “type hint” and the
interpreter doesn’t enforce it.  How quaint.

Well, better than a comment anyway.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3016 bytes --]

On 17.09.19 10:29, Max Reitz wrote:
> On 17.09.19 10:18, Max Reitz wrote:
>> On 13.09.19 20:30, John Snow wrote:
>>>
>>>
>>> On 9/13/19 8:47 AM, Max Reitz wrote:
>>>> On 20.08.19 23:32, John Snow wrote:
>>>>>
>>>>>
>>>>> On 8/19/19 4:18 PM, Max Reitz wrote:
>>>>>> 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 | 12 +++++++++---
>>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>>>>>> index 50c1e7f2ec..f03fa24a07 100755
>>>>>> --- a/tests/qemu-iotests/093
>>>>>> +++ b/tests/qemu-iotests/093
>>>>>> @@ -24,7 +24,7 @@ import iotests
>>>>>>  nsec_per_sec = 1000000000
>>>>>>  
>>>>>>  class ThrottleTestCase(iotests.QMPTestCase):
>>>>>> -    test_img = "null-aio://"
>>>>>> +    test_driver = "null-aio"
>>>>>>      max_drives = 3
>>>>>>  
>>>>>>      def blockstats(self, device):
>>>>>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>>>>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>>>>>>          raise Exception("Device not found for blockstats: %s" % device)
>>>>>>  
>>>>>> +    def required_drivers(self):
>>>>>> +        return [self.test_driver]
>>>>>> +
>>>>>> +    @iotests.skip_if_unsupported(required_drivers)
>>>>>
>>>>> Oh, I see why you're passing args[0] back to the callback now. Why not
>>>>> just pass self.required_drivers and call it with no arguments instead?
>>>>>
>>>>> You can get a bound version that way that doesn't need additional
>>>>> arguments, and then the callback is free to take generic callables of
>>>>> any kind.
>>>>
>>>> Am I doing something wrong?
>>>>
>>>> I just get
>>>>
>>>> +Traceback (most recent call last):
>>>> +  File "093", line 26, in <module>
>>>> +    class ThrottleTestCase(iotests.QMPTestCase):
>>>> +  File "093", line 41, in ThrottleTestCase
>>>> +    @iotests.skip_if_unsupported(self.required_drivers)
>>>> +NameError: name 'self' is not defined
>>>>
>>>> this way.
>>>>
>>>> Max
>>>>
>>> What was I even talking about? :\ Well.
>>>
>>> I'd still like to define func_wrapper with a nod to the type constraint
>>> it has:
>>>
>>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
>>>     [...]
>>>
>>>
>>> Then, you'd write:
>>>
>>> if callable(required_formats):
>>>     fmts = required_formats(instance)
>>> else:
>>>     fmts = required_formats
>>
>> Yep, that anyway.  (Although I didn’t know about the “param: type”
>> syntax and put that constraint in a comment instead.  Thanks again :-))
> 
> Ah, but it isn’t a constraint, but just a “type hint” and the
> interpreter doesn’t enforce it.  How quaint.

Oh, and they were introduced only in 3.5.  Maybe that’s a tad too new.

> Well, better than a comment anyway.

So maybe not.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Kevin Wolf]

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

Am 17.09.2019 um 10:18 hat Max Reitz geschrieben:
> On 13.09.19 20:30, John Snow wrote:
> > 
> > 
> > On 9/13/19 8:47 AM, Max Reitz wrote:
> >> On 20.08.19 23:32, John Snow wrote:
> >>>
> >>>
> >>> On 8/19/19 4:18 PM, Max Reitz wrote:
> >>>> 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 | 12 +++++++++---
> >>>>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> >>>> index 50c1e7f2ec..f03fa24a07 100755
> >>>> --- a/tests/qemu-iotests/093
> >>>> +++ b/tests/qemu-iotests/093
> >>>> @@ -24,7 +24,7 @@ import iotests
> >>>>  nsec_per_sec = 1000000000
> >>>>  
> >>>>  class ThrottleTestCase(iotests.QMPTestCase):
> >>>> -    test_img = "null-aio://"
> >>>> +    test_driver = "null-aio"
> >>>>      max_drives = 3
> >>>>  
> >>>>      def blockstats(self, device):
> >>>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
> >>>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
> >>>>          raise Exception("Device not found for blockstats: %s" % device)
> >>>>  
> >>>> +    def required_drivers(self):
> >>>> +        return [self.test_driver]
> >>>> +
> >>>> +    @iotests.skip_if_unsupported(required_drivers)
> >>>
> >>> Oh, I see why you're passing args[0] back to the callback now. Why not
> >>> just pass self.required_drivers and call it with no arguments instead?
> >>>
> >>> You can get a bound version that way that doesn't need additional
> >>> arguments, and then the callback is free to take generic callables of
> >>> any kind.
> >>
> >> Am I doing something wrong?
> >>
> >> I just get
> >>
> >> +Traceback (most recent call last):
> >> +  File "093", line 26, in <module>
> >> +    class ThrottleTestCase(iotests.QMPTestCase):
> >> +  File "093", line 41, in ThrottleTestCase
> >> +    @iotests.skip_if_unsupported(self.required_drivers)
> >> +NameError: name 'self' is not defined
> >>
> >> this way.
> >>
> >> Max
> >>
> > What was I even talking about? :\ Well.
> > 
> > I'd still like to define func_wrapper with a nod to the type constraint
> > it has:
> > 
> > def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
> >     [...]
> > 
> > 
> > Then, you'd write:
> > 
> > if callable(required_formats):
> >     fmts = required_formats(instance)
> > else:
> >     fmts = required_formats
> 
> Yep, that anyway.  (Although I didn’t know about the “param: type”
> syntax and put that constraint in a comment instead.  Thanks again :-))

Note that function annotations are Python 3 only, so we can't use that
syntax yet anyway. If you want to use type hints that are understood by
tools (like mypy) and compatible with Python 2, you have to use
something like this (feel free to be more specific than Any):

    def func_wrapper(instance, *args, **kwargs):
        # type: (iotests.QMPTestCase, Any, Any) -> None
        [...]

Or if you only want to declare the type for one parameter:

    def func_wrapper(instance,  # type: iotests.QMPTestCase
                     *args,
                     **kwargs):
        [...]

Especially the latter syntax might be sufficiently ugly that just doing
a free-form comment that can't be parsed by tools is justifiable.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 3125 bytes --]

On 17.09.19 10:40, Kevin Wolf wrote:
> Am 17.09.2019 um 10:18 hat Max Reitz geschrieben:
>> On 13.09.19 20:30, John Snow wrote:
>>>
>>>
>>> On 9/13/19 8:47 AM, Max Reitz wrote:
>>>> On 20.08.19 23:32, John Snow wrote:
>>>>>
>>>>>
>>>>> On 8/19/19 4:18 PM, Max Reitz wrote:
>>>>>> 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 | 12 +++++++++---
>>>>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
>>>>>> index 50c1e7f2ec..f03fa24a07 100755
>>>>>> --- a/tests/qemu-iotests/093
>>>>>> +++ b/tests/qemu-iotests/093
>>>>>> @@ -24,7 +24,7 @@ import iotests
>>>>>>  nsec_per_sec = 1000000000
>>>>>>  
>>>>>>  class ThrottleTestCase(iotests.QMPTestCase):
>>>>>> -    test_img = "null-aio://"
>>>>>> +    test_driver = "null-aio"
>>>>>>      max_drives = 3
>>>>>>  
>>>>>>      def blockstats(self, device):
>>>>>> @@ -35,10 +35,14 @@ class ThrottleTestCase(iotests.QMPTestCase):
>>>>>>                  return stat['rd_bytes'], stat['rd_operations'], stat['wr_bytes'], stat['wr_operations']
>>>>>>          raise Exception("Device not found for blockstats: %s" % device)
>>>>>>  
>>>>>> +    def required_drivers(self):
>>>>>> +        return [self.test_driver]
>>>>>> +
>>>>>> +    @iotests.skip_if_unsupported(required_drivers)
>>>>>
>>>>> Oh, I see why you're passing args[0] back to the callback now. Why not
>>>>> just pass self.required_drivers and call it with no arguments instead?
>>>>>
>>>>> You can get a bound version that way that doesn't need additional
>>>>> arguments, and then the callback is free to take generic callables of
>>>>> any kind.
>>>>
>>>> Am I doing something wrong?
>>>>
>>>> I just get
>>>>
>>>> +Traceback (most recent call last):
>>>> +  File "093", line 26, in <module>
>>>> +    class ThrottleTestCase(iotests.QMPTestCase):
>>>> +  File "093", line 41, in ThrottleTestCase
>>>> +    @iotests.skip_if_unsupported(self.required_drivers)
>>>> +NameError: name 'self' is not defined
>>>>
>>>> this way.
>>>>
>>>> Max
>>>>
>>> What was I even talking about? :\ Well.
>>>
>>> I'd still like to define func_wrapper with a nod to the type constraint
>>> it has:
>>>
>>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
>>>     [...]
>>>
>>>
>>> Then, you'd write:
>>>
>>> if callable(required_formats):
>>>     fmts = required_formats(instance)
>>> else:
>>>     fmts = required_formats
>>
>> Yep, that anyway.  (Although I didn’t know about the “param: type”
>> syntax and put that constraint in a comment instead.  Thanks again :-))
> 
> Note that function annotations are Python 3 only, so we can't use that
> syntax yet anyway. If you want to use type hints that are understood by
> tools (like mypy) and compatible with Python 2, you have to use
> something like this (feel free to be more specific than Any):

Do we really feel like staying compatible with Python 2, though?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Kevin Wolf]

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

Am 17.09.2019 um 13:07 hat Max Reitz geschrieben:
> On 17.09.19 10:40, Kevin Wolf wrote:
> > Am 17.09.2019 um 10:18 hat Max Reitz geschrieben:
> >> On 13.09.19 20:30, John Snow wrote:
> >>> I'd still like to define func_wrapper with a nod to the type constraint
> >>> it has:
> >>>
> >>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
> >>>     [...]
> >>>
> >>>
> >>> Then, you'd write:
> >>>
> >>> if callable(required_formats):
> >>>     fmts = required_formats(instance)
> >>> else:
> >>>     fmts = required_formats
> >>
> >> Yep, that anyway.  (Although I didn’t know about the “param: type”
> >> syntax and put that constraint in a comment instead.  Thanks again :-))
> > 
> > Note that function annotations are Python 3 only, so we can't use that
> > syntax yet anyway. If you want to use type hints that are understood by
> > tools (like mypy) and compatible with Python 2, you have to use
> > something like this (feel free to be more specific than Any):
> 
> Do we really feel like staying compatible with Python 2, though?

Feel like it? No.

It's more that we are compelled to do so because we only deprecated it
in 4.1.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[John Snow]

On 9/17/19 7:22 AM, Kevin Wolf wrote:
> Am 17.09.2019 um 13:07 hat Max Reitz geschrieben:
>> On 17.09.19 10:40, Kevin Wolf wrote:
>>> Am 17.09.2019 um 10:18 hat Max Reitz geschrieben:
>>>> On 13.09.19 20:30, John Snow wrote:
>>>>> I'd still like to define func_wrapper with a nod to the type constraint
>>>>> it has:
>>>>>
>>>>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
>>>>>     [...]
>>>>>
>>>>>
>>>>> Then, you'd write:
>>>>>
>>>>> if callable(required_formats):
>>>>>     fmts = required_formats(instance)
>>>>> else:
>>>>>     fmts = required_formats
>>>>
>>>> Yep, that anyway.  (Although I didn’t know about the “param: type”
>>>> syntax and put that constraint in a comment instead.  Thanks again :-))
>>>
>>> Note that function annotations are Python 3 only, so we can't use that
>>> syntax yet anyway. If you want to use type hints that are understood by
>>> tools (like mypy) and compatible with Python 2, you have to use
>>> something like this (feel free to be more specific than Any):
>>
>> Do we really feel like staying compatible with Python 2, though?
> 
> Feel like it? No.
> 
> It's more that we are compelled to do so because we only deprecated it
> in 4.1.
> 
> Kevin
> 

Sorry for the impromptu lesson on type hints in 3.5! I added that in to
my suggestion as a demonstrative example and didn't mean for you to use
it as-is, sorry for not making that clear.

I'm confused about the Python3 deprecation timeline. Normally we'd
follow our standard approach, but it does hit EOL at the end of this
year, so do we drop support then, too? I have the memory of a goldfish I
suppose, and can't quite remember our conclusions, if any, of previous
discussions on this subject.

If we do drop python2 though, the new minimum version appears to be 3.5
because that's what ships in EPEL. That'd give us standardized type
hints that we can use for static analysis tools.

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Kevin Wolf]

Am 17.09.2019 um 15:09 hat John Snow geschrieben:
> On 9/17/19 7:22 AM, Kevin Wolf wrote:
> > Am 17.09.2019 um 13:07 hat Max Reitz geschrieben:
> >> On 17.09.19 10:40, Kevin Wolf wrote:
> >>> Am 17.09.2019 um 10:18 hat Max Reitz geschrieben:
> >>>> On 13.09.19 20:30, John Snow wrote:
> >>>>> I'd still like to define func_wrapper with a nod to the type constraint
> >>>>> it has:
> >>>>>
> >>>>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
> >>>>>     [...]
> >>>>>
> >>>>>
> >>>>> Then, you'd write:
> >>>>>
> >>>>> if callable(required_formats):
> >>>>>     fmts = required_formats(instance)
> >>>>> else:
> >>>>>     fmts = required_formats
> >>>>
> >>>> Yep, that anyway.  (Although I didn’t know about the “param: type”
> >>>> syntax and put that constraint in a comment instead.  Thanks again :-))
> >>>
> >>> Note that function annotations are Python 3 only, so we can't use that
> >>> syntax yet anyway. If you want to use type hints that are understood by
> >>> tools (like mypy) and compatible with Python 2, you have to use
> >>> something like this (feel free to be more specific than Any):
> >>
> >> Do we really feel like staying compatible with Python 2, though?
> > 
> > Feel like it? No.
> > 
> > It's more that we are compelled to do so because we only deprecated it
> > in 4.1.
> 
> Sorry for the impromptu lesson on type hints in 3.5! I added that in to
> my suggestion as a demonstrative example and didn't mean for you to use
> it as-is, sorry for not making that clear.
> 
> I'm confused about the Python3 deprecation timeline. Normally we'd
> follow our standard approach, but it does hit EOL at the end of this
> year, so do we drop support then, too? I have the memory of a goldfish I
> suppose, and can't quite remember our conclusions, if any, of previous
> discussions on this subject.

It shouldn't make a difference actually because deprecation in 4.1 means
that 4.2 (in December) will be the last release that must still support
Python 2, and we can switch to Python 3 for 5.0.

> If we do drop python2 though, the new minimum version appears to be 3.5
> because that's what ships in EPEL. That'd give us standardized type
> hints that we can use for static analysis tools.

Actually I seem to remember I suggested that we should make 3.5 the
minimum Python 3 version, and I thought a patch to this effect had been
merged, but now I can't find any such check in configure. Maybe I should
find the old thread again to see if there was any reason not to do this.

Personally, I would have preferred 3.6 because it brings in variable
annotations, but I think last time the conclusion was that it would be
3.5 indeed.

Kevin

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[John Snow]

On 9/17/19 9:42 AM, Kevin Wolf wrote:
> Am 17.09.2019 um 15:09 hat John Snow geschrieben:
>> On 9/17/19 7:22 AM, Kevin Wolf wrote:
>>> Am 17.09.2019 um 13:07 hat Max Reitz geschrieben:
>>>> On 17.09.19 10:40, Kevin Wolf wrote:
>>>>> Am 17.09.2019 um 10:18 hat Max Reitz geschrieben:
>>>>>> On 13.09.19 20:30, John Snow wrote:
>>>>>>> I'd still like to define func_wrapper with a nod to the type constraint
>>>>>>> it has:
>>>>>>>
>>>>>>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
>>>>>>>     [...]
>>>>>>>
>>>>>>>
>>>>>>> Then, you'd write:
>>>>>>>
>>>>>>> if callable(required_formats):
>>>>>>>     fmts = required_formats(instance)
>>>>>>> else:
>>>>>>>     fmts = required_formats
>>>>>>
>>>>>> Yep, that anyway.  (Although I didn’t know about the “param: type”
>>>>>> syntax and put that constraint in a comment instead.  Thanks again :-))
>>>>>
>>>>> Note that function annotations are Python 3 only, so we can't use that
>>>>> syntax yet anyway. If you want to use type hints that are understood by
>>>>> tools (like mypy) and compatible with Python 2, you have to use
>>>>> something like this (feel free to be more specific than Any):
>>>>
>>>> Do we really feel like staying compatible with Python 2, though?
>>>
>>> Feel like it? No.
>>>
>>> It's more that we are compelled to do so because we only deprecated it
>>> in 4.1.
>>
>> Sorry for the impromptu lesson on type hints in 3.5! I added that in to
>> my suggestion as a demonstrative example and didn't mean for you to use
>> it as-is, sorry for not making that clear.
>>
>> I'm confused about the Python3 deprecation timeline. Normally we'd
>> follow our standard approach, but it does hit EOL at the end of this
>> year, so do we drop support then, too? I have the memory of a goldfish I
>> suppose, and can't quite remember our conclusions, if any, of previous
>> discussions on this subject.
> 
> It shouldn't make a difference actually because deprecation in 4.1 means
> that 4.2 (in December) will be the last release that must still support
> Python 2, and we can switch to Python 3 for 5.0.
> 
>> If we do drop python2 though, the new minimum version appears to be 3.5
>> because that's what ships in EPEL. That'd give us standardized type
>> hints that we can use for static analysis tools.
> 
> Actually I seem to remember I suggested that we should make 3.5 the
> minimum Python 3 version, and I thought a patch to this effect had been
> merged, but now I can't find any such check in configure. Maybe I should
> find the old thread again to see if there was any reason not to do this.
> 
> Personally, I would have preferred 3.6 because it brings in variable
> annotations, but I think last time the conclusion was that it would be
> 3.5 indeed.
> 

And with variable annotations you get data classes too, I believe, which
are quite handy.

Oh well.

I have a patch that replaces the configure checks, but there are a few
places in docker and the EDK2 build where we require python2 still, so I
have a few threads to chase down before proposing a patch.

Stay tuned, I guess.

--js

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Kevin Wolf]

Am 17.09.2019 um 15:44 hat John Snow geschrieben:
> 
> 
> On 9/17/19 9:42 AM, Kevin Wolf wrote:
> > Am 17.09.2019 um 15:09 hat John Snow geschrieben:
> >> On 9/17/19 7:22 AM, Kevin Wolf wrote:
> >>> Am 17.09.2019 um 13:07 hat Max Reitz geschrieben:
> >>>> On 17.09.19 10:40, Kevin Wolf wrote:
> >>>>> Am 17.09.2019 um 10:18 hat Max Reitz geschrieben:
> >>>>>> On 13.09.19 20:30, John Snow wrote:
> >>>>>>> I'd still like to define func_wrapper with a nod to the type constraint
> >>>>>>> it has:
> >>>>>>>
> >>>>>>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
> >>>>>>>     [...]
> >>>>>>>
> >>>>>>>
> >>>>>>> Then, you'd write:
> >>>>>>>
> >>>>>>> if callable(required_formats):
> >>>>>>>     fmts = required_formats(instance)
> >>>>>>> else:
> >>>>>>>     fmts = required_formats
> >>>>>>
> >>>>>> Yep, that anyway.  (Although I didn’t know about the “param: type”
> >>>>>> syntax and put that constraint in a comment instead.  Thanks again :-))
> >>>>>
> >>>>> Note that function annotations are Python 3 only, so we can't use that
> >>>>> syntax yet anyway. If you want to use type hints that are understood by
> >>>>> tools (like mypy) and compatible with Python 2, you have to use
> >>>>> something like this (feel free to be more specific than Any):
> >>>>
> >>>> Do we really feel like staying compatible with Python 2, though?
> >>>
> >>> Feel like it? No.
> >>>
> >>> It's more that we are compelled to do so because we only deprecated it
> >>> in 4.1.
> >>
> >> Sorry for the impromptu lesson on type hints in 3.5! I added that in to
> >> my suggestion as a demonstrative example and didn't mean for you to use
> >> it as-is, sorry for not making that clear.
> >>
> >> I'm confused about the Python3 deprecation timeline. Normally we'd
> >> follow our standard approach, but it does hit EOL at the end of this
> >> year, so do we drop support then, too? I have the memory of a goldfish I
> >> suppose, and can't quite remember our conclusions, if any, of previous
> >> discussions on this subject.
> > 
> > It shouldn't make a difference actually because deprecation in 4.1 means
> > that 4.2 (in December) will be the last release that must still support
> > Python 2, and we can switch to Python 3 for 5.0.
> > 
> >> If we do drop python2 though, the new minimum version appears to be 3.5
> >> because that's what ships in EPEL. That'd give us standardized type
> >> hints that we can use for static analysis tools.
> > 
> > Actually I seem to remember I suggested that we should make 3.5 the
> > minimum Python 3 version, and I thought a patch to this effect had been
> > merged, but now I can't find any such check in configure. Maybe I should
> > find the old thread again to see if there was any reason not to do this.
> > 
> > Personally, I would have preferred 3.6 because it brings in variable
> > annotations, but I think last time the conclusion was that it would be
> > 3.5 indeed.
> > 
> 
> And with variable annotations you get data classes too, I believe, which
> are quite handy.

I didn't know these. Looks convenient, only in 3.7, though. We might be
there in five years.

Kevin

Re: [Qemu-devel] [PATCH v3 6/8] iotests: Test driver whitelisting in 093

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1285 bytes --]

On 17.09.19 13:22, Kevin Wolf wrote:
> Am 17.09.2019 um 13:07 hat Max Reitz geschrieben:
>> On 17.09.19 10:40, Kevin Wolf wrote:
>>> Am 17.09.2019 um 10:18 hat Max Reitz geschrieben:
>>>> On 13.09.19 20:30, John Snow wrote:
>>>>> I'd still like to define func_wrapper with a nod to the type constraint
>>>>> it has:
>>>>>
>>>>> def func_wrapper(instance: iotests.QMPTestCase, *args, **kwargs):
>>>>>     [...]
>>>>>
>>>>>
>>>>> Then, you'd write:
>>>>>
>>>>> if callable(required_formats):
>>>>>     fmts = required_formats(instance)
>>>>> else:
>>>>>     fmts = required_formats
>>>>
>>>> Yep, that anyway.  (Although I didn’t know about the “param: type”
>>>> syntax and put that constraint in a comment instead.  Thanks again :-))
>>>
>>> Note that function annotations are Python 3 only, so we can't use that
>>> syntax yet anyway. If you want to use type hints that are understood by
>>> tools (like mypy) and compatible with Python 2, you have to use
>>> something like this (feel free to be more specific than Any):
>>
>> Do we really feel like staying compatible with Python 2, though?
> 
> Feel like it? No.
> 
> It's more that we are compelled to do so because we only deprecated it
> in 4.1.

Hm, yes, that’s too bad. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]