| Message ID | 20181015141453.32632-8-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: Make them work for both Python 2 and 3 | expand |
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 >
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.
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.
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_'
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(-)