Change in vdsm[master]: vm: unit test for vm._waitForDriveRemoval

nsoffer at redhat.com nsoffer at redhat.com
Wed Nov 25 00:29:38 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: vm: unit test for vm._waitForDriveRemoval
......................................................................


Patch Set 4: Code-Review-1

(10 comments)

https://gerrit.ovirt.org/#/c/48880/4/tests/vmTests.py
File tests/vmTests.py:

Line 1263:     @MonkeyPatch(vm, "config", make_config([
Line 1264:         ("vars", "hotunplug_timeout", "0.5"),
Line 1265:         ("vars", "hotunplug_check_interval", "0.1")
Line 1266:     ]))
Line 1267:     def test_wait_for_drive_removal_timeout(self):
We don't need to repeat the class name in the tests - use:

    def test_timeout(self):

Same for other tests.

To add tests for other devices, we can use permutations to run all tests with all devices without writing additional tests, or since this code does not care about the device, we can add one test for each device, testing one of the scenarios.
Line 1268:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")
Line 1269:         testvm = TestingVm(FakeVmDom(self.DRIVE_XML, times_to_match=9))
Line 1270: 
Line 1271:         self.assertRaises(HotunplugTimeout, testvm._waitForDriveRemoval, drive)


Line 1266:     ]))
Line 1267:     def test_wait_for_drive_removal_timeout(self):
Line 1268:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")
Line 1269:         testvm = TestingVm(FakeVmDom(self.DRIVE_XML, times_to_match=9))
Line 1270: 
Empty line is not needed, this is a very short and simple test - same for other tests.
Line 1271:         self.assertRaises(HotunplugTimeout, testvm._waitForDriveRemoval, drive)
Line 1272: 
Line 1273:     # The timeout hotunplug_check_interval=1 should never be reached. We should
Line 1274:     # never reach sleep when device is removed in first check, and method


Line 1280:     def test_wait_for_drive_removal_removed_on_first_check(self):
Line 1281:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")
Line 1282:         testvm = TestingVm(FakeVmDom(self.DRIVE_XML))
Line 1283: 
Line 1284:         testvm._waitForDriveRemoval(drive)
Check for testvm._dom.xmldesc_fetched == 1
Line 1285: 
Line 1286:     @MonkeyPatch(vm, "config", make_config([
Line 1287:         ("vars", "hotunplug_timeout", "1"),
Line 1288:         ("vars", "hotunplug_check_interval", "0")


Line 1290:     def test_wait_for_drive_removal_removed_on_x_check(self):
Line 1291:         drive = Drive({}, log=self.log, index=0, path='test_path', iface="")
Line 1292:         testvm = TestingVm(FakeVmDom(self.DRIVE_XML, times_to_match=2))
Line 1293: 
Line 1294:         testvm._waitForDriveRemoval(drive)
Check for testvm._dom.xmldesc_fetched == 3
Line 1295: 
Line 1296: 
Line 1297: class FakeVmDom(object):
Line 1298: 


Line 1293: 
Line 1294:         testvm._waitForDriveRemoval(drive)
Line 1295: 
Line 1296: 
Line 1297: class FakeVmDom(object):
Lets give this a more specific name.
Line 1298: 
Line 1299:     DEFAULT_XML = """
Line 1300:     <domain type='kvm' id='2'>
Line 1301:         <devices/>


Line 1298: 
Line 1299:     DEFAULT_XML = """
Line 1300:     <domain type='kvm' id='2'>
Line 1301:         <devices/>
Line 1302:     </domain>
We need other devices in the default xml. For testing drives, we need other drives with different path and different types.

We like to test all disk types: block, file, network.

Since all this is not relevant to removing nics, the default xml should be an argument to __init__ - each test will use a relevant xml for the type of device.
Line 1303:     """
Line 1304: 
Line 1305:     def __init__(self, device_xml, times_to_match=0):
Line 1306:         self.matches = times_to_match


Line 1301:         <devices/>
Line 1302:     </domain>
Line 1303:     """
Line 1304: 
Line 1305:     def __init__(self, device_xml, times_to_match=0):
Lets add a parameter for the domain_xml, the xml that should be returned when number of fetches exceeded times_to_match:
    
    self.domain_xml =
Line 1306:         self.matches = times_to_match
Line 1307:         result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308:         result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309:         self.xml = ET.tostring(result_xml)


Line 1302:     </domain>
Line 1303:     """
Line 1304: 
Line 1305:     def __init__(self, device_xml, times_to_match=0):
Line 1306:         self.matches = times_to_match
Now that we have a counter, we can keep the name as is
Line 1307:         result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308:         result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309:         self.xml = ET.tostring(result_xml)
Line 1310: 


Line 1305:     def __init__(self, device_xml, times_to_match=0):
Line 1306:         self.matches = times_to_match
Line 1307:         result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308:         result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309:         self.xml = ET.tostring(result_xml)
lets call this self.domain_xml_with_device to make it more clear.
Line 1310: 
Line 1311:     def XMLDesc(self, flags):
Line 1312:         if self.matches == 0:
Line 1313:             return self.DEFAULT_XML


Line 1307:         result_xml = ET.fromstring(self.DEFAULT_XML)
Line 1308:         result_xml.find("devices").append(ET.fromstring(device_xml))
Line 1309:         self.xml = ET.tostring(result_xml)
Line 1310: 
Line 1311:     def XMLDesc(self, flags):
We should use here:

    self.xmldesc_fetched += 1
    if self.xmldesc_fetched > self.times_to_match:
        return self.domain_xml
    else:
        return self.domain_xml_with_device
Line 1312:         if self.matches == 0:
Line 1313:             return self.DEFAULT_XML
Line 1314:         self.matches -= 1
Line 1315:         return self.xml


-- 
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: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: Amit Aviram <aaviram at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Marcin Mirecki <mmirecki at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list