classification
Title: Silently skipped tests in test_importlib
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: brett.cannon, pitrou, python-dev, serhiy.storchaka, zach.ware
Priority: normal Keywords: patch

Created on 2013-11-14 21:12 by zach.ware, last changed 2013-11-19 03:49 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
skipped_importlib_tests.diff zach.ware, 2013-11-14 21:12 review
skipped_importlib_tests.v2.diff zach.ware, 2013-11-18 04:49 Set empty tests to None review
Messages (11)
msg202890 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-14 21:12
Several tests in test_importlib are skipped by way of the test method consisting of a comment and a 'pass' statement.  The attached patch makes the skips explicit, using the removed comment as the reason.  Ideally these skipped tests should be removed from being 'tested' at all, since most of them are impossible to test, but I'm not sure how easy that would be to do.
msg202903 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-14 21:59
I use `test_spam = None` to skip the test in a subclass and remove it from the report.

The question is which of these tests should be removed because they are senseless in specified subclass and which of them are just not implemented yet and should be reported in the report for reminder.
msg202955 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-11-15 14:57
These tests can't be removed because the classes are inheriting from an ABC to make sure that those test cases are considered and dealt with, either by explicitly testing them or ignoring them because they don't apply to the finder/loader.

And since they are being ignored because the case has been considered and "tested" by doing nothing I don't want them listed as skipped since they aren't skipped based on some conditional result but instead because they literally can't be tested or don't apply. IOW testing packages with test_package() for the builtin loader wasn't skipped, it was tested by doing nothing since packages are flat-out not supported.

I say close this as rejected. I appreciate the sentiment but in this instance I think the skip label for the test is incorrect.
msg202956 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-15 15:10
In this case we should do

    test_package = None # Built-in modules cannot be a package.
    test_module_in_package = None # Built-in modules cannobt be in a package.
    ...

And then tests will be skipped and not shown in test report.
msg202957 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-11-15 15:13
As long as setting them to None satisfies the ABC that's fine with me.
msg202968 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-11-15 18:18
Agreed with both Brett and Serhiy.
msg203246 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-18 04:49
How does this one strike you, Brett?  Setting the empty tests to None seems to keep the ABC happy and reduces the total number of tests from 632 to 598.
msg203306 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-11-18 16:07
I've actually started doing this in the PEP 451 repo and it's working out so far. When that all lands I should have all the loaders ported but not the finders. If you want, Zachary, you can make the chances in the PEP 451 repo so you don't have to wonder what has or has not been cleaned up.
msg203327 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-18 22:11
Alright, I'll try to do that later this evening.
msg203335 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-19 03:47
New changeset 1ac4f0645519 by Zachary Ware in branch '3.3':
Issue #19596: Set untestable tests in test_importlib to None
http://hg.python.org/cpython/rev/1ac4f0645519

New changeset 34a65109d191 by Zachary Ware in branch 'default':
Issue #19596: Null merge with 3.3
http://hg.python.org/cpython/rev/34a65109d191
msg203336 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-19 03:49
Committed in 3.3 and as 5d38989191bb in features/pep-451.
History
Date User Action Args
2013-11-19 03:49:22zach.waresetstatus: open -> closed
resolution: fixed
messages: + msg203336

stage: patch review -> resolved
2013-11-19 03:47:53python-devsetnosy: + python-dev
messages: + msg203335
2013-11-18 22:11:43zach.waresetmessages: + msg203327
2013-11-18 16:07:58brett.cannonsetassignee: zach.ware
messages: + msg203306
2013-11-18 04:49:49zach.waresetfiles: + skipped_importlib_tests.v2.diff

messages: + msg203246
2013-11-15 18:18:08pitrousetnosy: + pitrou
messages: + msg202968
2013-11-15 15:13:22brett.cannonsetmessages: + msg202957
2013-11-15 15:10:12serhiy.storchakasetmessages: + msg202956
2013-11-15 14:57:18brett.cannonsetmessages: + msg202955
2013-11-14 21:59:38serhiy.storchakasetmessages: + msg202903
2013-11-14 21:12:22zach.warecreate