Nir Soffer has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 7: (5 comments) http://gerrit.ovirt.org/#/c/29392/7/tests/sigutilsTests.py File tests/sigutilsTests.py: Line 39: raise e Line 40: else: Line 41: break Line 42: Line 43: def _popen_helper(self, *args): We don't need to make stuff private in tests. Line 44: return subprocess.Popen([CHILD_SCRIPT] + list(args), Line 45: stdout=subprocess.PIPE) Line 46: Line 47: def test_signal_received(self): Line 57: now = time.time() Line 58: self.assertRead(popen.stdout, 'ready\n') Line 59: self.assertRead(popen.stdout, 'done\n') Line 60: later = time.time() Line 61: self.assertTrue((later - now) < TIMEOUT * 3) # 3 is safety factor Should be TIMEOUT < elapsed < TIMEOUT * 3 We want this to fail if the wait was too short (but in the polling code?), or too long. Line 62: Line 63: def test_signal_10_times(self): Line 64: TIMES = 10 Line 65: popen = self._popen_helper('test_signal_times', str(TIMES)) Line 63: def test_signal_10_times(self): Line 64: TIMES = 10 Line 65: popen = self._popen_helper('test_signal_times', str(TIMES)) Line 66: self.assertRead(popen.stdout, 'ready\n') Line 67: for _ in range(TIMES): We don't need the loop, 2 times is enough, see the comments in the version 6. Line 68: time.sleep(0.1) Line 69: popen.send_signal(signal.SIGUSR1) Line 70: self.assertRead(popen.stdout, 'signal\n') Line 71: self.assertRead(popen.stdout, 'done\n') http://gerrit.ovirt.org/#/c/29392/7/tests/sigutilsTests_child.py File tests/sigutilsTests_child.py: Line 24: signal.signal(signal.SIGCHLD, sighandler) Line 25: sys.stdout.write('ready\n') Line 26: func(*args, **kwargs) Line 27: sys.stdout.write('done\n') Line 28: return wrapper I think this added unnecessary complexity. This is equal to having the trivial script I suggested, since we don't have to support running multiple function in the same run of the script. Line 29: Line 30: Line 31: @test_wrapper Line 32: def test_signal_received(): Line 28: return wrapper Line 29: Line 30: Line 31: @test_wrapper Line 32: def test_signal_received(): Don't name this like a test - this is not a test that testing tool should detect and run automatically. These function do not test anything, this is the code using wait_for_signal, tested by the parent process. I would name after what the function does - wait_forever, wait_timeout, wait_for_sigchld. Line 33: sigutils.wait_for_signal() Line 34: Line 35: Line 36: @test_wrapper -- To view, visit http://gerrit.ovirt.org/29392 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5dbcd00cec22ef12f2b6253b016dcbd0aa889583 Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
