Dima Kuznetsov has posted comments on this change. Change subject: signals: Handle signals to non-main threads ......................................................................
Patch Set 6: (13 comments) 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 under Done 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, s Done 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 You're right 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. Done 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 s Done 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? yes, want to make sure it works more than once (because 1st call is a bit different from the rest) 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? yes, because upon event, we read all the bytes written to pipe, so i can fire 10 signals and wake up only twice. 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 I am running python with unbuffered stdin/out (-u flag). A bit hacky but I think it is good enough for a script this simple. 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. I think it over complicates and is unnecessary for tests simple as those (for example what if the timeout fires twice before I manage to send a SIGTERM). I did however move out the prologue and the epilogue of each test and they are much tidier now. 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. Done 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. Done 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(): Done 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? I use some arguments -- 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
