Yaniv Bronhaim has posted comments on this change. Change subject: terminating tests - verify process death ......................................................................
Patch Set 4: (3 comments) https://gerrit.ovirt.org/#/c/52362/4/tests/utilsTests.py File tests/utilsTests.py: Line 88: Line 89: def addedToReap(self, pid): Line 90: self.reaped.add(pid) Line 91: Line 92: def wait_for_removal(self, path, timeout=0.1, wait=0.1): > timeout=0.1 is way too low, this will fail on CI every week. Use at least currently we use it only here. testlib is a garbage of such things that might be used by others, but never been used more than once... agree about the default, but I prefer to keep the method here. if you'll need it later in life, feel free to move it to utils. Line 93: deadline = utils.monotonic_time() + timeout Line 94: while True: Line 95: if not os.path.exists(path): Line 96: return True Line 106: self.assertTrue(os.path.exists(proc_path)) Line 107: self.assertTrue(self.wait_for_removal(proc_path)) Line 108: Line 109: def testTerminatingWithKillException(self): Line 110: def raiseExc(): > Use pep8 style (lower_case) for all names except class names. you're making a mess with the current convention in more of vdsm code :/ better stick to one convention in all project than follow pep8 imo. please decide with dan (your co) about vdsm code style standards, and make it mandatory in all your and others reviews. for now I changed it to lower_case names Line 111: raise Exception() Line 112: Line 113: with MonkeyPatchScope([(zombiereaper, Line 114: 'autoReapPID', Line 121: with utils.terminating(proc): Line 122: self.assertTrue(os.path.exists(proc_path)) Line 123: self.assertNotRaises('Exception should raise') Line 124: except Exception: Line 125: pass > You are hiding real error in the code here. We must assert about the specif it doesn't as expected ====================================================================== FAIL: test_terminating_with_kill_exception (utilsTests.TerminatingTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/ybronhei/Projects/vdsm/tests/utilsTests.py", line 122, in test_terminating_with_kill_exception self.assertTrue(os.path.exists(proc_path)) AssertionError: FakeKillError not raised -------------------- >> begin captured logging << -------------------- root: DEBUG: /usr/bin/taskset --cpu-list 0-3 sleep 5 (cwd None) root: DEBUG: Terminating process pid=5463 root: ERROR: Failed to kill process 5463 Traceback (most recent call last): File "/home/ybronhei/Projects/vdsm/lib/vdsm/utils.py", line 779, in terminating proc.kill() File "/home/ybronhei/Projects/vdsm/tests/utilsTests.py", line 111, in fake_kill raise FakeKillError("fake kill exception") FakeKillError: fake kill exception --------------------- >> end captured logging << --------------------- ---------------------------------------------------------------------- Line 126: self.assertTrue(proc.pid in self.reaped) Line 127: self.assertFalse(self.wait_for_removal(proc_path)) Line 128: Line 129: def testTerminatingWithInfectedKill(self): -- To view, visit https://gerrit.ovirt.org/52362 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib1600a298b3faaffe3cc72b4eb62201370bf03c7 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <[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: Yaniv Bronhaim <[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
