Francesco Romani has posted comments on this change. Change subject: Adding unittests for caps.py ......................................................................
Patch Set 12: Code-Review-1 (6 comments) mostly minor things, but quite some of them. -1 for visibility. https://gerrit.ovirt.org/#/c/41077/12/tests/capsTests.py File tests/capsTests.py: Line 195: capsData) Line 196: self.assertEqual(support, False) Line 197: Line 198: def test_getLiveSnapshotSupport(self): Line 199: capsData = self._readCaps("caps_libvirt_intel_i73770_nosnap.out") please add one-line comment explaining why you use these caps. I guess you just need some readily-available test caps, right? Line 200: Line 201: result = caps._getLiveSnapshotSupport('i686', capsData) Line 202: self.assertIsNone(result) Line 203: Line 197: Line 198: def test_getLiveSnapshotSupport(self): Line 199: capsData = self._readCaps("caps_libvirt_intel_i73770_nosnap.out") Line 200: Line 201: result = caps._getLiveSnapshotSupport('i686', capsData) minor nit: a constant like 'UNSUPPORTED_ARCH' or any other intent-revealing name would be nicer. Line 202: self.assertIsNone(result) Line 203: Line 204: result = caps._getLiveSnapshotSupport(caps.Architecture.X86_64, Line 205: capsData) Line 215: self.assertTrue(result) Line 216: Line 217: def test_getAllCpuModels(self): Line 218: fileName = os.path.join(os.path.dirname(os.path.realpath(__file__)), Line 219: 'cpu_map.xml') I see this repeating. Worth moving into setUp()? Or maybe just a class constant? Line 220: result = caps._getAllCpuModels(capfile=fileName, Line 221: arch=caps.Architecture.X86_64) Line 222: Line 223: expected = {'qemu32': None, 'Haswell': 'Intel', 'cpu64-rhel6': None, Line 219: 'cpu_map.xml') Line 220: result = caps._getAllCpuModels(capfile=fileName, Line 221: arch=caps.Architecture.X86_64) Line 222: Line 223: expected = {'qemu32': None, 'Haswell': 'Intel', 'cpu64-rhel6': None, formatting like expected = { 'qemu': None, 'Haswell': 'Intel', ... has prove itself easier to work with, albeit more verbose. Line 224: 'cpu64-rhel5': None, 'Broadwell': 'Intel', 'pentium2': Line 225: None, 'pentiumpro': None, 'athlon': 'AMD', 'Nehalem': Line 226: 'Intel', 'Conroe': 'Intel', 'kvm32': None, 'pentium': None, Line 227: 'Opteron_G3': 'AMD', 'coreduo': 'Intel', 'Opteron_G1': Line 243: capsData = self._readCaps("caps_libvirt_intel_i73770_nosnap.out") Line 244: result = caps._getEmulatedMachines('x86_64', capsData) Line 245: expected = ['rhel6.5.0', 'pc', 'rhel6.4.0', 'rhel6.3.0', 'rhel6.2.0', Line 246: 'rhel6.1.0', 'rhel6.0.0', 'rhel5.5.0', 'rhel5.4.4', Line 247: 'rhel5.4.0'] please use a tuple Line 248: self.assertEqual(expected, result) Line 249: Line 250: def test_getNumaTopology(self): Line 251: capsData = self._readCaps("caps_libvirt_intel_i73770_nosnap.out") https://gerrit.ovirt.org/#/c/41077/12/tests/cpu_map.xml File tests/cpu_map.xml: Line 1: <!-- Taken from /usr/share/libvirt/cpu_map.xml Line 2: Needed for capsTests.testGetAllCpuModels to have a predictable Line 3: source of data to test the parsing of the file --> why not build-depend on libvirt and just read from the system data? We access it read-only anyway. Line 4: <cpus> Line 5: <arch name='x86'> Line 6: <!-- vendor definitions --> Line 7: <vendor name='Intel' string='GenuineIntel'/> -- To view, visit https://gerrit.ovirt.org/41077 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I86c2203bf245ba209e10b0905ae1090835408201 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Marcin Mirecki <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Marcin Mirecki <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
