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_'
-- 
2.17.1

On Mon, Oct 15, 2018 at 04:14:51PM +0200, 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))

The "lambda self: mc(self)" expression looks weird and
unnecessary at first look, but I have just confirmed that this
doesn't work:

    setattr(klass, 'test_' + name, mc)

Probably because the methodcaller object won't automatically
become a instance method after the TestCase class is
instantiated.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

>  
>  for cmb in list(itertools.product((True, False), repeat=4)):
>      name = ('_' if cmb[0] else '_not_') + 'persistent_'
> -- 
> 2.17.1
> 

-- 
Eduardo

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


 if __name__ == '__main__':

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.

-- 
Eduardo

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)

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

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

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