This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Author vstinner
Recipients jerry.seutter, r.david.murray, vstinner
Date 2010-03-03.02:13:59
SpamBayes Score 0.0
Marked as misclassified No
Message-id <201003030314.14539.victor.stinner@haypocalc.com>
In-reply-to <1267578473.25.0.907692851685.issue7449@psf.upfronthosting.co.za>
Content
> 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?
History
Date User Action Args
2010-03-03 02:14:03vstinnersetrecipients: + vstinner, jerry.seutter, r.david.murray
2010-03-03 02:14:01vstinnerlinkissue7449 messages
2010-03-03 02:13:59vstinnercreate