Created on 2009-12-06 17:35 by r.david.murray, last changed 2010-05-26 17:35 by haypo. This issue is now closed.
|nothreads.patch||jerry.seutter, 2009-12-11 06:54||Patch so that tests skip rather than get errors when python is compiled --without-threads|
|nothreads_2.patch||jerry.seutter, 2010-03-03 01:07||Reworked the patch to address all issues except the skip_if_no decorator naming|
|nothreads_3.patch||jerry.seutter, 2010-03-05 19:45||Reworked the patch to address issues from 2010-03-02|
|nothreads_4.patch||haypo, 2010-03-07 21:10|
|nothreads-socketserver-shutdown.patch||skrah, 2010-05-13 15:36|
|msg96035 - (view)||Author: R. David Murray (r.david.murray) *||Date: 2009-12-06 17:35|
In the past (<= 2.6) regrtest skipped a test if any import failure happened, which masked various real test failures. This was fixed, and tests that should be skipped if certain modules are not available were changed to use (test_)support.import_module, which causes a skip if that particular module cannot be imported. If python is compiled --without-threads, then the following tests currently 'crash' because they cannot import thread and/or threading: test_hashlib test_asyncore test_wait3 test_threading test_socket test_wait4 test_capi test_xmlrpc test_ctypes test_zipimport_support test_threading_local test_multiprocessing test_file2k test_smtplib test_threadedtempfile test_threadsignals test_thread test_queue test_asynchat test_contextlib test_bz2 test_ftplib test_cmd test_pdb test_io test_doctest test_sqlite test_logging test_telnetlib test_threaded_import test_httpservers test_fork1 test_docxmlrpc test_urllib2_localnet test_poplib test_socketserver All of these tests should either be changed to use import_module when importing thread/threading, or changed so that the tests requiring thread support are skipped if thread support is not available. Note that test_bsddb3 also fails, but it is not an import error crash.
|msg96233 - (view)||Author: Jerry Seutter (jerry.seutter) *||Date: 2009-12-11 06:54|
The patch makes it so that tests that use threading skip rather than generate errors when python is compiled --without-threads. I added a skip_if_no('modulename') decorator to test_support. Let me know if this patch is too big to review and I'll break it up. It is 800 lines. One thing to note: when threading is disabled, all sqlite3 tests are skipped. There are a few test files that do not use threading, but I couldn't think of an easy way to split them out. Tested on OS X and Linux.
|msg100176 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-02-26 22:53|
Comments about nothreads.patch. There are unrelated changes: Lib/test/test_xmlrpc.py: - p = xmlrpclib.ServerProxy(URL, transport=t) + p = xmlrpclib.ServerProxy('http:', transport=t) Lib/test/test_macostools.py (working copy) - try: - os.unlink(TESTFN2) - except: - pass Or you should explain me why you changed that :-) Why not using test_support.import_module('threading') in test_xmlrpc.py, test_file2k.py, etc.? I don't like importing a module in a function: you should use test_support.import_module('threading') in test_io.py, test_logging.py, test_capi.py, etc. skip_if_no() function name is a little bit too generic. I don't have any suggestion.
|msg100322 - (view)||Author: Jerry Seutter (jerry.seutter) *||Date: 2010-03-03 01:07|
In the test_xmlrpc.py case I changed the value from URL to 'http:' because the code that sets URL to a valid url relies on threading. When threading is disabled, URL is set to None and the test will fail. The two ServerProxy test cases that were modified in this way do not actually use the network at all. They instead test that the close() method returns None and that the transport() method returns the transport passed in to the constructor. I figured setting the url to 'http:' instead of an empty string was more readable. The reader would know that the string was supposed to be a url and that it was utterly meaningless in this case. In the test_macostools.py case, the os.unlink(TESTFN2) call is a copy and paste error from the previous test. This test tried to remove an alias it never created, and it failed to check that the destination directory for the alias actually was a directory (it only checked that the path existed - in my case it was a file, not a directory). I fixed the test to check that sys.prefix is a directory, and then clean up sys.prefix/TESTFN2. The skip_if_no decorator is not absolutely necessary and could have been skipped. I believe it adds to the readability of the code because with the decorator it becomes obvious that the test should skip in some cases. Perhaps this is what import_module() is for - if so, should I document it? I also believe the decorator helps prevent cases where a resource is allocated (like creating a directory), then the import_module() call fails and a test artifact is left laying around on disk. Having said that, I do not know if this actually happens in any of the tests and so might be a moot point. In reference to disliking the naming of skip_if_no(), I do not like the naming either. The decorator attempts to import the module, then raises SkipTest if there was an ImportError. I think it is essential to have the words "import" and "skip" in the method name to help indicate what the decorator does. These are names I could live with: import_or_skip_test('threading') import_module_or_skip_test('threading') skip_test_unless_import('threading') My preference is for the last one. Let me know which one you like best and I'll change the name to that.
|msg100323 - (view)||Author: R. David Murray (r.david.murray) *||Date: 2010-03-03 01:46|
I haven't reviewed the whole patch, but I would suggest that instead of making a test_support.skip_if_no (or any other name), you use use @unittest.skipUnless(threading) (note that lack of quotes around threading...you seem to already be getting it defined correctly in all the modules I scaned). That's what other test modules do in similar situations.
|msg100324 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-03-03 02:13|
> In the test_xmlrpc.py case I changed the value from URL to 'http:' because > the code that sets URL to a valid url relies on threading. When threading > is disabled, URL is set to None and the test will fail. I would prefer something like: if support threads: url = URL else: url = "http:" To avoid modifying the current tests in the most common case, Python with threads. > In the test_macostools.py case, the os.unlink(TESTFN2) call is a copy and > paste error from the previous test (...) Is this fix related to threads? If not, please open a different issue with a patch just to fix that. > The skip_if_no decorator is not absolutely necessary and could have been > skipped. I believe it adds to the readability of the code because with > the decorator it becomes obvious that the test should skip in some > cases. Perhaps this is what import_module() is for - if so, should I > document it? (...) I like your decorator, but not it's name :-) import_module() is not a decorator, but your decorator call it, so it's fine. > These are names I could live with: > > import_or_skip_test('threading') > import_module_or_skip_test('threading') > skip_test_unless_import('threading') The main goal is to skip a test, so a name starting with "skip" is a good idea. You might drop "test_" because we already have a context (namespace): the function is defined in the test_support module. @test_support.skip_unless_import('thread') doesn't look bad :-) -- Comments about your new patch. Lib/ctypes/test/test_errno.py is completly skipped whereas only half the tests depends on threads. I don't really like the idea of skipping the whole file if threads are disabled (except for tests only based on tests, like test_threading.py). If someone adds a new test without reading the imports header, he will not notice that the test will be skipped if threads are not available, even if his test doesn't depend on threads. Since Python 2.7 and 3.0+ supports class decorators, why not using a decorator on classes instead of the blacklisting the whole file? I don't know if you decorator can be used on a class without any change. You need maybe to add **kw to the wrapper() function. Lib/test/test_sqlite.py: only Lib/sqlite3/test/dbapi.py depends on threads, all other tests should work without thread. Lib/test/test_urllib2_localnet.py shouldn't inherit from BaseTestCase because it overrides all methods (setUp and tearDown), but simply from unittest.TestCase. This is not directly related to your patch. reap_threads(): you should move the "import thread" to do it only once, and write a dummy decorator if threads are disabled. Something like: if have threads: def reap_threads(func): ... else: def reap_threads(func): # do nothing return func And write a doc string to explain that the function does nothing if threads are disabled. fork_wait.py, test_bz2.py: the decorator is maybe useless here (import_module should be enough). Or do you suggest it for the readability? Lib/test/test_multiprocessing.py: I guess that some tests can be executed without thread (only testcases_threads require threads?). Lib/test/test_capi.py: because of import_module('threading'), TestPendingCalls will be skipped whereas TestPendingCalls.test_pendingcalls_non_threaded() works without threads. Lib/test/test_hashlib.py can simply use @reap_threads and import_module() in test_threaded_hashing() (instead of using a try/except at the beginning of the file). ... I'm too tired to re-read the whole patch. I think that you understood my ideads, and I don't have to detail changes on each file. Can you write a new version of the patch?
|msg100497 - (view)||Author: Jerry Seutter (jerry.seutter) *||Date: 2010-03-05 19:45|
Uploaded a new version of the patch, nothreads_3.patch. @r.david.murray - Good point about unittest.skipUnless, I didn't know about that function. I removed my decorator and used skipUnless() instead. @haypo: test_xmlrpc.py - Modified the URL changes so that now the tests should run exactly as before in the multithreaded case. test_macostools.py - The TESTFN changes are unrelated to the work I am doing. I removed them and will file as a separate item in Roundup. The skip_if_no decorator has been removed and replaced with unittest.skipUnless(). test_errno.py - You mentioned that only half the tests use threads. The file has two test methods, both of which use threads. test_sqlite.py - Fixed so that only tests that require threading are skipped. Thank you for the reminder about class decorators. test_urllib2_localnet.py - I will create an issue in the bug tracker to fix the usage of BaseTestCase. From the svn revision lot it appears the functionality in BaseTestCase is there for a reason and I think it should be fixed. test_support.py: - The thread module is now imported once at the top of test_support. - The reap_threads() function decorates with a thread cleanup function if thread is available, or with a no-op function if thread is not available. Added a docstring. - Removed "import thread" calls in threading_setup() and threading_cleanup(). Replaced with "if thread" statements. fork_wait.py - I removed the decorator and use import_module() instead. test_bz2.py - Modified the import so all but one of the tests run when threading is disabled. test_multiprocessing.py - Modified to use the try: import threading method of importing the module. It's kind of a moot point because the multiprocessing module does not exist when python is built without threading. test_capi.py - test_pendingcalls_non_threaded eventually makes a call to testcapi._pending_threadfunc(). testcapi._pending_threadfunc() does not exist when python is built without threading. What I'm trying to say is that both tests rely on threading. I added the skipUnless decorator to both test methods. test_hashlib.py - Switched to use skipUnless instead.
|msg100499 - (view)||Author: Jerry Seutter (jerry.seutter) *||Date: 2010-03-05 20:08|
I think this issue is going to reoccur every time someone adds a unit test that relies on threading. What do you think about using a buildbot slave to run the tests with a non-multithreaded build of python?
|msg100500 - (view)||Author: R. David Murray (r.david.murray) *||Date: 2010-03-05 20:10|
I think it is a good idea, but someone has to set one up.
|msg100502 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-03-05 21:58|
+1 to setup a buildbot, but only after all bugs are fixed :-)
|msg100514 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-03-06 03:32|
I'm applying fixes one by one in my local git-svn repository. I will commit them (in one huge patch \o/) when I'm done, or maybe post a new version of the patch (if I'm too scared of the patch).
|msg100516 - (view)||Author: Jerry Seutter (jerry.seutter) *||Date: 2010-03-06 04:29|
I think the latest (v3) patch is in pretty good shape. You and David have been through the changes and the result is a much improved set of changes. If you want (and you're okay with it), I can commit the changes from here and take the beating if it makes the buildbots fall over. :)
|msg100517 - (view)||Author: Jerry Seutter (jerry.seutter) *||Date: 2010-03-06 04:35|
I'll break up the patch into multiple files as well. It will make it easier to deal with if I cause a regression.
|msg100527 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-03-06 11:41|
> I think the latest (v3) patch is in pretty good shape. (...) > I'll break up the patch into multiple files as well. > It will make it easier to deal with if I cause a regression. I'm already doing that (apply the patch file by file). The patch v3 is not perfect, some tests are still disabled whereas they can be executed without tests. Give me a few days to work on a new version of the patch.
|msg100610 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-03-07 21:10|
Ok, here is my patch "version 4", based on jerry's nothreads_3.patch. Changes between version 3 and 4: - test_support: threading_setup() returns (1,) instead of None; reap_threads() simply returns the function instead of a dummy decorator - regrtest.py: display an error if -j/--multiprocess option is used - ctypes/test/test_errno.py, test_smtplib, test_socket, test_asynchat: skip some tests, not the whole file - test_logging, test_xmlrpc, test_file2k: move decorator from setUp() method to the class - test_capi: test threading variable value instead of the presence of the _test_thread_state function - test_capi, test_contextlib: move the decorator from methods to the class - test_multiprocessing: import threading (without try/except) after _multiprocessing because _multiprocessing depends on threading and so it will fail All tests pass on both builds: with and without threads. I'm not sure that my changes moving decorators from the setUp() method or other methods to the class is the right thing to do. FYI tests skipped because of the missing thread module are listed in "xx skips unexped on linux2: ...". I don't think that it's a problem. We discuss about that on IRC and bitdancer suggested to leave regrtest.py unchanged: "bitdancer> You should *definitely* not disable the skip message just because the threading module is missing, that would defeat the whole purpose of the message."
|msg100621 - (view)||Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) *||Date: 2010-03-08 00:40|
Did someone notice that on Windows, subprocess imports threading, and use threads for the communicate() method?
|msg100625 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-03-08 01:10|
> Did someone notice that on Windows, subprocess imports threading, > and use threads for the communicate() method? No, I didn't ran the tests on Windows. I can expect that someone build Python on Linux without threads, but is Python used in embedded systems with Windows but without threads? Since the patch is already huge, I would suggest to open a new issue (fix tests for Windows without threads).
|msg104386 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-04-28 00:15|
I commited the patch into small parts: 1 (r80552): fix test_support.py for Python compiled without thread 2 (r80553): regrtest.py -j option requires thread support 3 (r80554): test_doctest: import trace module in test_coverage() 4 (r80555): skip test_multiprocessing if thread support is disabled 5 (r80556, r80569): split Test.test_open() of ctypes/test/test_errno.py 6 (r80564): fix test_hashlib for missing threading module 7 (r80565): simplify threading detection in test_capi 8 (r80566): don't skip the whole test_asynchat if threading is missing 9 (r80568): fix test_xmlrpclib for missing threading module 10 (r80570): test_cmd imports trace module using test_support.import_module() 11 (r80571): fix many tests if thread support is disabled I created the patch to Jerry Seutter in the Misc/NEWS file. I'm waiting for the buildbot tests to port the commits to py3k. First commit of Part 5 (r80556) was wrong: I fixed it with a new commit (r80569).
|msg104472 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-04-28 22:39|
|msg105637 - (view)||Author: Stefan Krah (skrah) *||Date: 2010-05-13 15:36|
Victor, I think one more skip is required in test_socketserver.
|msg106551 - (view)||Author: STINNER Victor (haypo) *||Date: 2010-05-26 17:35|
|2010-05-26 17:35:25||haypo||set||status: open -> closed|
messages: + msg106551
|2010-05-13 15:36:45||skrah||set||status: closed -> open|
files: + nothreads-socketserver-shutdown.patch
nosy: + skrah
messages: + msg105637
resolution: fixed -> (no value)
|2010-04-28 22:39:41||haypo||set||status: pending -> closed|
messages: + msg104472
|2010-04-28 00:15:03||haypo||set||status: open -> pending|
messages: + msg104386
|2010-03-08 01:10:13||haypo||set||messages: + msg100625|
messages: + msg100621
messages: + msg100610
|2010-03-06 11:41:20||haypo||set||messages: + msg100527|
|2010-03-06 04:35:38||jerry.seutter||set||messages: + msg100517|
|2010-03-06 04:29:01||jerry.seutter||set||messages: + msg100516|
|2010-03-06 03:32:09||haypo||set||messages: + msg100514|
|2010-03-05 21:58:30||haypo||set||messages: + msg100502|
|2010-03-05 20:10:39||r.david.murray||set||messages: + msg100500|
|2010-03-05 20:08:16||jerry.seutter||set||messages: + msg100499|
messages: + msg100497
|2010-03-03 02:14:01||haypo||set||messages: + msg100324|
versions: - Python 3.1
messages: + msg100322
messages: + msg100176
|2010-02-26 19:54:36||jerry.seutter||set||keywords: + needs review|
|2009-12-11 06:54:56||jerry.seutter||set||stage: needs patch -> patch review|
keywords: + patch
messages: + msg96233
|2009-12-07 01:41:03||jerry.seutter||set||assignee: jerry.seutter|
nosy: + jerry.seutter