Francesco Romani has posted comments on this change. Change subject: virt: common handling of exceptions ......................................................................
Patch Set 7: (7 comments) https://gerrit.ovirt.org/#/c/54664/7/tests/api_response_test.py File tests/api_response_test.py: Line 36: Line 37: def test_success_extra_params(self): Line 38: vmList = ['foobar'] Line 39: res = self.vm.succeed_with_args(vmList=vmList) Line 40: self.assertFalse(response.is_error(res)) > This double negative is confusing - do we have response.is_sucess()? we don't yet :\ Let me try to improve a bit without adding (now) a new helper Line 41: self.assertEquals(res['vmList'], vmList) Line 42: Line 43: def test_success_override_message(self): Line 44: message = 'this message overrides the default' Line 41: self.assertEquals(res['vmList'], vmList) Line 42: Line 43: def test_success_override_message(self): Line 44: message = 'this message overrides the default' Line 45: res = self.vm.succeed_with_args(message=message) > I would use another "verb" (.e.g succeed_with_return) to test return values Done Line 46: self.assertFalse(response.is_error(res)) Line 47: self.assertEquals(res['status']['message'], message) Line 48: Line 49: def test_fail_with_vdsm_exception(self): Line 62: self.id = vm_id Line 63: Line 64: @api.method Line 65: def fail_with_vdsm_exception(self, exc): Line 66: raise exc() > I would call this fail(exception) Done Line 67: Line 68: @api.method Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() Line 66: raise exc() Line 67: Line 68: @api.method Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() > This is not needed, we can use previous method (after rename). Done Line 71: Line 72: @api.method Line 73: def succeed(self): Line 74: pass Line 69: def fail_with_plain_exception(self): Line 70: raise ValueError() Line 71: Line 72: @api.method Line 73: def succeed(self): > Lets add another one with parameter for testing return value: Done Line 74: pass Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): Line 70: raise ValueError() Line 71: Line 72: @api.method Line 73: def succeed(self): Line 74: pass > Lets add another one with parameter for testing return value: Done Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): Line 73: def succeed(self): Line 74: pass Line 75: Line 76: @api.method Line 77: def succeed_with_args(self, **kwargs): > I would test: OK. what those method should return? -- To view, visit https://gerrit.ovirt.org/54664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic186dc8fa062d8b3789c6057bba68fbbc23f311b Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
