Nir Soffer has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 6: (13 comments) The tests are very nice, but need some more work, mainly simplifying and removing duplication. http://gerrit.ovirt.org/#/c/29392/6/tests/sigutilsTests.py File tests/sigutilsTests.py: Line 28: Line 29: Line 30: class SigutilsTests(TestCaseBase): Line 31: Line 32: def assertRead(self, where, what): Where and what are little bit too generic, which makes this harder to understand. How about: def assertRead(self, file, expected): ... However, it is very nice and simple way to verify. Line 33: self.assertEquals(where.read(len(what)), what) Line 34: Line 35: def testSignalReceived(self): Line 36: popen = subprocess.Popen([CHILD_SCRIPT, 'testSignalReceived'], Line 31: Line 32: def assertRead(self, where, what): Line 33: self.assertEquals(where.read(len(what)), what) Line 34: Line 35: def testSignalReceived(self): Is there a reason to use mixedCase? You new module uses underscore_names, so lets be consistent. Line 36: popen = subprocess.Popen([CHILD_SCRIPT, 'testSignalReceived'], Line 37: stdout=subprocess.PIPE) Line 38: self.assertRead(popen.stdout, 'ready\n') Line 39: time.sleep(0.1) Line 35: def testSignalReceived(self): Line 36: popen = subprocess.Popen([CHILD_SCRIPT, 'testSignalReceived'], Line 37: stdout=subprocess.PIPE) Line 38: self.assertRead(popen.stdout, 'ready\n') Line 39: time.sleep(0.1) I think we don't need this sleep - we don't care what the other process is doing. Line 40: popen.send_signal(signal.SIGUSR1) Line 41: self.assertRead(popen.stdout, 'signal\n') Line 42: self.assertRead(popen.stdout, 'done\n') Line 43: Line 42: self.assertRead(popen.stdout, 'done\n') Line 43: Line 44: def testSignalTimeout(self): Line 45: popen = subprocess.Popen([CHILD_SCRIPT, 'testSignalTimeout'], Line 46: stdout=subprocess.PIPE) Lests create a function to run the child with specific test name. def run_child_test(name): return subprocess.Popen([CHILD_SCRIPT, 'testSignalTimeout'], stdout=subprocess.PIPE) Line 47: self.assertRead(popen.stdout, 'ready\n') Line 48: self.assertRead(popen.stdout, 'done\n') Line 49: Line 50: def testSignal10Times(self): Line 44: def testSignalTimeout(self): Line 45: popen = subprocess.Popen([CHILD_SCRIPT, 'testSignalTimeout'], Line 46: stdout=subprocess.PIPE) Line 47: self.assertRead(popen.stdout, 'ready\n') Line 48: self.assertRead(popen.stdout, 'done\n') In this case we would like to assert that we got done after more then 0.1 seconds, and less then some safe value (e.g. 0.2) Line 49: Line 50: def testSignal10Times(self): Line 51: TIMES = 10 Line 52: popen = subprocess.Popen([CHILD_SCRIPT, Line 46: stdout=subprocess.PIPE) Line 47: self.assertRead(popen.stdout, 'ready\n') Line 48: self.assertRead(popen.stdout, 'done\n') Line 49: Line 50: def testSignal10Times(self): How is this different from testSignalReceived? Line 51: TIMES = 10 Line 52: popen = subprocess.Popen([CHILD_SCRIPT, Line 53: 'testSignalTimes', Line 54: str(TIMES)], Line 54: str(TIMES)], Line 55: stdout=subprocess.PIPE) Line 56: self.assertRead(popen.stdout, 'ready\n') Line 57: for _ in range(TIMES): Line 58: time.sleep(0.1) Needed? Line 59: popen.send_signal(signal.SIGUSR1) Line 60: self.assertRead(popen.stdout, 'signal\n') Line 61: self.assertRead(popen.stdout, 'done\n') Line 62: http://gerrit.ovirt.org/#/c/29392/6/tests/sigutilsTests_child.py File tests/sigutilsTests_child.py: Line 13: Line 14: Line 15: def sighandler(signum, frame): Line 16: counter[0] += 1 Line 17: sys.stdout.write('signal\n') You should to flush the file, since when running with a pipe, stdout is not line buffered. So you will actually wait until all the process exists, instead of receiving the output immediately. For example, try this test: #pipe.py import sys import time for i in range(10): sys.stdout.write('%i\n' % i) time.sleep(1) And run it like this: python pipe.py | less Then add a flush after each write and run it again. I would add a function to send data to the parent: def send_to_parent(s): sys.stdout.write(s + '\n') sys.stdout.flush() Line 18: Line 19: Line 20: def testSignalReceived(): Line 21: signal.signal(signal.SIGUSR1, sighandler) Line 36: signal.signal(signal.SIGUSR1, sighandler) Line 37: sys.stdout.write('ready\n') Line 38: while counter[0] < times: Line 39: sigutils.wait_for_signal() Line 40: sys.stdout.write('done\n') These functions are almost the same. Lets move the common parts to the module level: running[0] = True def handle_sigchld(signo, frame): send_to_parent('received sigchld') def handle_sigusr1(signo, frame): send_to_parent('received sigusr1') def handle_sigterm(signo, frame): send_to_parent('received sigterm') running[0] = False def wait_forever(): while running[0]: sigutils.wait_for_signals() def wait_timeout(): while running[0]: sigutils.wait_for_signals(0.1) def thread(): ... signal.signal(signal.SIGUSR1, handle_sigusr1) signal.signal(signal.SIGUSR1, handle_sigusr1) signal.signal(signal.SIGCHLD, handle_sigchld) send_to_parent('ready') globals()[sys.argv[1]]() send_to_parent('done') Now we can test multiple secnarios in the parent: run_helper("wait_forever") expect "ready" send sigterm expect "received_sigterm" expect "wokeup" expect "done" run_helper("wait_timeout") expect "ready" expect "wokeup" send sigterm run_helper("wait_forever") expect "ready" for i in range(10): send sigusr1 expect "received sigusr1" expect "wokeup" send sigterm Line 41: Line 42: Line 43: def testSignalToThread(): Line 44: ''' Line 51: This test makes sure main thread is woken up. Line 52: ''' Line 53: Line 54: def thread_target(): Line 55: subprocess.Popen(['sleep', '1']) Why 1 second? sleep supports floats - 0.1 should be enough. Line 56: time.sleep(3) Line 57: signal.signal(signal.SIGCLD, sighandler) Line 58: sys.stdout.write('ready\n') Line 59: threading.Thread(target=thread_target).start() Line 52: ''' Line 53: Line 54: def thread_target(): Line 55: subprocess.Popen(['sleep', '1']) Line 56: time.sleep(3) Please comment why we need this sleep. Line 57: signal.signal(signal.SIGCLD, sighandler) Line 58: sys.stdout.write('ready\n') Line 59: threading.Thread(target=thread_target).start() Line 60: sigutils.wait_for_signal() Line 62: Line 63: tests = {'testSignalReceived': testSignalReceived, Line 64: 'testSignalTimeout': testSignalTimeout, Line 65: 'testSignalTimes': testSignalTimes, Line 66: 'testSignalToThread': testSignalToThread} You don't need this explicit dict - it alreay exists in globals(): globals()[sys.argv[1]]() Line 67: Line 68: if __name__ == '__main__': Line 65: 'testSignalTimes': testSignalTimes, Line 66: 'testSignalToThread': testSignalToThread} Line 67: Line 68: if __name__ == '__main__': Line 69: tests[sys.argv[1]](*sys.argv[2:]) You don't use arguments, by add code that is not needed? -- 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: 6 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
