| msg202890 - (view) |
Author: Zachary Ware (zach.ware) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2013-11-15 18:18 |
| Agreed with both Brett and Serhiy. |
|
|
| msg203246 - (view) |
Author: Zachary Ware (zach.ware) * |
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) * |
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) * |
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) * |
Date: 2013-11-19 03:49 |
| Committed in 3.3 and as 5d38989191bb in features/pep-451. |
|
|