Nir Soffer has posted comments on this change. Change subject: imagetickets: add tests ......................................................................
Patch Set 7: (10 comments) Nice to see 100% coverage! https://gerrit.ovirt.org/#/c/52900/7/tests/imagetickets_test.py File tests/imagetickets_test.py: Line 101: expected = [ Line 102: ("request", ("PATCH", "/tickets/uuid"), {"timeout": timeout}), Line 103: ] Line 104: Line 105: self.assertTrue(imagetickets.uhttp.__calls__, expected) assertEqual Line 106: self.assertTrue(imagetickets.uhttp.closed) Line 107: Line 108: @MonkeyPatch(imagetickets, 'uhttp', FakeUHTTP()) Line 109: def test_remove_ticket(self): Line 108: @MonkeyPatch(imagetickets, 'uhttp', FakeUHTTP()) Line 109: def test_remove_ticket(self): Line 110: imagetickets.remove_ticket("uuid") Line 111: expected = [ Line 112: ("request", ("PATCH", "/tickets/uuid"), None), Patch? Line 113: ] Line 114: Line 115: self.assertTrue(imagetickets.uhttp.__calls__, expected) Line 116: self.assertTrue(imagetickets.uhttp.closed) Line 111: expected = [ Line 112: ("request", ("PATCH", "/tickets/uuid"), None), Line 113: ] Line 114: Line 115: self.assertTrue(imagetickets.uhttp.__calls__, expected) assertEqual - this error hides the bad expected value Line 116: self.assertTrue(imagetickets.uhttp.closed) Line 117: Line 118: @MonkeyPatch(imagetickets, 'uhttp', FakeUHTTP()) Line 119: def test_res_header_error(self): Line 118: @MonkeyPatch(imagetickets, 'uhttp', FakeUHTTP()) Line 119: def test_res_header_error(self): Line 120: imagetickets.uhttp.response.status = 300 Line 121: Line 122: def str_ret(*args, **kwargs): getheader would be a better name, and we don't need to accept *args and **kwargs. We should accespt the same signature that the real getheader excepts. Also the value returned can be more clear - what is wrong in "string"? def getheader(name, default=None): return "invalid content legnth" Line 123: return "string" Line 124: Line 125: imagetickets.uhttp.response.getheader = str_ret Line 126: with self.assertRaises(se.ImageDaemonError): Line 129: @MonkeyPatch(imagetickets, 'uhttp', FakeUHTTP()) Line 130: def test_res_invalid_json_ret(self): Line 131: imagetickets.uhttp.response.status = 300 Line 132: Line 133: def not_json_ret(*args, **kwargs): Same as previous test: def getheader(name, defualt=None): return "not a json string" Line 134: return "string" Line 135: Line 136: imagetickets.uhttp.response.read = not_json_ret Line 137: with self.assertRaises(se.ImageDaemonError): Line 140: @MonkeyPatch(imagetickets, 'uhttp', FakeUHTTP()) Line 141: def test_image_daemon_error_ret(self): Line 142: imagetickets.uhttp.response.status = 300 Line 143: Line 144: def image_daemon_fake_ret(*args, **kwargs): Same: def read(amt=None): return ... Line 145: return '{"image_daemon_message":"content"}' Line 146: Line 147: imagetickets.uhttp.response.read = image_daemon_fake_ret Line 148: try: Line 143: Line 144: def image_daemon_fake_ret(*args, **kwargs): Line 145: return '{"image_daemon_message":"content"}' Line 146: Line 147: imagetickets.uhttp.response.read = image_daemon_fake_ret We should also fake getheader to return the correct content_length value, and return only the bytes requested by the caller. I think the best way to do this is implement getheader and read in our fake response like this: class FakeResponse(object): def __init__(self, status=200, reason="OK", headers=None, data=""): self.status = status self.reason = reason self.headers = headers or {"content-length": len(data)} self.file = io.StringIO(data) def getheader(self, name, default=None): return self.headers.get(name, default) def read(self, amt=None): return self.file.read(amt) Now we can construct the response we like for each test, without faking any response methods: imagetickets.uhttp.response = FakeResponse( code=400, reason="BadRequest", data={"key": "value"} ) Line 148: try: Line 149: imagetickets.remove_ticket("uuid") Line 150: except se.ImageDaemonError as e: Line 151: self.assertTrue("image_daemon_message" in e.value) Line 147: imagetickets.uhttp.response.read = image_daemon_fake_ret Line 148: try: Line 149: imagetickets.remove_ticket("uuid") Line 150: except se.ImageDaemonError as e: Line 151: self.assertTrue("image_daemon_message" in e.value) I think we expect: "image_daemon_message=message" in ... Line 152: Line 153: @MonkeyPatch(imagetickets, 'uhttp', FakeUHTTP()) Line 154: def test_res_read_error(self): Line 155: imagetickets.uhttp.response.status = 300 Line 154: def test_res_read_error(self): Line 155: imagetickets.uhttp.response.status = 300 Line 156: err_msg = "Environment error message" Line 157: Line 158: def env_err(*args, **kwargs): Same: def read(amt=None): raise ... Line 159: raise EnvironmentError(err_msg) Line 160: imagetickets.uhttp.response.read = env_err Line 161: Line 162: try: Line 168: @permutations([[httplib.HTTPException], [socket.error], [OSError]]) Line 169: def test_image_tickets_error(self, exc_type): Line 170: ticket = create_ticket(uuid="uuid") Line 171: Line 172: def failing_request(*args, **kwargs): Better have the same signature of the real request. Will fail with TypeError if the code is calling it in the wrong way. Otherwise the test may succeed when the real code is failing. Line 173: raise exc_type Line 174: Line 175: imagetickets.uhttp.request = failing_request Line 176: with self.assertRaises(se.ImageTicketsError): -- To view, visit https://gerrit.ovirt.org/52900 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2728851a91529ec35501f423d9a798af961fb82a Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Amit Aviram <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[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
