Dan Kenigsberg has posted comments on this change. Change subject: tests: Improve utils.retry tests ......................................................................
Patch Set 1: (3 comments) https://gerrit.ovirt.org/#/c/46399/1//COMMIT_MSG Commit Message: Line 11: This patch adds 4 tests, revealing issues in the current code: Line 12: Line 13: - Test that we bail out when the deadline is reached (broken). Line 14: - Test the special case when running the operation did not time out, but Line 15: we don't have time for sleep (broken). suggested rephrase: test that we do not wait needlessly if the a sleep would make timeout expire. (if this is indeed the case that worries you) Line 16: - Test the special case when deadline has passed when the operation was Line 17: finished (ok). Line 18: - Test the special case of zero timeout (ok). Line 19: https://gerrit.ovirt.org/#/c/46399/1/tests/utilsTests.py File tests/utilsTests.py: Line 58: def __call__(self): Line 59: return self.now Line 60: Line 61: Line 62: def fake_sleep(seconds): > Yes, this dependency is ugly, but FakeTime does not know anything about uti How about placing fake_sleep as a FakeTime method? FakeTime can then be instantiated in the module level, and passed to both monkey-patches. The current code has an awkward surprise factor ("but monotonic_time does not have a 'now' attribute" was my first impression of it). Line 63: utils.monotonic_time.now += seconds Line 64: Line 65: Line 66: class RetryTests(TestCaseBase): Line 93: # time action Line 94: # 0 first attempt Line 95: # 1 sleep Line 96: # 2 second attempt Line 97: # 3 bail out (3 == deadline) > Sure, I think this interpretation is more useful, specially when dealing wi Nir, I don't understand the broken condition. Are you referring to us not using ">=" in ((monotonic_time() - startTime) > timeout) ? Line 98: Line 99: def operation(): Line 100: time.sleep(1) Line 101: raise RuntimeError -- To view, visit https://gerrit.ovirt.org/46399 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I62062f36e20d263b1aed64135345d496cb7cfd61 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[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: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
