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

2018-10-19 Thread Eduardo Habkost 
On Fri, Oct 19, 2018 at 11:46:59AM +0200, Max Reitz wrote:
[...]
> 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.

I agree with this approach, and your original patch looks good
enough to me.

-- 
Eduardo

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

2018-10-19 Thread Max Reitz 
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 
 ---
  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



signature.asc
Description: OpenPGP digital signature

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

2018-10-15 Thread Cleber Rosa 



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 
>>> ---
>>>  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)

 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2'])