Change in vdsm[master]: tests: add more tests for custom error responses

fromani at redhat.com fromani at redhat.com
Mon Mar 23 12:08:41 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: tests: add more tests for custom error responses
......................................................................


Patch Set 5:

(4 comments)

this patch is started with good intentions, but it has become a kitchen sink of new vaguely related code. Will split in smaller and clearly focused patches.

https://gerrit.ovirt.org/#/c/38665/5/tests/vmTests.py
File tests/vmTests.py:

Line 69: 
Line 70: 
Line 71: _TICKET_PARAMS = {
Line 72:     'userName': 'admin',
Line 73:     'userId': 'fdfc627c-d875-11e0-90f0-83df133b58cc'
> Is this related to adding more tests for error?
I need to verify the behaviour of setTicket in two different test classes, and this is shared code used by both. Previously, we had tests in just one class, hence this refactoring. See below for more details.
Line 74: }
Line 75: 
Line 76: 
Line 77: class TestVm(XMLTestCase):


Line 988:             'password': '***',
Line 989:             'ttl': 0,
Line 990:             'existingConnAction': 'disconnect',
Line 991:             'params': _TICKET_PARAMS
Line 992:         }
> Why is this a property of the test class? Can be moved to module function n
Will move
Line 993: 
Line 994:     def _updateGraphicsDevice(self, testvm, device_type):
Line 995:         def _check_ticket_params(domXML, conf, params):
Line 996:             self.assertEqual(params, _TICKET_PARAMS)


Line 1006:             testvm._dom = fake.Domain(domXml)
Line 1007: 
Line 1008:             self._updateGraphicsDevice(testvm, device['device'])
Line 1009: 
Line 1010:             self.assertEquals(testvm._dom.devXml, devXml)
> How is this refactoring related to this patch?
because we need _verifyDeviceUpdate in two tests now.
But this is becoming confusing. I will split this patch:

- refactoring
- new tests for Ticket
- new storage-related tests (no need for refactoring here)
- new setNumberOfVcpus tests (minor refactoring of vmfakelib included)
Line 1011: 
Line 1012:     def testDomainNotRunningWithoutDomain(self):
Line 1013:         with fake.VM() as testvm:
Line 1014:             self.assertEqual(testvm._dom, None)


https://gerrit.ovirt.org/#/c/38665/5/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 109:     def vcpusFlags(self, flags):
Line 110:         return -1
Line 111: 
Line 112:     def setVcpusFlags(self, numberOfCpus, flags):
Line 113:         self._failIfRequested()
> A nicer way to have methods that allways fail is to do:
Seems nicer indeed, and more explicit. Will try this concept in the next upload
Line 114: 
Line 115:     def metadata(self, type, uri, flags):
Line 116:         self._failIfRequested()
Line 117: 


-- 
To view, visit https://gerrit.ovirt.org/38665
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icf3d03f125f9c53339d9ad7fef8c31482bab0bfb
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list