Nir Soffer has posted comments on this change. Change subject: virt: common handling of exceptions ......................................................................
Patch Set 9: (3 comments) https://gerrit.ovirt.org/#/c/54664/9/tests/virt_api_test.py File tests/virt_api_test.py: Line 43: def test_success_with_args(self): Line 44: status = { 'vmName': 'foo', 'vmId': 'bar' } Line 45: self.assertRaises(TypeError, Line 46: self.vm.succeed_with_args, Line 47: status) This is a strange success Line 48: Line 49: def test_success_with_kwargs(self): Line 50: port = 0 Line 51: res = self.vm.succeed_with_kwargs(migrationPort=port) Line 70: self.assertEquals(res, expected) Line 71: Line 72: def test_fail_with_plain_exception(self): Line 73: res = self.vm.fail(ValueError) Line 74: self.assertTrue(response.is_error(res)) It seems that we control both the error handler and the code raising the exception, so maybe we can do: e = ValueError() expected = exception.GeneralException(str(e)).response() To make it less fragile, we can pass an instance of the error, and implement fail like this: def fail(self, e): raise e Or if you think this is too fragile, at least check the error code of the response? This seems to be the contract of this wrapper, returning the response of vdsm exceptions, or general exception with the details of the original error. Line 75: Line 76: Line 77: class FakeVM(object): Line 78: Line 89: return ret Line 90: Line 91: @api.method Line 92: def succeed_with_args(self, *args): Line 93: return args We can use single method for these tests: def succeed_with_args_kw(self, *args, **kw): return {"args": args, "kwargs": kwargs} Or return always a dict from both: return {"args": args} And in the following method: return {"kwargs": kwargs} Line 94: Line 95: @api.method Line 96: def succeed_with_kwargs(self, **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: 9 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
