Fix test discovery for test_concurrent_futures.py

[zware]

BPO	16968
Nosy	@brettcannon, @ezio-melotti, @cjerdonek, @vadmium, @zware, @serhiy-storchaka
PRs	bpo-16968: Clean up test_concurrent_futures #8099
Files	test_concurrent_futures_discovery.diff: Fix discovery, but not test_main test_concurrent_futures_discovery.v2.diff: v2, test_main replaced but gives warning test_concurrent_futures_discovery.v3.diff: Version 3 test_concurrent_futures_discovery.v4.diff: Version 4 test_concurrent_futures_discovery.v5.diff: Version 5

^{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']

[zware]

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]

The way I would replace test_main() would be:

if __name__ == '__main__':
 try:
 test.support.reap_threads(unittest.main)()
 finally:
 test.support_reap_children()

[zware]

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 cover python -m test.test_concurrent_futures and standard regrtest, though; is 2 out of three good enough?

[brettcannon]

Yes as even the current solution doesn't work with -m unittest discover anyway, 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]

Ok then, I'll update the patch. Thanks, Brett!

[cjerdonek]

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]

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]

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]

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]

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]

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:

 if test_runner is None:
 loader = unittest.TestLoader()
 #tests = loader.loadTestsFromModule(the_module)
 @support.reap_threads
 def test_runner():
 try:
 tests = loader.loadTestsFromModule(the_module)
 support.run_unittest(tests)
 finally:
 support.reap_children()
 test_runner()

This is mostly a suggestion though -- not 100%.

[cjerdonek]

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:

if test_runner is None:
 def test_runner():
 tests = unittest.TestLoader().loadTestsFromModule(the_module)
 support.run_unittest(tests)
test_runner()

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]

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:

diff -r 5f655369ef06 Lib/test/regrtest.py
--- a/Lib/test/regrtest.py Mon Jan 28 13:27:02 2013 +0200
+++ b/Lib/test/regrtest.py Mon Jan 28 08:50:59 2013 -0600
@@ -1275,8 +1275,8 @@
 # tests. If not, use normal unittest test loading.
 test_runner = getattr(the_module, "test_main", None)
 if test_runner is None:
- tests = unittest.TestLoader().loadTestsFromModule(the_module)
- test_runner = lambda: support.run_unittest(tests)
+ test_runner = lambda: support.run_unittest(
+ unittest.TestLoader().loadTestsFromModule(the_module))
 test_runner()
 if huntrleaks:
 refleak = dash_R(the_module, test, test_runner,

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.