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