homepage

Some tests in Python testsuite are silently skipped if requirements is not satisfied. The proposed patch adds explicit "skipUnless()" and "raise SkipTest()" so that these tests now reported as skipped.
I.e. the code like
 if not condition:
 def test_foo(self):
 ...
is replaced by
 @unittest.skipUnless(condition, "requires foo")
 def test_foo(self):
 ...
and the code like
 def test_foo(self):
 ...
 if not condition:
 return
 ...
is replaced by
 def test_foo(self):
 ...
 if not condition:
 raise unittest.SkipTest("requires foo")
 ...

The patch applies cleanly on my 3.4 Win 7, fresh debug build. Somewhat fortuitously, as it turns out, I have not downloaded the files needed for ssl support. For each modified file, I ran
python_d -m test -v test_xxx
test test_nntplib crashed -- Traceback (most recent call last):
 File "F:\Python\dev\py34\lib\test\regrtest.py", line 1298, in runtest_inner
 the_module = importlib.import_module(abstest)
 File "F:\Python\dev\py34\lib\importlib\__init__.py", line 93, in import_module
 return _bootstrap._gcd_import(name[level:], package, level)
 File "<frozen importlib._bootstrap>", line 1613, in _gcd_import
 File "<frozen importlib._bootstrap>", line 1594, in _find_and_load
 File "<frozen importlib._bootstrap>", line 1561, in _find_and_load_unlocked
 File "<frozen importlib._bootstrap>", line 607, in _check_name_wrapper
 File "<frozen importlib._bootstrap>", line 1056, in load_module
 File "<frozen importlib._bootstrap>", line 926, in load_module
 File "<frozen importlib._bootstrap>", line 274, in _call_with_frames_removed
 File "F:\Python\dev\py34\lib\test\test_nntplib.py", line 305, in <module>
 class NetworkedNNTP_SSLTests(NetworkedNNTPTests):
 File "F:\Python\dev\py34\lib\test\test_nntplib.py", line 315, in NetworkedNNTP_SSLTests
 NNTP_CLASS = nntplib.NNTP_SSL
AttributeError: 'module' object has no attribute 'NNTP_SSL'
I do not understand the frozen importlib stuff, but nntplib._have_ssl is False, so the line asking for the non-existent attribute should not be executed.
The problem is that changing a guard from 'if hasattr' to the decorator is a *semantic change* in that it allows execution of the guarded statement. Skips only skip function calling. For a function statement, creating a function object from the compiled code object and binding a name is trivial. Not calling the function is the important part. For a class with code other than function definitions, the difference is crucial, as the above shows.
The general solution is to put class code inside a function so it is only executed later, as part of the test process, rather than during inport.
 @classmethod
 def setUpClass(cls):
 cls.NNTP_CLASS = nntplib.NNTP_SSL
In this case, you could instead guard it with 'if _have_ssl', but since this assignment is the only thing being tested by this test case, it should be inside a method anyway.
I suggest that you review your patch for other changed classes that might have class-level code that could fail with the change to the skip decorator.
Here is similar problem: trying to subclass a class that does not exist.
test test_socketserver crashed
 File "F:\Python\dev\py34\lib\test\test_socketserver.py", line 50, in <module>
 socketserver.UnixStreamServer):
AttributeError: 'module' object has no attribute 'UnixStreamServer'
class ForkingUnixStreamServer(socketserver.ForkingMixIn,
 socketserver.UnixStreamServer): pass
I think you have to go back to 'if HAVE_UNIX_SOCKETS:' for this. Or re-factor somehow.
---
I cannot help wondering whether test_math.xxx.test_exceptions should still be (normally) disabled. I sent Mark Dickinson a separate note.

Thank you Ezio and Terry for review. Here is updated patch. Problems with class initialization solved. I have made a lot of other changes especially in test_os.py and test_posix.py.

Applies and runs (with text_posix entirely skipped).

I read the patch. It looks good.
Anyway, I have two questions:
1. I saw 3 variants of writing description for skipping test descriptor:
requires os.mkfifo
test needs socket.inet_pton()
needs os.read
Which one is the preferred way to go? "requires" or "test needs" or "needs"? Or it does not matter?
2. I saw 2 variants of writing the function name required for running the test.
test needs socket.inet_pton()
test needs socket.inet_pton
Which one is the preferred way to go? With or without parentheses? Or it does not matter?

> Which one is the preferred way to go? "requires" or "test needs" or "needs"? Or it does not matter?
I used the wording which used in other skips in the same file or in similar skips in other files. If it matters I will correct messages.
> Which one is the preferred way to go? With or without parentheses? Or it does not matter?
Thank you for note. I will add missed parentheses to function names.
There are existing skip messages which uses function names without parentheses. I left them untouched (the patch already is a large enough).

I'm not sure I understand you Rietveld comment right Terry.
We can get rid of _have_ssl (this is implementation detail and shouldn't be required) and just try import ssl.
try:
 import ssl
except ImportError:
 ssl = None
If ssl is not None but nntplib.NNTP_SSL doesn't exist the NetworkedNNTP_SSLTests tests will failed:
======================================================================
ERROR: setUpClass (test.test_nntplib.NetworkedNNTP_SSLTests)
----------------------------------------------------------------------
Traceback (most recent call last):
 File "/home/serhiy/py/cpython/Lib/test/test_nntplib.py", line 298, in setUpClass
 cls.server = cls.NNTP_CLASS(cls.NNTP_HOST, timeout=TIMEOUT, usenetrc=False)
TypeError: 'NoneType' object is not callable
----------------------------------------------------------------------
This doesn't different from a case when some exception is raised in NNTP_SSL constructor.
We can add a separate test in MiscTests:
 @unittest.skipUnless(ssl, 'requires SSL support')
 def test_ssl_support(self):
 self.assertTrue(hasattr(nntplib, 'NNTP_SSL'))

You changed "NNTP_CLASS = nntplib.NNTP_SSL", which could potentially fail, to "NNTP_CLASS = getattr(nntplib, 'NNTP_SSL', None)", which cannot fail. Since that was the only thing that previously could fail, the change leaves nothing that can fail, so the test is not a test.
I suggested that you either not add the third param, a default, or that you remove the null test completely. If particular, if the only chunk of code in nntplib that is currently being tested is being executed by some other test, then do the latter.

> You changed "NNTP_CLASS = nntplib.NNTP_SSL", which could potentially fail, to "NNTP_CLASS = getattr(nntplib, 'NNTP_SSL', None)", which cannot fail. Since that was the only thing that previously could fail, the change leaves nothing that can fail, so the test is not a test.
This statement is not a part of a test. It runs at module import time. As you said in msg194907 the failing here will prohibit the running of all tests (even not SSL related).
> I suggested that you either not add the third param, a default,
getattr(nntplib, 'NNTP_SSL') is same as nntplib.NNTP_SSL. And it will break all tests.
> or that you remove the null test completely.
The purpose of this issue is stopping silence skipping tests. If some tests skipped for some reasons it should be reported.

My original suggestion was to put the possibly failing assignment inside
 @classmethod
 def setUpClass:
 NNTP_CLASS = nntplib.NNTP_SSL
so that failure of the assignment would be be reported, but not affect import.

Actually you should call parent's setUpClass at the and of declared setUpClass. Therefore you need 4 nontrivial lines instead of 1. The test will fail in setUpClass() in any case because parent's setUpClass() uses NNTP_CLASS. There are no behavior difference between this variants, the variant with getattr just is shorter.

Patch is rebased to tip.

I took a quick look at the revised patch. Clearly, it's too big to review at a detailed level. I tried the approach of running two complete verbose regrtest runs (-m test -v -uall), before and after the patch, and diffing the results in a "smart" diff app. Again, this is extremely tedious: while some simple filtering could be done to mask some differences (like elapsed times and time stamps), the non-deterministic output of many tests remains (and demonstrates how difficult it currently is to automatically compare regrtest run results). I did a spot check of the results and didn't notice any glaring problems and *did* notice that some tests were now being reported as skipped, as expected. Overall, I think this is a useful change to the test suite so I would say apply it to default (for 3.4.0) but I'm ambivalent about applying it to 3.3 at this point.

New changeset 1feeeb8992f8 by Serhiy Storchaka in branch '3.3':
Issue #18702: All skipped tests now reported as skipped.
http://hg.python.org/cpython/rev/1feeeb8992f8
New changeset 09105051b9f4 by Serhiy Storchaka in branch 'default':
Issue #18702: All skipped tests now reported as skipped.
http://hg.python.org/cpython/rev/09105051b9f4 

New changeset 2d330f7908e7 by Serhiy Storchaka in branch '2.7':
Issue #18702: All skipped tests now reported as skipped.
http://hg.python.org/cpython/rev/2d330f7908e7 

New changeset 0d8f0526813f by Serhiy Storchaka in branch '2.7':
Fix test_os (issue #18702).
http://hg.python.org/cpython/rev/0d8f0526813f 

New changeset a699550bc73b by Ned Deily in branch 'default':
Issue #18702 null merge
http://hg.python.org/cpython/rev/a699550bc73b 

I forgot to merge branches? Thanks Ned.

Related issues: issue19492 and issue19493.