From a243eed9f452813d4e8f84f5481e1cf732ee139d Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Wed, 20 Aug 2014 11:26:33 +0200 Subject: integration-test: Drop sync_workaround, fix property testing Object property updates are inherently asynchronous. Add new assertEventually() and assertProperty() helpers which retry a few times and sync the client between iterations. Drop the now unnecessary --no-workarounds option and sync_workaround(), and various manual settle() calls. Note that we still keep some direct get_property() comparisons in places where we don't expect any latency. --- src/tests/integration-test | 144 ++++++++++++++++++++------------------------- 1 file changed, 63 insertions(+), 81 deletions(-) diff --git a/src/tests/integration-test b/src/tests/integration-test index ac4a034..5c02462 100755 --- a/src/tests/integration-test +++ b/src/tests/integration-test @@ -70,14 +70,6 @@ VDEV_SIZE = 300000000 # size of virtual test device # particular the executable bit BROKEN_PERMISSIONS_FS = ['ntfs', 'exfat'] -# Some D-BUS API methods cause properties to not be up to date yet when a -# method call finishes, thus we do an udevadm settle as a workaround. Those -# methods should eventually get fixed properly, but it's unnerving to have -# the tests fail on them when you are working on something else. This flag -# gets set by the --no-workarounds option to disable those syncs, so that these -# race conditions can be fixed. -workaround_syncs = False - no_options = GLib.Variant('a{sv}', {}) @@ -201,23 +193,6 @@ class UDisksTestCase(unittest.TestCase): timeout -= 1 if timeout <= 0: sys.stderr.write('[wait timeout!] ') - sys.stderr.flush() - klass.client.settle() - os.sync() - - @classmethod - def sync_workaround(klass): - '''Wait until pending events finished processing (bug workaround). - - This should be called for race conditions in the D-BUS API which cause - properties to not be up to date yet when a method call finishes. Those - should eventually get fixed properly, but it's unnerving to have the - tests fail on them when you are working on something else. - - This sync is not done if running with --no-workarounds. - ''' - if workaround_syncs: - klass.sync() @classmethod def zero_device(klass): @@ -335,7 +310,6 @@ class UDisksTestCase(unittest.TestCase): block = klass.udisks_block(partition) block.call_format_sync(type, options, None) - klass.sync_workaround() @classmethod def retry_busy(klass, fn, *args): @@ -487,6 +461,27 @@ class UDisksTestCase(unittest.TestCase): time.sleep(0.5) klass.sync() + def assertEventually(self, fn, value): + '''Check that an function is eventually equal to value. + + This is mostly meant for checking object properties, as these are + updated asynchronously. This retries up to 10 times. + ''' + retries = 10 + while retries > 0: + if fn() == value: + break + retries -= 1 + time.sleep(0.1) + self.sync() + + self.assertEqual(fn(), value) + + def assertProperty(self, obj, name, value): + '''Check that an object's property is eventually equal to value''' + + self.assertEventually(lambda: obj.get_property(name), value) + # ---------------------------------------------------------------------------- @@ -560,9 +555,9 @@ class Manager(UDisksTestCase): # can't format due to permission error self.assertRaises(GLib.GError, block.call_format_sync, 'ext2', no_options, None) - self.assertEqual(block.get_property('id-label'), '') - self.assertEqual(block.get_property('id-usage'), '') - self.assertEqual(block.get_property('id-type'), '') + self.assertProperty(block, 'id-label', '') + self.assertProperty(block, 'id-usage', '') + self.assertProperty(block, 'id-type', '') finally: self.client.settle() loop.call_delete_sync(no_options, None) @@ -609,10 +604,10 @@ class FS(UDisksTestCase): '''properties of zeroed out device''' self.zero_device() - self.assertEqual(self.block.get_property('device'), self.device) + self.assertProperty(self.block, 'device', self.device) self.assertTrue('Linux_scsi_debug' in self.block.get_property('drive')) + self.assertProperty(self.block, 'id-label', '') self.assertEqual(self.block.get_property('hint-system'), True) - self.assertEqual(self.block.get_property('id-label'), '') self.assertEqual(self.block.get_property('id-usage'), '') self.assertEqual(self.block.get_property('id-type'), '') self.assertEqual(self.block.get_property('id-uuid'), '') @@ -675,16 +670,16 @@ class FS(UDisksTestCase): self.mkfs('ext4', 'foo') block = self.udisks_block() - self.assertEqual(block.get_property('id-usage'), 'filesystem') - self.assertEqual(block.get_property('id-type'), 'ext4') - self.assertEqual(block.get_property('id-label'), 'foo') + self.assertProperty(block, 'id-usage', 'filesystem') + self.assertProperty(block, 'id-type', 'ext4') + self.assertProperty(block, 'id-label', 'foo') self.assertNotEqual(self.udisks_filesystem(), None) self.fs_create(None, 'empty', no_options) - self.assertEqual(block.get_property('id-usage'), '') - self.assertEqual(block.get_property('id-type'), '') - self.assertEqual(block.get_property('id-label'), '') + self.assertProperty(block, 'id-usage', '') + self.assertProperty(block, 'id-type', '') + self.assertProperty(block, 'id-label', '') self.assertEqual(self.udisks_filesystem(), None) def test_create_fs_unknown_type(self): @@ -728,18 +723,16 @@ class FS(UDisksTestCase): # after putting it back, it should be mountable again self.readd_devices() fs = self.udisks_filesystem() - self.assertEqual(fs.get_property('mount-points'), []) + self.assertProperty(fs, 'mount-points', []) mount_path = fs.call_mount_sync(no_options, None) self.assertTrue(mount_path.endswith('udiskstest')) self.assertTrue('/media/' in mount_path) self.assertTrue(self.is_mountpoint(mount_path)) - self.client.settle() - self.assertEqual(fs.get_property('mount-points'), [mount_path]) + self.assertProperty(fs, 'mount-points', [mount_path]) self.retry_busy(fs.call_unmount_sync, no_options, None) - self.client.settle() - self.assertEqual(fs.get_property('mount-points'), []) + self.assertProperty(fs, 'mount-points', []) def test_existing_manual_mount_point(self): '''fs: does not reuse existing manual mount point''' @@ -752,8 +745,7 @@ class FS(UDisksTestCase): self.assertTrue(mount_path.endswith('udiskstest')) self.retry_busy(fs.call_unmount_sync, no_options, None) - self.client.settle() - self.assertEqual(fs.get_property('mount-points'), []) + self.assertProperty(fs, 'mount-points', []) # cleans up mountpoint self.assertFalse(os.path.exists(mount_path)) @@ -765,8 +757,7 @@ class FS(UDisksTestCase): try: new_mount_path = fs.call_mount_sync(no_options, None) self.retry_busy(fs.call_unmount_sync, no_options, None) - self.client.settle() - self.assertEqual(fs.get_property('mount-points'), []) + self.assertProperty(fs, 'mount-points', []) self.assertEqual(new_mount_path, mount_path + '1') finally: os.rmdir(mount_path) @@ -794,8 +785,7 @@ class FS(UDisksTestCase): fs = self.udisks_filesystem() new_mount_path = fs.call_mount_sync(no_options, None) self.retry_busy(fs.call_unmount_sync, no_options, None) - self.client.settle() - self.assertEqual(fs.get_property('mount-points'), []) + self.assertProperty(fs, 'mount-points', []) self.assertEqual(new_mount_path, mount_path) def _do_fs_check(self, type): @@ -848,9 +838,8 @@ class FS(UDisksTestCase): block = self.udisks_block() - self.assertEqual(block.get_property('id-usage'), (type == 'swap') and 'other' or 'filesystem') - - self.assertEqual(block.get_property('id-type'), type) + self.assertProperty(block, 'id-usage', (type == 'swap') and 'other' or 'filesystem') + self.assertProperty(block, 'id-type', type) l = block.get_property('id-label') if type == 'vfat': l = l.lower() # VFAT is case insensitive @@ -887,14 +876,11 @@ class FS(UDisksTestCase): return self.assertEqual(ret, 0) - time.sleep(0.5) - self.sync() - self.assertEqual(fs.get_property('mount-points'), [self.workdir]) + self.assertProperty(fs, 'mount-points', [self.workdir]) # unmount it subprocess.call(['umount', self.workdir]) - self.sync() - self.assertEqual(fs.get_property('mount-points'), []) + self.assertProperty(fs, 'mount-points', []) def _do_udisks_check(self, type, label=None): '''udisks API correctly changes file system''' @@ -916,12 +902,13 @@ class FS(UDisksTestCase): self.assertEqual(l, label or '') block = self.udisks_block() - self.assertEqual(block.get_property('id-usage'), (type == 'swap') and 'other' or 'filesystem') - self.assertEqual(block.get_property('id-type'), type) - l = block.get_property('id-label') - if type == 'vfat': - l = l.lower() # VFAT is case insensitive - self.assertEqual(l, label or '') + self.assertProperty(block, 'id-usage', (type == 'swap') and 'other' or 'filesystem') + self.assertProperty(block, 'id-type', type) + if type == 'vfat' and label: + # VFAT is case insensitive + self.assertEventually(lambda: block.get_property('id-label').lower(), label.lower()) + else: + self.assertProperty(block, 'id-label', label or '') if type == 'swap': return @@ -944,9 +931,11 @@ class FS(UDisksTestCase): else: self.assertTrue(mount_path.endswith(label)) - self.sync() - self.assertEqual(fs.get_property('mount-points'), [mount_path]) self.assertTrue(self.is_mountpoint(mount_path)) + # FIXME: this should work on the existing fs object, but doesn't! + self.sync() + fs = self.udisks_filesystem() + self.assertProperty(fs, 'mount-points', [mount_path]) # no ownership taken, should be root owned st = os.stat(mount_path) @@ -957,7 +946,7 @@ class FS(UDisksTestCase): # unmount self.retry_busy(fs.call_unmount_sync, no_options, None) self.assertFalse(os.path.exists(mount_path), 'mount point was not removed') - self.assertEqual(fs.get_property('mount-points'), [mount_path]) + self.assertProperty(fs, 'mount-points', []) # create fs with taking ownership (daemon:mail == 1:8) # if supports_unix_owners: @@ -991,20 +980,18 @@ class FS(UDisksTestCase): block = self.udisks_block() blkid_label = self.blkid().get('ID_FS_LABEL_ENC', '').replace('\\x22', '"').replace( '\\x5c', '\\').replace('\\x24', '$') - self.sync_workaround() if type == 'vfat': # EXFAIL: often (but not always) the label appears in all upper case self.assertEqual(blkid_label.upper(), l.upper()) - self.assertEqual(block.get_property('id-label').upper(), l.upper()) + self.assertEventually(lambda: block.get_property('id-label').upper(), l.upper()) else: self.assertEqual(blkid_label, l) - self.assertEqual(block.get_property('id-label'), l) + self.assertProperty(block, 'id-label', l) # test setting empty label fs.call_set_label_sync('', no_options, None) - self.sync_workaround() self.assertEqual(self.blkid().get('ID_FS_LABEL_ENC', ''), '') - self.assertEqual(block.get_property('id-label'), '') + self.assertProperty(block, 'id-label', '') # check fs - Not implemented in udisks yet # self.assertEqual(iface.FilesystemCheck([]), True) @@ -1025,15 +1012,14 @@ class FS(UDisksTestCase): mount_path = cd_fs.call_mount_sync(no_options, None) try: self.assertTrue('/media/' in mount_path) - self.sync() - self.assertEqual(cd_fs.get_property('mount-points'), [mount_path]) + self.assertProperty(cd_fs, 'mount-points', [mount_path]) self.assertTrue(self.is_mountpoint(mount_path)) - self.assertEqual(self.udisks_block(cd=True).get_property('read-only'), True) + self.assertProperty(self.udisks_block(cd=True), 'read-only', True) finally: self.retry_busy(cd_fs.call_unmount_sync, no_options, None) self.assertFalse(os.path.exists(mount_path), 'mount point was not removed') - self.assertEqual(cd_fs.get_property('mount-points'), [mount_path]) + self.assertProperty(cd_fs, 'mount-points', []) def _do_file_perms_checks(self, type, mount_point): '''Check for permissions for data files and executables. @@ -1139,6 +1125,7 @@ class Luks(UDisksTestCase): self.fs_create(None, 'ext4', GLib.Variant('a{sv}', { 'encrypt.passphrase': GLib.Variant('s', 's3kr1t'), 'label': GLib.Variant('s', 'treasure')})) + self.client.settle() try: block = self.udisks_block() @@ -1214,8 +1201,7 @@ class Luks(UDisksTestCase): self.assertTrue('/media/' in mount_path) self.assertTrue(mount_path.endswith('treasure')) self.assertTrue(self.is_mountpoint(mount_path)) - self.client.settle() - self.assertEqual(fs.get_property('mount-points'), [mount_path]) + self.assertProperty(fs, 'mount-points', [mount_path]) # can't lock, busy try: @@ -1318,7 +1304,7 @@ class Polkit(UDisksTestCase, test_polkitd.PolkitTestCase): options = GLib.Variant('a{sv}', {'label': GLib.Variant('s', 'polkityes')}) self.fs_create(None, 'ext4', options) block = self.udisks_block() - self.assertEqual(block.get_property('id-usage'), 'filesystem') + self.assertProperty(block, 'id-usage', 'filesystem') self.assertEqual(block.get_property('id-type'), 'ext4') self.assertEqual(block.get_property('id-label'), 'polkityes') @@ -1349,14 +1335,10 @@ if __name__ == '__main__': argparser = argparse.ArgumentParser(description='udisks2 integration test suite') argparser.add_argument('-l', '--log-file', dest='logfile', help='write daemon log to a file') - argparser.add_argument('-w', '--no-workarounds', action="store_true", default=False, - help='Disable workarounds for race conditions in the D-BUS API') argparser.add_argument('testname', nargs='*', help='name of test class or method (e. g. "Drive", "FS.test_ext2")') args = argparser.parse_args() - workaround_syncs = not args.no_workarounds - UDisksTestCase.init(logfile=args.logfile) if args.testname: tests = unittest.TestLoader().loadTestsFromNames( -- cgit v1.2.3