load_tests not invoked in package/__init__.py

[rbtcollins]

BPO	16662
Nosy	@warsaw, @vstinner, @rbtcollins, @bitdancer, @voidspace, @cjerdonek, @zware, @ElOpio, @asottile
PRs	bpo-30813: Fix unittest when hunting refleaks #2502 [3.6] bpo-30813: Fix unittest when hunting refleaks (#2502) #2505 [3.5] bpo-30813: Fix unittest when hunting refleaks (#2502) #2506
Files	16662.diff 16662_with_doc.diff: Barry's patch + doc changes 16662_passing_tests.diff: patch 16662_passing_tests_full.diff: updated patch. fix-windows-tests.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/warsaw'
closed_at = <Date 2014-10-17.19:42:38.292>
created_at = <Date 2012-12-11.09:14:51.800>
labels = ['type-feature', 'library']
title = 'load_tests not invoked in package/__init__.py'
updated_at = <Date 2017-06-30.11:12:22.901>
user = 'https://github.com/rbtcollins'

bugs.python.org fields:

activity = <Date 2017-06-30.11:12:22.901>
actor = 'vstinner'
assignee = 'barry'
closed = True
closed_date = <Date 2014-10-17.19:42:38.292>
closer = 'rbcollins'
components = ['Library (Lib)']
creation = <Date 2012-12-11.09:14:51.800>
creator = 'rbcollins'
dependencies = []
files = ['31102', '31575', '36482', '36491', '36714']
hgrepos = ['269']
issue_num = 16662
keywords = ['patch']
message_count = 37.0
messages = ['177327', '177340', '177346', '177350', '191024', '194019', '194022', '194032', '196853', '197203', '200271', '200272', '200274', '225940', '225943', '226008', '226585', '226589', '226590', '226596', '226600', '226601', '226604', '226611', '226613', '226747', '226802', '227331', '227496', '227497', '227865', '229594', '264204', '264253', '297373', '297383', '297387']
nosy_count = 11.0
nosy_names = ['barry', 'vstinner', 'rbcollins', 'vila', 'r.david.murray', 'michael.foord', 'chris.jerdonek', 'python-dev', 'zach.ware', 'elopio', 'Anthony Sottile']
pr_nums = ['2502', '2505', '2506']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue16662'
versions = ['Python 3.5']

[rbtcollins]

In loader.py:
if fnmatch(path, pattern):
# only check load_tests if the package directory itself matches the filter
name = self._get_name_from_path(full_path)
package = self._get_module_from_name(name)
load_tests = getattr(package, 'load_tests', None)
tests = self.loadTestsFromModule(package, use_load_tests=False)

But pattern is test*.py by default, and packages will never match that.

Its not at all clear why this is special cased at all, but if it is, we'll need a separate pattern. (Its not special cased in the bzrlib implementation that acted as the initial implementation).

[bitdancer]

test_email is a package, and has a load_tests in its __init__.py that I had to add in order to make "python -m unittest test.test_email" work. So I'm not sure what bug you are reporting here.

[cjerdonek]

I think he's saying that a test package will never be discovered by default, because the default value of discover()'s pattern argument is test*.py:

http://hg.python.org/cpython/file/bc322469a7a8/Lib/unittest/loader.py#l152

So I think he wants package directories with names of the form test* to match by default. The discover() docstring touches on this special case:

If a test package name (directory with '__init__.py') matches the
pattern then the package will be checked for a 'load_tests' function. If
this exists then it will be called with loader, tests, pattern.

[rbtcollins]

I have a package with tests in it. If the tests match test*.py, they are loaded, and load_tests within each one called. But load_tests on the package isn't called.

If I change the pattern supplied by the user to match the package, then the tests within adjacent packages that don't have a load_tests hook but have files called test*.py will no longer match, but the package will match.

My preference would be for the special case to just be removed, and load_tests called if it exists: its existence is enough of an opt-in.

Failing that, having two distinct fn patterns, one for packages and one for filenames (note the difference: one operates in the python namespace, one the filesystem namespace), would suffice.

[voidspace]

I agree that load_tests *should* be used in packages and that not doing this from the start was a mistake.

[warsaw]

Hah, I just ran into this too. I was perplexed why my load_tests() function wasn't being called and ended up pdb'ing unittest's discover, and found exactly this problem. I'm not surprised lifeless beat me to it.

(My use case was to piggyback on load_tests() to implement a package fixture, similar to what nose provides.)

Note that in http://docs.python.org/3/library/unittest.html#load-tests-protocol the docs even give you a recipe for a "no-op" load_tests() which would have been perfect, except for this problem with pattern matching the directory.

My preference would be to remove the pattern match on the path. I agree that the presence of a load_tests() is probably enough of an opt-in. The question is whether we could classify this change as a bug fix or new feature. I'd love to see this fixed in 3.3 so I'm hoping for the former.

[warsaw]

Seems like this patch does the trick for my very limited testing.

[voidspace]

I'm happy with the check being removed.

The old behaviour was documented I believe - so there'd need to be documentation changes to go with it. As the old behaviour is so "un-useful" I don't think there are backward compatibility issues with changing it.

[zware]

I took a stab at the doc changes, attached here and including Barry's patch.

[voidspace]

I get a couple of test failures with this patch applied:

======================================================================
ERROR: test_find_tests (unittest.test.test_discovery.TestDiscovery)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/compile/cpython/Lib/unittest/test/test_discovery.py", line 72, in test_find_tests
    suite = list(loader._find_tests(top_level, 'test*.py'))
  File "/compile/cpython/Lib/unittest/loader.py", line 298, in _find_tests
    tests = self.loadTestsFromModule(package, use_load_tests=False)
TypeError: <lambda>() got an unexpected keyword argument 'use_load_tests'

======================================================================
FAIL: test_find_tests_with_package (unittest.test.test_discovery.TestDiscovery)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/compile/cpython/Lib/unittest/test/test_discovery.py", line 137, in test_find_tests_with_package
    ['load_tests', 'test_directory2' + ' module tests'])
AssertionError: Lists differ: ['a_directory module tests', '... != ['load_tests', 'test_directory...

First differing element 0:
a_directory module tests
load_tests

First list contains 1 additional elements.
First extra element 2:
test_directory2 module tests

• ['a_directory module tests', 'load_tests', 'test_directory2 module tests']
  ? ----------------------------

+ ['load_tests', 'test_directory2 module tests']

----------------------------------------------------------------------

[warsaw]

The failure in test_discovery.py is odd. It's failing because loadTestsFromModule() is being passed a keyword arguemnt use_load_tests=False.

On the surface, the failure makes sense because if you look in test_discover.py, it's defining a lambda for loader.loadTestsFromModule that takes only one argument, the module name.

But the deeper question is why self.loadTestsFromModule() is being passed use_load_tests=False? loadTestsFromModule() is documented to take *only* the module name:

http://docs.python.org/3/library/unittest.html#unittest.TestLoader.loadTestsFromModule

But then if you look at loader.py, loadTestsFromModule does indeed take an undocumented use_load_tests keyword argument.

So it seems like there's two problems here. use_load_tests is undocumented, and the lambda in test_discovery.py should take that keyword argument. Michael, can you weigh in on this?

[warsaw]

On the second failure, the expected output just needs to be updated. Is that the right thing to do?