[PATCH 0/4] iotests/297: Cover tests/

[PATCH 0/4] iotests/297: Cover tests/

[Max Reitz]

Hi,

When reviewing Vladimir’s new addition to tests/, I noticed that 297 so
far does not cover named tests.  That isn’t so good.

This series makes it cover them, and because tests/ is rather sparse at
this point, I decided to also fix up the two tests in there that don’t
pass pylint’s scrutiny yet.  I think it would be nice if we could keep
all of tests/ clean.


Max Reitz (4):
  iotests/297: Drop 169 and 199 from the skip list
  migrate-bitmaps-postcopy-test: Fix pylint warnings
  migrate-bitmaps-test: Fix pylint warnings
  iotests/297: Cover tests/

 tests/qemu-iotests/297                        |  7 ++--
 .../tests/migrate-bitmaps-postcopy-test       |  3 ++
 tests/qemu-iotests/tests/migrate-bitmaps-test | 38 ++++++++++---------
 3 files changed, 27 insertions(+), 21 deletions(-)

-- 
2.29.2

[PATCH 1/4] iotests/297: Drop 169 and 199 from the skip list

[Max Reitz]

169 and 199 have been renamed and moved to tests/ (commit a44be0334be:
"iotests: rename and move 169 and 199 tests"), so we can drop them from
the skip list.

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index a37910b42d..e3244d40a0 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -29,7 +29,7 @@ import iotests
 SKIP_FILES = (
     '030', '040', '041', '044', '045', '055', '056', '057', '065', '093',
     '096', '118', '124', '132', '136', '139', '147', '148', '149',
-    '151', '152', '155', '163', '165', '169', '194', '196', '199', '202',
+    '151', '152', '155', '163', '165', '194', '196', '202',
     '203', '205', '206', '207', '208', '210', '211', '212', '213', '216',
     '218', '219', '222', '224', '228', '234', '235', '236', '237', '238',
     '240', '242', '245', '246', '248', '255', '256', '257', '258', '260',
-- 
2.29.2

Re: [PATCH 1/4] iotests/297: Drop 169 and 199 from the skip list

[Willian Rampazzo]

On Mon, Mar 29, 2021 at 10:28 AM Max Reitz <mreitz@redhat.com> wrote:
>
> 169 and 199 have been renamed and moved to tests/ (commit a44be0334be:
> "iotests: rename and move 169 and 199 tests"), so we can drop them from
> the skip list.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/297 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 1/4] iotests/297: Drop 169 and 199 from the skip list

[Vladimir Sementsov-Ogievskiy]

29.03.2021 16:26, Max Reitz wrote:
> 169 and 199 have been renamed and moved to tests/ (commit a44be0334be:
> "iotests: rename and move 169 and 199 tests"), so we can drop them from
> the skip list.
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>

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

-- 
Best regards,
Vladimir

[PATCH 2/4] migrate-bitmaps-postcopy-test: Fix pylint warnings

[Max Reitz]

pylint complains that discards1_sha256 and all_discards_sha256 are first
set in non-__init__ methods.  Let's make it happy.

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

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
index 584062b412..013e94fc39 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
@@ -76,6 +76,9 @@ def check_bitmaps(vm, count):
 
 
 class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
+    discards1_sha256 = None
+    all_discards_sha256 = None
+
     def tearDown(self):
         if debug:
             self.vm_a_events += self.vm_a.get_qmp_events()
-- 
2.29.2

Re: [PATCH 2/4] migrate-bitmaps-postcopy-test: Fix pylint warnings

[Willian Rampazzo]

On Mon, Mar 29, 2021 at 10:28 AM Max Reitz <mreitz@redhat.com> wrote:
>
> pylint complains that discards1_sha256 and all_discards_sha256 are first
> set in non-__init__ methods.  Let's make it happy.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 3 +++
>  1 file changed, 3 insertions(+)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 2/4] migrate-bitmaps-postcopy-test: Fix pylint warnings

[Vladimir Sementsov-Ogievskiy]

29.03.2021 16:26, Max Reitz wrote:
> pylint complains that discards1_sha256 and all_discards_sha256 are first
> set in non-__init__ methods.  Let's make it happy.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> index 584062b412..013e94fc39 100755
> --- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> +++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
> @@ -76,6 +76,9 @@ def check_bitmaps(vm, count):
>   
>   
>   class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
> +    discards1_sha256 = None
> +    all_discards_sha256 = None
> +
>       def tearDown(self):
>           if debug:
>               self.vm_a_events += self.vm_a.get_qmp_events()
> 

I'd prefer not making them class-variables. I think initializing them in setUp should work (as a lot of other variables are initialized in setUp() and pylint doesn't complain). And better thing is return it together with event_resume from start_postcopy(), as actually it's a kind of result of the function.

-- 
Best regards,
Vladimir

Re: [PATCH 2/4] migrate-bitmaps-postcopy-test: Fix pylint warnings

[Max Reitz]

On 30.03.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
> 29.03.2021 16:26, Max Reitz wrote:
>> pylint complains that discards1_sha256 and all_discards_sha256 are first
>> set in non-__init__ methods.  Let's make it happy.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test 
>> b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
>> index 584062b412..013e94fc39 100755
>> --- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
>> +++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
>> @@ -76,6 +76,9 @@ def check_bitmaps(vm, count):
>>   class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
>> +    discards1_sha256 = None
>> +    all_discards_sha256 = None
>> +
>>       def tearDown(self):
>>           if debug:
>>               self.vm_a_events += self.vm_a.get_qmp_events()
>>
> 
> I'd prefer not making them class-variables. I think initializing them in 
> setUp should work (as a lot of other variables are initialized in 
> setUp() and pylint doesn't complain). And better thing is return it 
> together with event_resume from start_postcopy(), as actually it's a 
> kind of result of the function.

Oh, that sounds good.  Is a list fine, i.e. return (event_resume, 
discards1_sha256, all_discards_sha256)?

(We could also make it an object.  I don’t know what Python prefers. :))

Max

Re: [PATCH 2/4] migrate-bitmaps-postcopy-test: Fix pylint warnings

[Vladimir Sementsov-Ogievskiy]

30.03.2021 20:18, Max Reitz wrote:
> On 30.03.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
>> 29.03.2021 16:26, Max Reitz wrote:
>>> pylint complains that discards1_sha256 and all_discards_sha256 are first
>>> set in non-__init__ methods.  Let's make it happy.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
>>> index 584062b412..013e94fc39 100755
>>> --- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
>>> +++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
>>> @@ -76,6 +76,9 @@ def check_bitmaps(vm, count):
>>>   class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
>>> +    discards1_sha256 = None
>>> +    all_discards_sha256 = None
>>> +
>>>       def tearDown(self):
>>>           if debug:
>>>               self.vm_a_events += self.vm_a.get_qmp_events()
>>>
>>
>> I'd prefer not making them class-variables. I think initializing them in setUp should work (as a lot of other variables are initialized in setUp() and pylint doesn't complain). And better thing is return it together with event_resume from start_postcopy(), as actually it's a kind of result of the function.
> 
> Oh, that sounds good.  Is a list fine, i.e. return (event_resume, discards1_sha256, all_discards_sha256)?
> 

I think list is fine for now. If in future we'll want more logic in this place, we'll refactor it to some object or something.


-- 
Best regards,
Vladimir

[PATCH 3/4] migrate-bitmaps-test: Fix pylint warnings

[Max Reitz]

There are a couple of things pylint takes issue with:
- The "time" import is unused
- The import order (iotests should come last)
- get_bitmap_hash() doesn't use @self and so should be a function
- Semicolons at the end of some lines
- Parentheses after "if"
- Some lines are too long (80 characters instead of 79)
- inject_test_case()'s @name parameter shadows a top-level @name
  variable
- "lambda self: mc(self)" is equivalent to just "mc"
- Always put two empty lines after a function
- f'exec: cat > /dev/null' does not need to be an f-string

Fix them.

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

diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test b/tests/qemu-iotests/tests/migrate-bitmaps-test
index a5c7bc83e0..fb5ffbb8c4 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-test
@@ -20,11 +20,10 @@
 #
 
 import os
-import iotests
-import time
 import itertools
 import operator
 import re
+import iotests
 from iotests import qemu_img, qemu_img_create, Timeout
 
 
@@ -37,6 +36,12 @@ mig_cmd = 'exec: cat > ' + mig_file
 incoming_cmd = 'exec: cat ' + mig_file
 
 
+def get_bitmap_hash(vm):
+    result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
+                    node='drive0', name='bitmap0')
+    return result['return']['sha256']
+
+
 class TestDirtyBitmapMigration(iotests.QMPTestCase):
     def tearDown(self):
         self.vm_a.shutdown()
@@ -62,21 +67,16 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
             params['persistent'] = True
 
         result = vm.qmp('block-dirty-bitmap-add', **params)
-        self.assert_qmp(result, 'return', {});
-
-    def get_bitmap_hash(self, vm):
-        result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
-                        node='drive0', name='bitmap0')
-        return result['return']['sha256']
+        self.assert_qmp(result, 'return', {})
 
     def check_bitmap(self, vm, sha256):
         result = vm.qmp('x-debug-block-dirty-bitmap-sha256',
                         node='drive0', name='bitmap0')
         if sha256:
-            self.assert_qmp(result, 'return/sha256', sha256);
+            self.assert_qmp(result, 'return/sha256', sha256)
         else:
             self.assert_qmp(result, 'error/desc',
-                            "Dirty bitmap 'bitmap0' not found");
+                            "Dirty bitmap 'bitmap0' not found")
 
     def do_test_migration_resume_source(self, persistent, migrate_bitmaps):
         granularity = 512
@@ -97,7 +97,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
         self.add_bitmap(self.vm_a, granularity, persistent)
         for r in regions:
             self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)
-        sha256 = self.get_bitmap_hash(self.vm_a)
+        sha256 = get_bitmap_hash(self.vm_a)
 
         result = self.vm_a.qmp('migrate', uri=mig_cmd)
         while True:
@@ -106,7 +106,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
                 break
         while True:
             result = self.vm_a.qmp('query-status')
-            if (result['return']['status'] == 'postmigrate'):
+            if result['return']['status'] == 'postmigrate':
                 break
 
         # test that bitmap is still here
@@ -164,7 +164,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
         self.add_bitmap(self.vm_a, granularity, persistent)
         for r in regions:
             self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)
-        sha256 = self.get_bitmap_hash(self.vm_a)
+        sha256 = get_bitmap_hash(self.vm_a)
 
         if pre_shutdown:
             self.vm_a.shutdown()
@@ -214,16 +214,17 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
             self.check_bitmap(self.vm_b, sha256 if persistent else False)
 
 
-def inject_test_case(klass, name, method, *args, **kwargs):
+def inject_test_case(klass, suffix, method, *args, **kwargs):
     mc = operator.methodcaller(method, *args, **kwargs)
-    setattr(klass, 'test_' + method + name, lambda self: mc(self))
+    setattr(klass, 'test_' + method + suffix, mc)
+
 
 for cmb in list(itertools.product((True, False), repeat=5)):
     name = ('_' if cmb[0] else '_not_') + 'persistent_'
     name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
     name += '_online' if cmb[2] else '_offline'
     name += '_shared' if cmb[3] else '_nonshared'
-    if (cmb[4]):
+    if cmb[4]:
         name += '__pre_shutdown'
 
     inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
@@ -270,7 +271,8 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         # Check that the bitmaps are there
-        for node in self.vm.qmp('query-named-block-nodes', flat=True)['return']:
+        nodes = self.vm.qmp('query-named-block-nodes', flat=True)['return']
+        for node in nodes:
             if 'node0' in node['node-name']:
                 self.assert_qmp(node, 'dirty-bitmaps[0]/name', 'bmap0')
 
@@ -287,7 +289,7 @@ class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
         """
         Continue the source after migration.
         """
-        result = self.vm.qmp('migrate', uri=f'exec: cat > /dev/null')
+        result = self.vm.qmp('migrate', uri='exec: cat > /dev/null')
         self.assert_qmp(result, 'return', {})
 
         with Timeout(10, 'Migration timeout'):
-- 
2.29.2

Re: [PATCH 3/4] migrate-bitmaps-test: Fix pylint warnings

[Willian Rampazzo]

On Mon, Mar 29, 2021 at 10:28 AM Max Reitz <mreitz@redhat.com> wrote:
>
> There are a couple of things pylint takes issue with:
> - The "time" import is unused
> - The import order (iotests should come last)
> - get_bitmap_hash() doesn't use @self and so should be a function
> - Semicolons at the end of some lines
> - Parentheses after "if"
> - Some lines are too long (80 characters instead of 79)
> - inject_test_case()'s @name parameter shadows a top-level @name
>   variable
> - "lambda self: mc(self)" is equivalent to just "mc"
> - Always put two empty lines after a function
> - f'exec: cat > /dev/null' does not need to be an f-string
>
> Fix them.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/tests/migrate-bitmaps-test | 38 ++++++++++---------
>  1 file changed, 20 insertions(+), 18 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 3/4] migrate-bitmaps-test: Fix pylint warnings

[Vladimir Sementsov-Ogievskiy]

29.03.2021 16:26, Max Reitz wrote:
> There are a couple of things pylint takes issue with:
> - The "time" import is unused
> - The import order (iotests should come last)
> - get_bitmap_hash() doesn't use @self and so should be a function
> - Semicolons at the end of some lines

Wow that's funny :) My old good code :)

> - Parentheses after "if"
> - Some lines are too long (80 characters instead of 79)
> - inject_test_case()'s @name parameter shadows a top-level @name
>    variable
> - "lambda self: mc(self)" is equivalent to just "mc"
> - Always put two empty lines after a function
> - f'exec: cat > /dev/null' does not need to be an f-string
> 
> Fix them.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Thanks a lot!

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

-- 
Best regards,
Vladimir

[PATCH 4/4] iotests/297: Cover tests/

[Max Reitz]

297 so far does not check the named tests, which reside in the tests/
directory (i.e. full path tests/qemu-iotests/tests).  Fix it.

Thanks to the previous two commits, all named tests pass its scrutiny,
so we do not have to add anything to SKIP_FILES.

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

diff --git a/tests/qemu-iotests/297 b/tests/qemu-iotests/297
index e3244d40a0..ce0e82e279 100755
--- a/tests/qemu-iotests/297
+++ b/tests/qemu-iotests/297
@@ -55,8 +55,9 @@ def is_python_file(filename):
 
 
 def run_linters():
-    files = [filename for filename in (set(os.listdir('.')) - set(SKIP_FILES))
-             if is_python_file(filename)]
+    named_tests = [f'tests/{entry}' for entry in os.listdir('tests')]
+    check_tests = set(os.listdir('.') + named_tests) - set(SKIP_FILES)
+    files = [filename for filename in check_tests if is_python_file(filename)]
 
     iotests.logger.debug('Files to be checked:')
     iotests.logger.debug(', '.join(sorted(files)))
-- 
2.29.2

Re: [PATCH 4/4] iotests/297: Cover tests/

[Willian Rampazzo]

On Mon, Mar 29, 2021 at 10:28 AM Max Reitz <mreitz@redhat.com> wrote:
>
> 297 so far does not check the named tests, which reside in the tests/
> directory (i.e. full path tests/qemu-iotests/tests).  Fix it.
>
> Thanks to the previous two commits, all named tests pass its scrutiny,
> so we do not have to add anything to SKIP_FILES.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/297 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 4/4] iotests/297: Cover tests/

[Vladimir Sementsov-Ogievskiy]

29.03.2021 16:26, Max Reitz wrote:
> 297 so far does not check the named tests, which reside in the tests/
> directory (i.e. full path tests/qemu-iotests/tests).  Fix it.
> 
> Thanks to the previous two commits, all named tests pass its scrutiny,
> so we do not have to add anything to SKIP_FILES.
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>

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

-- 
Best regards,
Vladimir

Re: [PATCH 0/4] iotests/297: Cover tests/

[Kevin Wolf]

Am 29.03.2021 um 15:26 hat Max Reitz geschrieben:
> Hi,
> 
> When reviewing Vladimir’s new addition to tests/, I noticed that 297 so
> far does not cover named tests.  That isn’t so good.
> 
> This series makes it cover them, and because tests/ is rather sparse at
> this point, I decided to also fix up the two tests in there that don’t
> pass pylint’s scrutiny yet.  I think it would be nice if we could keep
> all of tests/ clean.

For patch 2, Vladimir already made the point I would have made.

For the rest:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 0/4] iotests/297: Cover tests/

[Vladimir Sementsov-Ogievskiy]

Hi!

Kindly remind. Didn't you forget? I'm responsible for these two tests, let me know if you don't have time, and I'll resend this series :)

29.03.2021 16:26, Max Reitz wrote:
> Hi,
> 
> When reviewing Vladimir’s new addition to tests/, I noticed that 297 so
> far does not cover named tests.  That isn’t so good.
> 
> This series makes it cover them, and because tests/ is rather sparse at
> this point, I decided to also fix up the two tests in there that don’t
> pass pylint’s scrutiny yet.  I think it would be nice if we could keep
> all of tests/ clean.
> 
> 
> Max Reitz (4):
>    iotests/297: Drop 169 and 199 from the skip list
>    migrate-bitmaps-postcopy-test: Fix pylint warnings
>    migrate-bitmaps-test: Fix pylint warnings
>    iotests/297: Cover tests/
> 
>   tests/qemu-iotests/297                        |  7 ++--
>   .../tests/migrate-bitmaps-postcopy-test       |  3 ++
>   tests/qemu-iotests/tests/migrate-bitmaps-test | 38 ++++++++++---------
>   3 files changed, 27 insertions(+), 21 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [PATCH 0/4] iotests/297: Cover tests/

[Max Reitz]

On 05.05.21 10:02, Vladimir Sementsov-Ogievskiy wrote:
> Hi!
> 
> Kindly remind. Didn't you forget? I'm responsible for these two tests, 
> let me know if you don't have time, and I'll resend this series :)

Sorry, I forgot indeed... v2 coming right up. :)

Max