Marcin Mirecki has posted comments on this change. Change subject: vm: unit test for vm._waitForDriveRemoval ......................................................................
Patch Set 3: (13 comments) https://gerrit.ovirt.org/#/c/48880/3/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 145: Line 146: ('hotunplug_timeout', '30', Line 147: 'Time to wait (in seconds) for a VM to detach its disk'), Line 148: Line 149: ('hotunplug_sleep_time', '1', > Maybe hotunplug_check_interval? Done Line 150: 'Sleep time between consecutive checks for VM device detach'), Line 151: Line 152: ('vm_watermark_interval', '2', Line 153: 'How often should we check drive watermark on block storage for ' Line 146: ('hotunplug_timeout', '30', Line 147: 'Time to wait (in seconds) for a VM to detach its disk'), Line 148: Line 149: ('hotunplug_sleep_time', '1', Line 150: 'Sleep time between consecutive checks for VM device detach'), > Maybe "Time to wait (in seconds) between consecutive checks for device remo Done Line 151: Line 152: ('vm_watermark_interval', '2', Line 153: 'How often should we check drive watermark on block storage for ' Line 154: 'automatic extension of thin provisioned volumes (seconds).'), https://gerrit.ovirt.org/#/c/48880/3/tests/vmTests.py File tests/vmTests.py: Line 1005: Line 1006: DRIVE_XML = """ Line 1007: <disk type='file' device='disk' snapshot='no'> Line 1008: <source file='test_path'/> Line 1009: </disk>""" > This is used only by the wait for removal tests, should be in another class Done Line 1010: Line 1011: @MonkeyPatch(libvirtconnection, 'get', lambda x: fake.Connection()) Line 1012: @permutations([[define.NORMAL], [define.ERROR]]) Line 1013: def testTimeOffsetNotPresentByDefault(self, exitCode): Line 1257: with fake.VM() as testvm: Line 1258: testvm._dom = fake.Domain(vmId='testvm') Line 1259: self.assertFalse(response.is_error(testvm.acpiShutdown())) Line 1260: Line 1261: @MonkeyPatch(config, 'getint', lambda x, y: 0) > Nice hack, but too dirty :-) Done Line 1262: def test_wait_for_drive_removal_timeout(self): Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', iface="") Line 1264: testvm = TestingVm(fake.Domain()) Line 1265: Line 1262: def test_wait_for_drive_removal_timeout(self): Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', iface="") Line 1264: testvm = TestingVm(fake.Domain()) Line 1265: Line 1266: testvm._dom = FakeVmDom(self.DRIVE_XML, times_to_return_matching=99) > Why not create the TestingVm with FakeVmDom() ? Done Line 1267: self.assertRaises(HotunplugTimeout, testvm._waitForDriveRemoval, drive) Line 1268: Line 1269: @MonkeyPatch(config, 'getint', lambda x, y: 0) Line 1270: def test_wait_for_drive_removal_removed_on_first_check(self): Line 1263: drive = Drive({}, log=self.log, index=0, path='test_path', iface="") Line 1264: testvm = TestingVm(fake.Domain()) Line 1265: Line 1266: testvm._dom = FakeVmDom(self.DRIVE_XML, times_to_return_matching=99) Line 1267: self.assertRaises(HotunplugTimeout, testvm._waitForDriveRemoval, drive) > To test this, it is enough to do couple of loops. We can override the timeo Done Line 1268: Line 1269: @MonkeyPatch(config, 'getint', lambda x, y: 0) Line 1270: def test_wait_for_drive_removal_removed_on_first_check(self): Line 1271: drive = Drive({}, log=self.log, index=0, path='test_path', iface="") Line 1271: drive = Drive({}, log=self.log, index=0, path='test_path', iface="") Line 1272: testvm = TestingVm(fake.Domain()) Line 1273: Line 1274: testvm._dom = FakeVmDom(self.DRIVE_XML) Line 1275: testvm._waitForDriveRemoval(drive) > Nice! We can count the sleeps in FakeVmDom Adding: self.assertEqual(testvm._dom._result_count, 0) which check how many times we have waited. Line 1276: Line 1277: @MonkeyPatch(config, 'getint', lambda x, y: 0) Line 1278: def test_wait_for_drive_removal_removed_on_x_check(self): Line 1279: drive = Drive({}, log=self.log, index=0, path='test_path', iface="") Line 1280: testvm = TestingVm(fake.Domain()) Line 1281: Line 1282: testvm._dom = FakeVmDom(self.DRIVE_XML, times_to_return_matching=2) Line 1283: testvm._waitForDriveRemoval(drive) Line 1284: self.assertEqual(testvm._dom._result_count, 2) > Like the previous test, with timeout=0.5 (sometimes we run on overloaded sl Increasing the timeout to 1s. The 1s should never be reached, as it should exit after 2 matching responses from dom. Line 1285: Line 1286: Line 1287: class FakeVmDom(object): Line 1288: Line 1285: Line 1286: Line 1287: class FakeVmDom(object): Line 1288: Line 1289: def __init__(self, device_xml, times_to_return_matching=0): > This is little too long Done Line 1290: self._times_to_return_matching = times_to_return_matching Line 1291: self._result_count = 0 Line 1292: self.result_xml = ET.fromstring( Line 1293: """ Line 1287: class FakeVmDom(object): Line 1288: Line 1289: def __init__(self, device_xml, times_to_return_matching=0): Line 1290: self._times_to_return_matching = times_to_return_matching Line 1291: self._result_count = 0 > Why not use: Done Line 1292: self.result_xml = ET.fromstring( Line 1293: """ Line 1294: <domain type='kvm' id='2'> Line 1295: <devices/> Line 1294: <domain type='kvm' id='2'> Line 1295: <devices/> Line 1296: </domain> Line 1297: """) Line 1298: self.result_xml.find("devices").append(ET.fromstring(device_xml)) > Why keep etree element and return xml, when we can prepare the xml once in Done Line 1299: Line 1300: def XMLDesc(self, anything): Line 1301: if self._result_count < self._times_to_return_matching: Line 1302: self._result_count += 1 Line 1299: Line 1300: def XMLDesc(self, anything): Line 1301: if self._result_count < self._times_to_return_matching: Line 1302: self._result_count += 1 Line 1303: return ET.tostring(self.result_xml) > We can simplify a little bit: Done Line 1304: return '<does_not_match/>' Line 1305: Line 1306: Line 1307: class ChangingSchedulerDomain(object): Line 1300: def XMLDesc(self, anything): Line 1301: if self._result_count < self._times_to_return_matching: Line 1302: self._result_count += 1 Line 1303: return ET.tostring(self.result_xml) Line 1304: return '<does_not_match/>' > Nice hack, but it would better to return the expected xml. Done Line 1305: Line 1306: Line 1307: class ChangingSchedulerDomain(object): Line 1308: -- 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: 3 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