Nir Soffer has posted comments on this change. Change subject: vm: unit test for vm._waitForDriveRemoval ......................................................................
Patch Set 5: (8 comments) https://gerrit.ovirt.org/#/c/48880/5/tests/vmTests.py File tests/vmTests.py: Line 1261: <disk type='file' snapshot='no'> Line 1262: <source file='test_path'/> Line 1263: </disk>""" Line 1264: NETWORK_DRIVE_XML = """ Line 1265: <disk type='file' snapshot='no'> type="network" Line 1266: <source name='test_path'/> Line 1267: </disk>""" Line 1268: BLOCK_DRIVE_XML = """ Line 1269: <disk type='file' snapshot='no'> Line 1265: <disk type='file' snapshot='no'> Line 1266: <source name='test_path'/> Line 1267: </disk>""" Line 1268: BLOCK_DRIVE_XML = """ Line 1269: <disk type='file' snapshot='no'> type="block" See vmStorageTests for example xmls. Line 1270: <source dev='/block_path'/> Line 1271: </disk>""" Line 1272: Line 1273: drive_default = Drive({}, log=logging.getLogger(''), index=0, iface="", Line 1270: <source dev='/block_path'/> Line 1271: </disk>""" Line 1272: Line 1273: drive_default = Drive({}, log=logging.getLogger(''), index=0, iface="", Line 1274: path='test_path', ) We can remove the default, we are not testing the Drive class here. Line 1275: drive_file = Drive({}, log=logging.getLogger(''), index=0, iface="", Line 1276: path='test_path', diskType=DISK_TYPE.FILE) Line 1277: drive_network = Drive({}, log=logging.getLogger(''), index=0, iface="", Line 1278: path='test_path', diskType=DISK_TYPE.NETWORK) Line 1276: path='test_path', diskType=DISK_TYPE.FILE) Line 1277: drive_network = Drive({}, log=logging.getLogger(''), index=0, iface="", Line 1278: path='test_path', diskType=DISK_TYPE.NETWORK) Line 1279: drive_block = Drive({}, log=logging.getLogger(''), index=0, iface="", Line 1280: path= "/block_path", diskType=DISK_TYPE.BLOCK) path is always /rhev/data-center/p/d/images/i/v, (p=poolid, d=domainid, i=imageid, v=volumeid) for file and block. For network the path is rbd:pool/volume, but it does not really matter. For the tests, using the same "test_path" is fine. Line 1281: Line 1282: @MonkeyPatch(vm, "config", make_config([ Line 1283: ("vars", "hotunplug_timeout", "0.1"), Line 1284: ("vars", "hotunplug_check_interval", "0.03") Line 1279: drive_block = Drive({}, log=logging.getLogger(''), index=0, iface="", Line 1280: path= "/block_path", diskType=DISK_TYPE.BLOCK) Line 1281: Line 1282: @MonkeyPatch(vm, "config", make_config([ Line 1283: ("vars", "hotunplug_timeout", "0.1"), > Made it still shorter. 0.03 is too short for overloaded ci slaves. Lets use timeout=0.25, check_internal=0.1 will time out after retries, in the worst case after one retry, good enough. Line 1284: ("vars", "hotunplug_check_interval", "0.03") Line 1285: ])) Line 1286: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == "/block_path") Line 1287: @permutations([[drive_default, FILE_DRIVE_XML], Line 1282: @MonkeyPatch(vm, "config", make_config([ Line 1283: ("vars", "hotunplug_timeout", "0.1"), Line 1284: ("vars", "hotunplug_check_interval", "0.03") Line 1285: ])) Line 1286: @MonkeyPatch(utils, "isBlockDevice", lambda x: x == "/block_path") > Drive actually goes to the file system to check if the device is a block de Looks ok, the actual code is dirty in this case :-) Line 1287: @permutations([[drive_default, FILE_DRIVE_XML], Line 1288: [drive_file, FILE_DRIVE_XML], Line 1289: [drive_network, NETWORK_DRIVE_XML], Line 1290: [drive_block, BLOCK_DRIVE_XML]]) Line 1338: <source file='invalid_test_path'/> Line 1339: </disk> Line 1340: <disk type='file' device='disk' snapshot='no'> Line 1341: <source file='invalid_test_path'/> Line 1342: </disk> Lets update the xml of the devices to look more like the real xml, see vmStorageTests. Line 1343: <interface> Line 1344: <mac address='invalid_macAddr'/> Line 1345: </interface> Line 1346: </devices> Line 1356: Line 1357: def XMLDesc(self, flags): Line 1358: self.xmldesc_fetched += 1 Line 1359: if self.xmldesc_fetched > self.times_to_match + 1: Line 1360: raise BaseException( You should raise an Exception, Base Exception is not for user code (in general). But I don't think we need it, it will make it less useful, I thin we should just count the fetches and assert about them in the tests. Line 1361: "_waitForDriveRemoval called XMLDesc too many times.") Line 1362: if self.xmldesc_fetched > self.times_to_match: Line 1363: return self.DEFAULT_XML Line 1364: else: -- To view, visit https://gerrit.ovirt.org/48880 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d1f86e17132a7099f109a5e33407c56dde40df2 Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Marcin Mirecki <mmire...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Marcin Mirecki <mmire...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches