Re: [Qemu-devel] [PATCH 7/9] iotests: 'new' module replacement in 169

[Max Reitz <mreitz@redhat.com>]

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

On 16.10.18 03:01, Cleber Rosa wrote:
> 
> 
> On 10/15/18 7:57 PM, Eduardo Habkost wrote:
>> On Mon, Oct 15, 2018 at 07:38:45PM -0400, Cleber Rosa wrote:
>>>
>>>
>>> On 10/15/18 10:14 AM, Max Reitz wrote:
>>>> iotest 169 uses the 'new' module to add methods to a class.  This module
>>>> no longer exists in Python 3.  Instead, we can use a lambda.  Best of
>>>> all, this works in 2.7 just as well.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>  tests/qemu-iotests/169 | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
>>>> index f243db9955..e5614b159d 100755
>>>> --- a/tests/qemu-iotests/169
>>>> +++ b/tests/qemu-iotests/169
>>>> @@ -23,7 +23,6 @@ import iotests
>>>>  import time
>>>>  import itertools
>>>>  import operator
>>>> -import new
>>>>  from iotests import qemu_img
>>>>  
>>>>  
>>>> @@ -144,7 +143,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>>>>  
>>>>  def inject_test_case(klass, name, method, *args, **kwargs):
>>>>      mc = operator.methodcaller(method, *args, **kwargs)
>>>> -    setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
>>>> +    setattr(klass, 'test_' + name, lambda self: mc(self))
>>>>  
>>>>  for cmb in list(itertools.product((True, False), repeat=4)):
>>>>      name = ('_' if cmb[0] else '_not_') + 'persistent_'
>>>>
>>>
>>> This can be simplified with:
>>>
>>> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
>>> index e5614b159d..2199f14ae7 100755
>>> --- a/tests/qemu-iotests/169
>>> +++ b/tests/qemu-iotests/169
>>> @@ -22,7 +22,6 @@ import os
>>>  import iotests
>>>  import time
>>>  import itertools
>>> -import operator
>>>  from iotests import qemu_img
>>>
>>>
>>> @@ -141,18 +140,15 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>>>              self.check_bitmap(self.vm_b, sha256 if persistent else False)
>>>
>>>
>>> -def inject_test_case(klass, name, method, *args, **kwargs):
>>> -    mc = operator.methodcaller(method, *args, **kwargs)
>>> -    setattr(klass, 'test_' + name, lambda self: mc(self))
>>> -
>>>  for cmb in list(itertools.product((True, False), repeat=4)):
>>>      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'
>>>
>>> -    inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
>>> -                     *list(cmb))
>>> +    setattr(TestDirtyBitmapMigration, 'test_' + name,
>>> +            lambda self: TestDirtyBitmapMigration.do_test_migration(
>>> +                self, *list(cmb)))
>>
>> I'm not fond of the long multi-line lambda expression, but I love
>> that you got rid of operator.methodcaller().  :)
>>
>> However, this doesn't seem to work: `*list(cmb)` will be
>> evaluated only when the lambda is called, and `cmb` will be
>> always `(False, False, False, False)`.
>>
>> I was going to suggest defining a nested function inside
>> inject_test_case() to replace operator.methodcaller(), but then I
>> thought it was not worth it.
>>
> 
> You're right! I missed that.  Anyway, another possibility:
> 
> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
> index e5614b159d..cd0d9c289c 100755
> --- a/tests/qemu-iotests/169
> +++ b/tests/qemu-iotests/169
> @@ -22,7 +22,7 @@ import os
>  import iotests
>  import time
>  import itertools
> -import operator
> +import functools
>  from iotests import qemu_img
> 
> 
> @@ -140,20 +140,15 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>              self.vm_b.launch()
>              self.check_bitmap(self.vm_b, sha256 if persistent else False)
> 
> -
> -def inject_test_case(klass, name, method, *args, **kwargs):
> -    mc = operator.methodcaller(method, *args, **kwargs)
> -    setattr(klass, 'test_' + name, lambda self: mc(self))
> -
> -for cmb in list(itertools.product((True, False), repeat=4)):
> +for cmb in itertools.product((True, False), repeat=4):
>      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'
> 
> -    inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
> -                     *list(cmb))
> -
> +    test =
> functools.partialmethod(TestDirtyBitmapMigration.do_test_migration,
> +                                   *cmb)
> +    setattr(TestDirtyBitmapMigration, 'test_' + name, test)

I'm not sure how that is any simpler, though (apart from the inlining). :-)

I mean, my personal goal is not to touch this beyond what I need to
because it's black magic to me anyway, so I'm happy with what works.

Max


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