Nir Soffer has posted comments on this change. Change subject: terminating tests - verify process death ......................................................................
Patch Set 4: (11 comments) Nice tests https://gerrit.ovirt.org/#/c/52362/4/tests/utilsTests.py File tests/utilsTests.py: Line 85: Line 86: def setUp(self): Line 87: self.reaped = set() Line 88: Line 89: def addedToReap(self, pid): Not needed Line 90: self.reaped.add(pid) Line 91: Line 92: def wait_for_removal(self, path, timeout=0.1, wait=0.1): Line 93: deadline = utils.monotonic_time() + timeout 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 1 second. This helper is not part of this test class, and does not use its state. This should be a generic helper that can be used by other tests, and maybe even n utils, for using in the application. I would add it in testlib for now. Also having default timeout with valid value in an api is a bad idea. Typically default timeout means *no* timeout, and you provide a value when you want some timeout. Here we always want a timeout, so this should be a positional argument and the caller must provide the timeout needed. Line 93: deadline = utils.monotonic_time() + timeout Line 94: while True: Line 95: if not os.path.exists(path): Line 96: return True Line 97: if utils.monotonic_time() > deadline: Line 98: return False Line 99: time.sleep(wait) Line 100: Line 101: def testTerminating(self): Can you use new pep8 style? test_terminating Line 102: proc = commands.execCmd([EXT_SLEEP, "5"], sync=False) Line 103: proc_path = "/proc/%d" % proc.pid Line 104: self.assertTrue(os.path.exists(proc_path)) Line 105: with utils.terminating(proc): 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. Line 111: raise Exception() Line 112: Line 113: with MonkeyPatchScope([(zombiereaper, Line 114: 'autoReapPID', Line 107: self.assertTrue(self.wait_for_removal(proc_path)) Line 108: Line 109: def testTerminatingWithKillException(self): Line 110: def raiseExc(): Line 111: raise Exception() You want to raise specific exception, here, certainly not the base class for all exception, hiding real errors in the code (e.g. syntax error). Do: class FakeKillError(Exception): pass def fake_kill(): raise FakeKillError Line 112: Line 113: with MonkeyPatchScope([(zombiereaper, Line 114: 'autoReapPID', Line 115: self.addedToReap Line 111: raise Exception() Line 112: Line 113: with MonkeyPatchScope([(zombiereaper, Line 114: 'autoReapPID', Line 115: self.addedToReap Use self.reaped.add Line 116: )]): Line 117: proc = commands.execCmd([EXT_SLEEP, "5"], sync=False) Line 118: proc_path = "/proc/%d" % proc.pid Line 119: proc.kill = raiseExc # mocking kill func Line 115: self.addedToReap Line 116: )]): Line 117: proc = commands.execCmd([EXT_SLEEP, "5"], sync=False) Line 118: proc_path = "/proc/%d" % proc.pid Line 119: proc.kill = raiseExc # mocking kill func Nice! Line 120: try: Line 121: with utils.terminating(proc): Line 122: self.assertTrue(os.path.exists(proc_path)) Line 123: self.assertNotRaises('Exception should raise') Line 119: proc.kill = raiseExc # mocking kill func Line 120: try: Line 121: with utils.terminating(proc): Line 122: self.assertTrue(os.path.exists(proc_path)) Line 123: self.assertNotRaises('Exception should raise') Not needed, and wrong usage of assertNotRaises. Line 124: except Exception: Line 125: pass Line 126: self.assertTrue(proc.pid in self.reaped) Line 127: self.assertFalse(self.wait_for_removal(proc_path)) 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 specific error our fake kill raises. with self.assertRaises(FakeKillError): with utils.terminating(proc): ... 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): 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): Line 130: def doNothing(): Uneeded Line 131: pass Line 132: Line 133: with MonkeyPatchScope([(zombiereaper, Line 134: 'autoReapPID', Line 135: self.addedToReap Line 136: )]): Line 137: proc = commands.execCmd([EXT_SLEEP, "5"], sync=False) Line 138: proc_path = "/proc/%d" % proc.pid Line 139: proc.kill = doNothing # mocking kill func Use: proc.kill = lambda: None Line 140: with utils.terminating(proc): Line 141: self.assertTrue(os.path.exists(proc_path)) Line 142: self.assertTrue(proc.pid in self.reaped) Line 143: self.assertFalse(self.wait_for_removal(proc_path)) -- 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
