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