classification
Title: Report skipped tests as skipped
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ezio.melotti, haypo, michael.foord, ned.deily, pitrou, python-dev, serhiy.storchaka, terry.reedy, vajrasky, zach.ware
Priority: normal Keywords: patch

Created on 2013-08-10 11:09 by serhiy.storchaka, last changed 2013-11-04 13:03 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
skip_tests.patch serhiy.storchaka, 2013-08-10 11:09 review
skip_tests_2.patch serhiy.storchaka, 2013-08-11 21:42 review
skip_tests_3.patch serhiy.storchaka, 2013-10-20 12:15 review
Messages (19)
msg194790 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-10 11:09
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")
        ...
msg194907 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-08-11 18:22
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.
msg194912 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-11 21:42
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.
msg194913 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-08-11 22:34
Applies and runs (with text_posix entirely skipped).
msg194926 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-08-12 04:57
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?
msg194933 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-12 09:29
> 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).
msg195455 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-17 09:40
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'))
msg195459 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-08-17 12:27
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.
msg195512 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-17 19:44
> 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.
msg195515 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-08-17 20:10
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.
msg195517 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-17 20:32
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.
msg200571 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-10-20 12:15
Patch is rebased to tip.
msg200649 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2013-10-20 23:01
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.
msg202052 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-03 19:32
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
msg202060 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-03 21:16
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
msg202061 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-03 21:26
New changeset 0d8f0526813f by Serhiy Storchaka in branch '2.7':
Fix test_os (issue #18702).
http://hg.python.org/cpython/rev/0d8f0526813f
msg202067 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-03 23:35
New changeset a699550bc73b by Ned Deily in branch 'default':
Issue #18702 null merge
http://hg.python.org/cpython/rev/a699550bc73b
msg202112 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-04 10:26
I forgot to merge branches? Thanks Ned.
msg202126 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-04 13:03
Related issues: issue19492 and issue19493.
History
Date User Action Args
2013-11-04 13:03:33serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg202126

stage: patch review -> resolved
2013-11-04 10:26:16serhiy.storchakasetmessages: + msg202112
2013-11-03 23:35:47python-devsetmessages: + msg202067
2013-11-03 21:26:33python-devsetmessages: + msg202061
2013-11-03 21:16:12python-devsetmessages: + msg202060
2013-11-03 19:32:19python-devsetnosy: + python-dev
messages: + msg202052
2013-10-20 23:01:37ned.deilysetnosy: + ned.deily
messages: + msg200649
2013-10-20 12:15:15serhiy.storchakasetfiles: + skip_tests_3.patch

messages: + msg200571
2013-08-28 19:35:53hayposetnosy: + haypo
2013-08-28 16:39:49zach.waresetnosy: + zach.ware
2013-08-17 20:32:36serhiy.storchakasetmessages: + msg195517
2013-08-17 20:10:12terry.reedysetmessages: + msg195515
2013-08-17 19:44:37serhiy.storchakasetmessages: + msg195512
2013-08-17 12:27:06terry.reedysetmessages: + msg195459
2013-08-17 09:40:25serhiy.storchakasetmessages: + msg195455
2013-08-12 09:29:21serhiy.storchakasetmessages: + msg194933
2013-08-12 04:57:17vajraskysetnosy: + vajrasky
messages: + msg194926
2013-08-11 22:34:04terry.reedysetmessages: + msg194913
2013-08-11 21:42:29serhiy.storchakasetfiles: + skip_tests_2.patch

messages: + msg194912
2013-08-11 18:22:05terry.reedysetnosy: + terry.reedy
messages: + msg194907
2013-08-10 11:09:26serhiy.storchakacreate