Change in vdsm[master]: terminating tests - verify process death
nsoffer at redhat.com
nsoffer at redhat.com
Thu Jan 21 08:07:03 UTC 2016
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 <ybronhei at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list