Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions ......................................................................
Patch Set 7: (7 comments) I like the tests, see the 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()? 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 and their effect, just to make it easier to understand the tests. 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) 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). 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: def succeed_with_return(self, ret): return ret Then we can test returning dict with one key, multiple keys, dict with status keys, something else, etc. 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: def succeed_with_return(self, ret): return ret Then we can test returning dict with one key, multiple keys, dict with status keys, something else, etc. 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: succeed_with_args(self, *args): succeed_with_kwargs(self, **kwargs) succeed_with_args_and_kwargs(self, *args, **kwargs) -- 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 <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
