[issue16968] Fix test discovery for test_concurrent_futures.py

Serhiy Storchaka added the comment: test_concurrent_futures was made discoverable by Victor Stinner in issue37421. It used tearDownModule() to run threading_cleanup() and reap_children(). -- status: open -> pending ___ Python tracker

[issue16968] Fix test discovery for test_concurrent_futures.py

Serhiy Storchaka added the comment: Seems that reap_children() in regrtest is ran only if the test is passed. And it doesn't produce warnings. -- ___ Python tracker ___

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: 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_main` function's only real purpose was to cause any dangling threads or processes to be reaped, which should

[issue16968] Fix test discovery for test_concurrent_futures.py

Change by Zachary Ware : -- pull_requests: +7696 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue16968] Fix test discovery for test_concurrent_futures.py

Changes by Mark Lawrence : -- nosy: -BreamoreBoy ___ Python tracker ___ ___

[issue16968] Fix test discovery for test_concurrent_futures.py

Martin Panter added the comment: 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: Since then, Senthil Kumaran converted test_urllib2_localnet (and another urllib test or two) to unittest.main, using this: threads_key = None def setUpModule(): # Store the threading_setup in a key and ensure that it is cleaned up # in the tearDown

[issue16968] Fix test discovery for test_concurrent_futures.py

Mark Lawrence added the comment: 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Serhiy Storchaka added the comment: I think this problem deserves a discussion on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/141068 . -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue16968

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: That's a much better solution, thank you, Serhiy. Here's a new patch. test.support is no longer changed at all, but regrtest.py still is; the extra reference to the tests still causes issues, so it is removed. -- Added file:

[issue16968] Fix test discovery for test_concurrent_futures.py

Serhiy Storchaka added the comment: As far as I understand threads reaping needed only when ThreadPoolExecutor is used and children reaping needed only when ProcessPoolExecutor is used. Why not use them only in tests which needs them? class ThreadPoolMixin(ExecutorMixin): executor_type =

[issue16968] Fix test discovery for test_concurrent_futures.py

Changes by Serhiy Storchaka storch...@gmail.com: -- nosy: +serhiy.storchaka stage: - patch review ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue16968 ___

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: Ping. Version 4 still applies cleanly, and with issue16935 fixed, it works properly. The fix for test_socket is looking like it could use the ReapedSuite from this patch, so would anyone mind looking at this one again and committing if it's qualified?

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: Right you are, Chris. v4 has a comment added to regrtest.runtest_inner pointing back to this issue. Also in v4, ReapedSuite has been moved to test.support. At least one other test module (test_pydoc) uses the same idiom as test_concurrent_futures, and so

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: I failed to mention; v4 also removes setUpModule() which was present in the first 3 patches. With 16935 fixed, setUpModule would be unnecessary. -- ___ Python tracker rep...@bugs.python.org

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Chris Jerdonek added the comment: Whichever solution you pick for the test issue, I would at least add a code comment explaining that the test return value needs to be garbage collected and why, etc. and probably reference this issue. If anyone thinks the 'ReapedSuite' class (or a better

[issue16968] Fix test discovery for test_concurrent_futures.py

Chris Jerdonek added the comment: 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Chris Jerdonek added the comment: 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Chris Jerdonek added the comment: 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 =

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: 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. -- ___ Python

[issue16968] Fix test discovery for test_concurrent_futures.py

New submission from Zachary Ware: 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Brett Cannon added the comment: The way I would replace test_main() would be: if __name__ == '__main__': try: test.support.reap_threads(unittest.main)() finally: test.support_reap_children() -- ___ Python tracker rep...@bugs.python.org

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: That looks quite good, except for the fact that discovery (of the form `python -m unittest discover 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Brett Cannon added the comment: 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. --

[issue16968] Fix test discovery for test_concurrent_futures.py

Zachary Ware added the comment: Ok then, I'll update the patch. Thanks, Brett! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue16968 ___ ___

[issue16968] Fix test discovery for test_concurrent_futures.py

Chris Jerdonek added the comment: 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

[issue16968] Fix test discovery for test_concurrent_futures.py

Chris Jerdonek added the comment: 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).