Zhou Zheng Sheng has posted comments on this change. Change subject: Refactored xmlrpcTests to allow for more complex tests using running VM ......................................................................
Patch Set 2: Code-Review-1 (5 comments) Test runs successfully on my laptop. Please consider checking the result of "self._s.destroy(self._id)". This is why I -1. I like your idea in this patch. I think you can do better in future patch. When initramfs is missing, it is generated and destroyed per RunningVM. This takes much time. In future we can generate the initramfs in module setup [1] or class setup [2] method, so it is available for all the tests in this module or class. Another way is still generate it in RunningVM but cache the result in a class variable, then re-use the result. At last destroy it in module tearDown [1]. [1] http://nose.readthedocs.org/en/latest/writing_tests.html#test-modules [2] http://nose.readthedocs.org/en/latest/writing_tests.html#test-classes .................................................... File tests/functional/xmlrpcTests.py Line 166 Line 167 Line 168 Line 169 Line 170 The empty VM test is to start a VM without kernel and initramfs, and see if libvirt and KVM is functional. In reality I have not met a situation that testStartEmptyVM() succeeds but testStartSmallVM() fails, so it's OK to remove this test and just leave testSimpleVM(). Line 109: return (self._id, self._s.create(self._template)) Line 110: Line 111: def __exit__(self, type, value, traceback): Line 112: if self._tempInitramfs: Line 113: os.unlink(self._initramfsPath) Though it's rare, I think it's possible for os.unlike raises an exception, and this would skip the following destroy(), leaving a leftover VM. Maybe we can try: if self._tempInitramfs: os.unlink(self._initramfsPath) finally: self._s.assertVdsOK(self._s.destroy(self._id)) Line 114: self._s.destroy(self._id) Line 115: Line 116: def _verifyBootImages(self, kernelPath, initramfsPath): Line 117: if not os.path.isfile(kernelPath): Line 110: Line 111: def __exit__(self, type, value, traceback): Line 112: if self._tempInitramfs: Line 113: os.unlink(self._initramfsPath) Line 114: self._s.destroy(self._id) I think it's better to check the return value of destroy() as well. self._s.assertVdsOK(self._s.destroy(self._id)) This is because destroy() can fail when there is bug in VDSM. In the original _runVMKernelBootTemplate(), this check once helped me find a problem in vdsm/clientIF.py:removeVmFromMonitoredDomains . The bug could be triggered by destory(). There is a bugzilla for this problem [1], and Yeela fixed it. I think if we run functional tests before submitting patches, this kind of problem can be avoid early. [1] https://bugzilla.redhat.com/show_bug.cgi?id=990985 Line 115: Line 116: def _verifyBootImages(self, kernelPath, initramfsPath): Line 117: if not os.path.isfile(kernelPath): Line 118: raise SkipTest("Can not locate kernel image for release %s" % Line 112: if self._tempInitramfs: Line 113: os.unlink(self._initramfsPath) Line 114: self._s.destroy(self._id) Line 115: Line 116: def _verifyBootImages(self, kernelPath, initramfsPath): The method name is a bit misleading for me. At first I think "_verifyBootImages" should return True if it finds kernel image and initramfs are existed and readable. Actually it's the reverse. I get its real idea when I see "self._tempInitramfs = self._verifyBootImages ..." . So I think maybe "_missingBootImages" is a better name. Line 117: if not os.path.isfile(kernelPath): Line 118: raise SkipTest("Can not locate kernel image for release %s" % Line 119: self._kernelVer) Line 120: if not readableBy(kernelPath, QEMU_PROCESS_USER): Line 230: self._waitForStartup(vm) Line 231: Line 232: @skipNoKVM Line 233: @permutations([['hotplugNic']]) Line 234: def testVm(self, devices=[]): From what I can see, "testVm"' is to test various devices by hot-plug, currently just hot-plug NIC, and more devices in future. So I think "testVmHotplugDevice" is a better name for this test. Line 235: customization = {'vmId': '77777777-ffff-3333-bbbb-222222222222', Line 236: 'vmName': 'testVm', 'devices': []} Line 237: Line 238: with RunningVm(self.s, customization) as (vm, status): -- To view, visit http://gerrit.ovirt.org/17893 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I683c9e056137e7a189815bf6be2eb79ee80994cf Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Martin Polednik <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Giuseppe Vallarelli <[email protected]> Gerrit-Reviewer: Mark Wu <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Peter V. Saveliev <[email protected]> Gerrit-Reviewer: Petr Šebek <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
