classification
Title: No test for thread.interrupt_main()
Type: behavior Stage: commit review
Components: Versions:
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: charlie.proctor, christian.heimes, r.david.murray
Priority: low Keywords: patch

Created on 2013-12-05 15:20 by christian.heimes, last changed 2016-11-04 20:44 by r.david.murray.

Files
File name Uploaded Description Edit
test_interrupt_main.patch charlie.proctor, 2016-11-04 14:37 initial draft of test_interrupt_main review
test_interrupt_main.patch charlie.proctor, 2016-11-04 16:02 more descriptive comments; restore state in addCleanup review
test_interrupt_main.patch charlie.proctor, 2016-11-04 18:39 divide two cases into two separate tests via expect_sigint helper function review
Messages (8)
msg205306 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-12-05 15:20
Accoring to LCOV thread_PyThread_interrupt_main() is never called by any test. It's only referenced by some idle code.

http://tiran.bitbucket.org/python-lcov/Modules/_threadmodule.c.gcov.html#1110
msg280047 - (view) Author: Charlie Proctor (charlie.proctor) * Date: 2016-11-04 14:37
Found this through the "Random Issue" button -- I've uploaded a simple test case for thread.interrupt_main().

This is my first patch :) So let me know if there's something else I should do and I'd love to hear any feedback...
msg280050 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-11-04 15:07
Thanks, Charlie.

You should use addCleanup to handle the resetting of the state, so that it gets cleaned up no matter what happens in the test.  IMO the comments should either be omitted or be more descriptive about what exactly is being tested.  For example "If this timer goes off, then interrupt_main did not work, so fail the test".

I don't really understand what exactly is being tested in the body...it looks like two tests, one for calling it from the main thread (I suppose it makes sense to test that, but I don't know what behavior is expedted) and one from a subthread, which I would think was the real test.  I would expect the main thread to be catching KeyboardInterrupt, based on the description of interrupt_main, so I'm not even sure what the sigalrm is for.  Can you explain?
msg280054 - (view) Author: Charlie Proctor (charlie.proctor) * Date: 2016-11-04 16:02
Thanks for the feedback David!

I've posted a revised patch with more descriptive comments and the restoration code moved into addCleanup.

As described in the comments, in the context of this test, the "main" thread is the one running the test suite... so rather than self.assertRaises(KeyboardInterrupt), I assert that appropriate SIGINTS are received. The lock is used to facilitate these assertions, since signals are asynchronous.
msg280055 - (view) Author: Charlie Proctor (charlie.proctor) * Date: 2016-11-04 16:06
To clarify further, the SIGALRM handler catches the timeout sent by the ITIMER_REAL... whereas the SIGINT handler catches the interrupt_main() signals.
msg280060 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-11-04 16:46
Ah, of course.  The revised comments look good.

I think I'd rather see the two cases tested separately, but if they are kept as one another comment ("lock was successfully released; reacquire the lock and test that it also works from a sub-thread") might be helpful.
msg280066 - (view) Author: Charlie Proctor (charlie.proctor) * Date: 2016-11-04 18:39
I broke the two cases (interrupt_main from test thread and from sub-thread) into two separate tests, using an "expect_sigint" helper.

Let me know what you think -- thanks!
msg280081 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-11-04 20:44
This looks great, thanks.
History
Date User Action Args
2016-11-04 20:44:22r.david.murraysetmessages: + msg280081
stage: needs patch -> commit review
2016-11-04 18:39:23charlie.proctorsetfiles: + test_interrupt_main.patch

messages: + msg280066
2016-11-04 16:46:27r.david.murraysetmessages: + msg280060
2016-11-04 16:06:00charlie.proctorsetmessages: + msg280055
2016-11-04 16:02:42charlie.proctorsetfiles: + test_interrupt_main.patch

messages: + msg280054
2016-11-04 15:07:30r.david.murraysetnosy: + r.david.murray
messages: + msg280050
2016-11-04 14:37:52charlie.proctorsetfiles: + test_interrupt_main.patch

nosy: + charlie.proctor
messages: + msg280047

keywords: + patch
2013-12-05 15:20:32christian.heimescreate