Francesco Romani has posted comments on this change. Change subject: tests: add more tests for custom error responses ......................................................................
Patch Set 4: (4 comments) https://gerrit.ovirt.org/#/c/38665/4/tests/vmTests.py File tests/vmTests.py: Line 897: {'type': 'graphics', 'device': 'vnc', 'port': '-1'}] Line 898: Line 899: TICKET_PARAMS = { Line 900: 'userName': 'admin', Line 901: 'userId': 'fdfc627c-d875-11e0-90f0-83df133b58cc'} > Have you considered moving this to top of the module? Did it look better/wo No good reason to be "public", I can rename it. A mildly valid reason to keep them here in this class and not in the module is to reduce the noise, but it is not a compelling reason. Line 902: Line 903: @MonkeyPatch(libvirtconnection, 'get', lambda x: fake.Connection()) Line 904: @permutations([[define.NORMAL], [define.ERROR]]) Line 905: def testTimeOffsetNotPresentByDefault(self, exitCode): Line 984: 'graphicsType': graphicsType, Line 985: 'password': '***', Line 986: 'ttl': 0, Line 987: 'existingConnAction': 'disconnect', Line 988: 'params': self.TICKET_PARAMS} > It would be nicer to have pep8 name and have this as a property imho (in a good point. Line 989: Line 990: def _verifyDeviceUpdate(self, device, allDevices, domXml, devXml): Line 991: def _check_ticket_params(domXML, conf, params): Line 992: self.assertEqual(params, self.TICKET_PARAMS) Line 1050: Line 1051: @permutations([[libvirt.VIR_ERR_OPERATION_DENIED, 'setNumberOfCpusErr', Line 1052: 'Failed to set the number of cpus'], Line 1053: [libvirt.VIR_ERR_NO_DOMAIN, 'noVM', None]]) Line 1054: def testSetNumberOfVcpusFailed(self, virtError, vdsmError, errorMessage): > Lots of non-pep8 naming :( Yes, and only because of sloppiness. Will fix. Line 1055: with MonkeyPatchScope([(hooks, 'before_set_num_of_cpus', Line 1056: lambda: None)]): Line 1057: with fake.VM() as testvm: Line 1058: testvm._dom = fake.Domain(virtError=virtError, Line 1062: Line 1063: self.assertEqual(res, response.error(vdsmError)) Line 1064: Line 1065: def testUpdateDeviceGraphicsFailed(self): Line 1066: def _check_ticket_params(domXML, conf, params): > This is repeeating approach in _verifyDeviceUpdate and here, I'd say either Will factor out. Line 1067: pass Line 1068: Line 1069: with MonkeyPatchScope([(hooks, 'before_vm_set_ticket', Line 1070: _check_ticket_params)]): -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: [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
