STINNER Victor <victor.stin...@haypocalc.com> added the comment:
> 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?
----------
_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue7449>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
