Change in vdsm[master]: Added unit test libvirtvmTests.py:TestLibvirtvm.testBuildCmd...
danken at redhat.com
danken at redhat.com
Tue May 14 09:12:00 UTC 2013
Dan Kenigsberg has posted comments on this change.
Change subject: Added unit test libvirtvmTests.py:TestLibvirtvm.testBuildCmdLine
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(5 inline comments)
....................................................
File tests/libvirtvmTestData.py
Line 61: <feature name="svm" policy="disable"/>
Line 62: <topology cores="1" sockets="1" threads="1"/>
Line 63: </cpu>
Line 64: </domain>
Line 65: """, ]
yes, it's fine to have a list of one item for now, but we'd need to add more and more conf/xml pairs in the future.
....................................................
File tests/libvirtvmTests.py
Line 407: inputDict = {'hostUUID': utils.getHostUUID(),
Line 408: 'SystemVersion': SystemVersion}
Line 409:
Line 410: self.assertEqual(
Line 411: type(libvirtvmTestData.testBuildCmdLineScenarios) is list, True,
I do not see the point of this (and the following) assertion: testBuildCmdLineScenarios is a static part of the test. We know that it is a list by looking at the code. And if it were a tuple or an another iterable, no harm would be done.
Line 412: "Input from testBuildCmdLineScenarios is not a list")
Line 413:
Line 414: self.assertEqual(
Line 415: type(libvirtvmTestData.testBuildCmdLineXMLS) is list, True,
Line 414: self.assertEqual(
Line 415: type(libvirtvmTestData.testBuildCmdLineXMLS) is list, True,
Line 416: "Input testBuildCmdLineXMLS should be a list")
Line 417:
Line 418: self.assertEqual(len(libvirtvmTestData.testBuildCmdLineScenarios),
AssertionError should be raised only when the tested functionality fails. if the test's input is broken (unequal lengths), a simple ValueError is better fitting.
Line 419: len(libvirtvmTestData.testBuildCmdLineXMLS),
Line 420: "TestScenario and expected XML arrays differ in size")
Line 421:
Line 422: #Perform test
Line 424: for selfConfScenario, expXML in zip(
Line 425: libvirtvmTestData.testBuildCmdLineScenarios,
Line 426: libvirtvmTestData.testBuildCmdLineXMLS):
Line 427:
Line 428: self.assertEqual(type(expXML) is str,
again, if the input is wrong, let it explode when it explodes. Testing for str would fail once we add Unicode to the test.
Line 429: True, "Element of testBuildCmdLineXMLS at " +
Line 430: str(testNum) + " is not a base string")
Line 431:
Line 432: self.assertEqual(type(selfConfScenario) is dict,
Line 444: re.sub('\n\s*', ' ',
Line 445: expectedOutput.strip(' ')),
Line 446: "XML output match fail" +
Line 447: " at pos " + str(testNum))
Line 448: testNum = testNum + 1
Bala, I appreciate your review, but at this point, I actually think that simple string comparison (with whitespace removal) is good enough. Unlike with gluster_cli, we never convert domxml to dict in vdsm, and do not see the benefit that doing so would give us.
--
To view, visit http://gerrit.ovirt.org/14111
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I74b898a6398a72608d7933009644703aa3f8d831
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Maciej Lichon <maciej.lichon.wroclaw at gmail.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Maciej Lichon <maciej.lichon.wroclaw at gmail.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
More information about the vdsm-patches
mailing list