| Message ID | 20210514154351.629027-4-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests/297: Cover tests/ | expand |
14.05.2021 18:43, 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 > - 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)" were equivalent to just "mc", but in > inject_test_case(), it is not equivalent, so add a comment and disable > the warning locally > - 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> > --- [..] > -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)) > + # The lambda is required to enforce the `self` parameter. Without it, > + # `mc` would be called without any arguments, and then complain. > + # pylint: disable=unnecessary-lambda > + setattr(klass, 'test_' + method + suffix, lambda self: mc(self)) > + > Interesting... I decided to experiment a bit, and what I can say now: The actual reason is that class attrubute of type <class 'function'>, becomes a <class 'method'> of the class instance on instantiation. lambda is a function, so on instantiation we'll have "method", and method can be called as obj.method(), and original function will get "self" first argument automatically. mc is not a function, it's <class 'operator.methodcaller'>, so there is no magic, instance of the class doesn't get own method but just a refence to class variable instead. So, let's modify comment to something like: We want to add function attribute to class, so that it correctly converted to method on instantiation. lamba is necessary to "convert" methodcaller object (which is callable, but not a function) to function. with it: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> ==== my expirements ===== # cat x.py import operator class X: def hello(self, arg): print("hello", arg) mc = operator.methodcaller("hello", "Vova") lmd = lambda self: mc(self) print('mc:', type(mc)) print('lmd:', type(lmd)) setattr(X, "test_hello_direct", mc) setattr(X, "test_hello_lambda", lmd) X.simply_assigned = lmd x = X() x.assigned_to_instance = lmd print('mc attached:', type(x.test_hello_direct)) print('lmd attached:', type(x.test_hello_lambda)) print('lmd simply assigned:', type(x.simply_assigned)) print('lmd assigned to instance:', type(x.assigned_to_instance)) x.test_hello_lambda() x.simply_assigned() print("x.test_hello_lambda is x.simply_assigned", x.test_hello_lambda is x.simply_assigned) print("x.test_hello_lambda is X.test_hello_lambda", x.test_hello_lambda is X.test_hello_lambda) print("x.test_hello_direct is X.test_hello_direct", x.test_hello_direct is X.test_hello_direct) print("X.test_hello_lambda is X.simply_assigned", X.test_hello_lambda is X.simply_assigned) print("X.test_hello_lambda type:", type(X.test_hello_lambda)) try: x.assigned_to_instance() except Exception as e: print("assigned to instance call failed:", e) try: x.test_hello_direct() except Exception as e: print("direct call failed:", e) # python3 x.py mc: <class 'operator.methodcaller'> lmd: <class 'function'> mc attached: <class 'operator.methodcaller'> lmd attached: <class 'method'> lmd simply assigned: <class 'method'> lmd assigned to instance: <class 'function'> hello Vova hello Vova x.test_hello_lambda is x.simply_assigned False x.test_hello_lambda is X.test_hello_lambda False x.test_hello_direct is X.test_hello_direct True X.test_hello_lambda is X.simply_assigned True X.test_hello_lambda type: <class 'function'> assigned to instance call failed: <lambda>() missing 1 required positional argument: 'self' direct call failed: methodcaller expected 1 argument, got 0
diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-test b/tests/qemu-iotests/tests/migrate-bitmaps-test index a5c7bc83e0..31d3255943 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,20 @@ 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)) + # The lambda is required to enforce the `self` parameter. Without it, + # `mc` would be called without any arguments, and then complain. + # pylint: disable=unnecessary-lambda + setattr(klass, 'test_' + method + suffix, lambda self: mc(self)) + 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 +274,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 +292,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'):
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)" were equivalent to just "mc", but in inject_test_case(), it is not equivalent, so add a comment and disable the warning locally - 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 | 41 +++++++++++-------- 1 file changed, 23 insertions(+), 18 deletions(-)