classification
Title: A number tests "crash" if python is compiled --without-threads
Type: behavior Stage: patch review
Components: Tests Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: jerry.seutter Nosy List: amaury.forgeotdarc, jerry.seutter, r.david.murray, skrah, vstinner
Priority: low Keywords: easy, needs review, patch

Created on 2009-12-06 17:35 by r.david.murray, last changed 2010-05-26 17:35 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
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 vstinner, 2010-03-07 21:10
nothreads-socketserver-shutdown.patch skrah, 2010-05-13 15:36
Messages (21)
msg96035 - (view) Author: R. David Murray (r.david.murray) * (Python committer) 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) * (Python committer) 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 (vstinner) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 (vstinner) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 (vstinner) * (Python committer) Date: 2010-03-05 21:58
+1 to setup a buildbot, but only after all bugs are fixed :-)
msg100514 - (view) Author: STINNER Victor (vstinner) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 (vstinner) * (Python committer) 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 (vstinner) * (Python committer) 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) * (Python committer) 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 (vstinner) * (Python committer) 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 (vstinner) * (Python committer) 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 (vstinner) * (Python committer) Date: 2010-04-28 22:39
Ported to py3k (r80600), blocked in 2.6 (r80602) and 3.1 (r80601).
msg105637 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2010-05-13 15:36
Victor, I think one more skip is required in test_socketserver.
msg106551 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2010-05-26 17:35
> I think one more skip is required in test_socketserver.

Fixed: 2.7 (r81543), 3.2 (r81545), 3.1 (r81546). Blocked in 2.6 (r81544).
History
Date User Action Args
2010-05-26 17:35:25vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg106551
2010-05-13 15:36:45skrahsetstatus: closed -> open
files: + nothreads-socketserver-shutdown.patch

nosy: + skrah
messages: + msg105637

resolution: fixed -> (no value)
2010-04-28 22:39:41vstinnersetstatus: pending -> closed

messages: + msg104472
2010-04-28 00:15:03vstinnersetstatus: open -> pending
resolution: fixed
messages: + msg104386
2010-03-08 01:10:13vstinnersetmessages: + msg100625
2010-03-08 00:40:48amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg100621
2010-03-07 21:10:34vstinnersetfiles: + nothreads_4.patch

messages: + msg100610
2010-03-06 11:41:20vstinnersetmessages: + msg100527
2010-03-06 04:35:38jerry.seuttersetmessages: + msg100517
2010-03-06 04:29:01jerry.seuttersetmessages: + msg100516
2010-03-06 03:32:09vstinnersetmessages: + msg100514
2010-03-05 21:58:30vstinnersetmessages: + msg100502
2010-03-05 20:10:39r.david.murraysetmessages: + msg100500
2010-03-05 20:08:16jerry.seuttersetmessages: + msg100499
2010-03-05 19:45:47jerry.seuttersetfiles: + nothreads_3.patch

messages: + msg100497
2010-03-03 02:14:01vstinnersetmessages: + msg100324
2010-03-03 01:46:15r.david.murraysetmessages: + msg100323
versions: - Python 3.1
2010-03-03 01:07:51jerry.seuttersetfiles: + nothreads_2.patch

messages: + msg100322
2010-02-26 22:53:58vstinnersetnosy: + vstinner
messages: + msg100176
2010-02-26 19:54:36jerry.seuttersetkeywords: + needs review
2009-12-11 06:54:56jerry.seuttersetstage: needs patch -> patch review
2009-12-11 06:54:14jerry.seuttersetfiles: + nothreads.patch
keywords: + patch
messages: + msg96233
2009-12-07 01:41:03jerry.seuttersetassignee: jerry.seutter

nosy: + jerry.seutter
2009-12-06 17:35:30r.david.murraycreate