Repository navigation
Fix test discovery for test_concurrent_futures.py #61172
Description
| BPO | 16968 |
|---|---|
| Nosy | @brettcannon, @ezio-melotti, @cjerdonek, @vadmium, @zware, @serhiy-storchaka |
| PRs | |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2013-01-14.22:19:34.136>
labels = ['3.7', '3.8', 'type-bug', 'tests']
title = 'Fix test discovery for test_concurrent_futures.py'
updated_at = <Date 2021-09-17.19:33:58.330>
user = 'https://github.com/zware'bugs.python.org fields:
activity = <Date 2021-09-17.19:33:58.330>
actor = 'serhiy.storchaka'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Tests']
creation = <Date 2013-01-14.22:19:34.136>
creator = 'zach.ware'
dependencies = []
files = ['28729', '28819', '28881', '28896', '31191']
hgrepos = []
issue_num = 16968
keywords = ['patch']
message_count = 26.0
messages = ['179983', '179984', '179985', '179992', '179993', '179997', '179998', '180041', '180551', '180566', '180567', '180689', '180860', '180876', '180917', '180950', '186531', '194533', '194643', '194910', '222058', '222107', '273260', '321072', '321160', '402081']
nosy_count = 6.0
nosy_names = ['brett.cannon', 'ezio.melotti', 'chris.jerdonek', 'martin.panter', 'zach.ware', 'serhiy.storchaka']
pr_nums = ['8099']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'pending'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue16968'
versions = ['Python 3.7', 'Python 3.8']
Activity
zware commentedon Jan 14, 2013
Here's an incomplete patch to test_concurrent_futures.py that does not convert from test_main() to unittest.main(); the decorator on test_main has me unsure how to make the conversion. I've attempted moving various parts of the decorator's functions to setUpModule and tearDownModule functions, but always wind up with a warning that the test has modified threading._dangling.
Guidance on this one would be much appreciated :)
brettcannon commentedon Jan 14, 2013
The way I would replace test_main() would be:
zware commentedon Jan 14, 2013
That looks quite good, except for the fact that discovery (of the form
python -m unittest discover [Lib/test/](https://github.com/python/cpython/blob/main/Lib/test) 'test_*.py') won't hit reap_threads or reap_children. It does coverpython -m test.test_concurrent_futuresand standard regrtest, though; is 2 out of three good enough?brettcannon commentedon Jan 14, 2013
Yes as even the current solution doesn't work with
-m unittest discoveranyway, right? And if you are running the tests independently then it is a separate process and thus you don't need to worry about dangling threads or processes.zware commentedon Jan 14, 2013
Ok then, I'll update the patch. Thanks, Brett!
cjerdonek commentedon Jan 15, 2013
If you want to get the same cleanup logic working with unittest discovery, you could try using the load_tests protocol, and wrapping the functions that actually run the tests there (which may simply mean wrapping the tests passed to load_tests, which I believe are callables).
cjerdonek commentedon Jan 15, 2013
By the way, I think this process of using unittest (i.e. dog-fooding) is a good exercise in part because it helps us understand better where unittest could use improvement (e.g. the SkipTest during import issue, and the decorator issue raised here).
zware commentedon Jan 15, 2013
Hmmm, actually, I was wrong; Brett's suggestion doesn't cover running as part of regrtest. It looks like Chris is right and some kind of load_tests magic will have to happen. I'm looking into it now.
zware commentedon Jan 24, 2013
I'm at a loss on this one. I tried a few different methods of wrapping the tests in load_tests(), I tried doing support.threading_setup() in setUpModule and support.threading_cleanup() in tearDownModule, tried the same in each test class (following the example of test_asynchat.TestAsynchat). Every way I've tried, I still get the warning that threading._dangling was modified when running the test as part of regrtest, even though it appears that everything is getting called at the right time with the right arguments.
I did just stumble onto something interesting: if you simply remove the decorator and finally clause from test_main and run it (with the initial discovery patch applied), there is no warning at the end. Remove test_main and there's the warning. Add a load_tests that builds a suite in what appears to be the same order as what support.run_unittest does, and the warning is still there. I can't find what makes the difference.
The attached diff is the best I've come up with that seems to do the right things with the reap functions, but still causes the warning on my machine.
Am I missing something obvious here or what?
cjerdonek commentedon Jan 25, 2013
I looked into this a bit. It seems like this is because with the patch, the call to "loader.loadTestsFromModule(the_module)" inside regrtest comes before the try-finally:
http://hg.python.org/cpython/file/fcdb35b114ab/Lib/test/regrtest.py#l1277
whereas with the current code, the analogous test-loading code is part of test.support.run_unittest() and so is protected by the try-finally inside test_main(). Apparently, simply discovering/loading tests from test_concurrent_futures.py is enough to modify threading._dangling (e.g. when finding the tests to pass to load_tests).
I'm not sure yet what the right solution is, but it doesn't seem like test discovery should have that side effect. It could be because of how test_concurrent_futures is written, or because of certain initialization code in one of the modules it depends on.
cjerdonek commentedon Jan 25, 2013
I confirmed the above by changing regrtest as follows in the patch (moving the decorator from load_tests()), and trying both placements of the loadTestsFromModule() line. One gives the warning and one does not:
This is mostly a suggestion though -- not 100%.
cjerdonek commentedon Jan 26, 2013
Okay, I think I understand the issue better now. The threading._dangling warning happens because when leaving the saved_test_environment context manager in regrtest:
http://hg.python.org/cpython/file/fcdb35b114ab/Lib/test/regrtest.py#l1271
the context manager finds threads still in threading._dangling that weren't there at the beginning. I think the threads are still in there because the "tests" variable in the code linked to above is holding references to those threads (which isn't surprising given what test_concurrent_futures.py tests). So this is preventing the threads (which are in a WeakSet) from being garbage collected.
On my machine, severing those references before leaving the context manager solves the problem. You can do this by adding "test = 0" after the test_runner() line, or defining "tests" inside an inner function as follows:
To be completely sure, we may also need to do something to ensure that the threads are garbage collected before leaving the context manager. Currently, an explicit call to support.gc_collect() happens in threading_cleanup() (which is in turn called by reap_threads(), etc):
http://hg.python.org/cpython/file/fcdb35b114ab/Lib/test/support.py#l1665
But I'm not sure if that's late enough in the process to guarantee garbage collection of those threads with the test_runner() change above (since the tests list might still be defined).
zware commentedon Jan 28, 2013
Thank you, Chris. I'm rather ashamed of how long I've spent beating my head on this issue and missed the spare tests reference in runtest_inner.
Simply removing the "tests" name entirely clears things up, if this isn't too ugly:
As far as the reap_threads wrapper and reap_children follow-up, I think the TestSuite subclass and load_tests function in the last patch I uploaded may be about the simplest way to keep them for this test without adding them to all tests (by adding it to regrtest.runtest_inner). If anyone thinks the 'ReapedSuite' class (or a better named copy) could be useful elsewhere, it might could go in test.support which would make test_concurrent_futures look a little cleaner.
Patch v3 is v2 plus the regrtest change inline above.
6 remaining items
serhiy-storchaka commentedon Aug 11, 2013
I think this problem deserves a discussion on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/141068 .
BreamoreBoy commentedon Jul 1, 2014
I followed the link in msg194910 to python-dev but didn't find a single response so where does that leave us? I ask because I'd like to help out with the outstanding test issues, but there are so many of them and they seem so interlinked that I don't know where to start. Please advise.
zware commentedon Jul 2, 2014
Since then, Senthil Kumaran converted test_urllib2_localnet (and another urllib test or two) to unittest.main, using this:
"""
threads_key = None
Since nobody has said anything against that in the two months since it was done, that's what I was going to go with when I got back to this; support.reap_children() can just be called in tearDownModule. If you'd like to provide a patch, Mark, please do so!
vadmium commentedon Aug 21, 2016
I don’t know much about the concurrent.futures testing, but in general IMO it makes more sense to call thread.join(), or at least @reap_threads, in each individual test case that needs it. If appropriate, you can call join() with a one-second timeout, which should be functionally equivalent to @reap_threads. Leaving a background thread running while you start another test seems like a bad idea; concurrent tests aren’t meant to be run in the same process.
Also, I added a gc_collect() call to the test infrastructure in bpo-27787, which may help avoid problems with spurious “dangling” thread object references.
zware commentedon Jul 5, 2018
Looking at this again after 5 years, things have changed a bit and I also better understand what's actually going on in the tests. IIUC, the
test_mainfunction's only real purpose was to cause any dangling threads or processes to be reaped, which should actually be done by the shutdown methods of the tested executors. Doing that cleanup in test_main (if successful), would actually prevent us from getting useful warnings from regrtest if shutdown did not clean up properly, so the attached PR simply removes test_main.serhiy-storchaka commentedon Jul 6, 2018
Seems that reap_children() in regrtest is ran only if the test is passed. And it doesn't produce warnings.
serhiy-storchaka commentedon Sep 17, 2021
test_concurrent_futures was made discoverable by Victor Stinner in bpo-37421. It used tearDownModule() to run threading_cleanup() and reap_children().
erlend-aasland commentedon May 16, 2022
Sounds like we can close this, then.
erlend-aasland commentedon May 19, 2022
Fixed by gh-14563 in gh-81602.