Nir Soffer has posted comments on this change. Change subject: terminating tests - verify process death ......................................................................
Patch Set 6: (7 comments) https://gerrit.ovirt.org/#/c/52362/6/tests/utilsTests.py File tests/utilsTests.py: Line 92: Line 93: Line 94: class TerminatingTests(TestCaseBase): Line 95: Line 96: def setUp(self): All the tests are doing the same setup, how about adding this to setup? self.proc = commands.execCmd([EXT_SLEEP, "2"], sync=False) self.proc_path = "/proc/%d" % self.proc.pid self.kill_proc = self.proc.kill self.assertTrue(os.path.exists(self.proc_path)) Line 97: self.reaped = set() Line 98: Line 99: def test_terminating(self): Line 100: proc = commands.execCmd([EXT_SLEEP, "5"], sync=False) Line 96: def setUp(self): Line 97: self.reaped = set() Line 98: Line 99: def test_terminating(self): Line 100: proc = commands.execCmd([EXT_SLEEP, "5"], sync=False) 2 seconds sleep is enough, this test will take less then 0.1 seconds most of the time. Line 101: proc_path = "/proc/%d" % proc.pid Line 102: self.assertTrue(os.path.exists(proc_path)) Line 103: with utils.terminating(proc): Line 104: self.assertTrue(os.path.exists(proc_path)) Line 101: proc_path = "/proc/%d" % proc.pid Line 102: self.assertTrue(os.path.exists(proc_path)) Line 103: with utils.terminating(proc): Line 104: self.assertTrue(os.path.exists(proc_path)) Line 105: self.assertTrue(wait_for_removal(proc_path, timeout=0.1)) This may fail on overloaded slave, use at least 1 second timeout. Line 106: Line 107: def test_terminating_with_kill_exception(self): Line 108: class FakeKillError(Exception): Line 109: pass Line 121: proc.kill = fake_kill Line 122: with utils.terminating(proc): Line 123: self.assertTrue(os.path.exists(proc_path)) Line 124: # kill failed, pid should be registered to zombiereaper Line 125: self.assertTrue(proc.pid in self.reaped) If the assert fails, we are not killing the process. Use try-finally to ensure we kill it, or kill it in tearDown. Line 126: real_kill() Line 127: self.assertFalse(wait_for_removal(proc_path, timeout=0.1)) Line 128: Line 129: def test_terminating_with_infected_kill(self): Line 123: self.assertTrue(os.path.exists(proc_path)) Line 124: # kill failed, pid should be registered to zombiereaper Line 125: self.assertTrue(proc.pid in self.reaped) Line 126: real_kill() Line 127: self.assertFalse(wait_for_removal(proc_path, timeout=0.1)) Not needed, no chance that kill will fail here. Line 128: Line 129: def test_terminating_with_infected_kill(self): Line 130: with MonkeyPatchScope([(zombiereaper, Line 131: 'autoReapPID', Line 136: real_kill = proc.kill Line 137: proc.kill = lambda: None Line 138: with utils.terminating(proc): Line 139: self.assertTrue(os.path.exists(proc_path)) Line 140: self.assertTrue(proc.pid in self.reaped) Same, failed test will not kill the process. Line 141: real_kill() Line 142: self.assertFalse(wait_for_removal(proc_path, timeout=0.1)) Line 143: Line 144: Line 138: with utils.terminating(proc): Line 139: self.assertTrue(os.path.exists(proc_path)) Line 140: self.assertTrue(proc.pid in self.reaped) Line 141: real_kill() Line 142: self.assertFalse(wait_for_removal(proc_path, timeout=0.1)) Not needed Line 143: Line 144: Line 145: class RetryTests(TestCaseBase): Line 146: def testStopCallback(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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
