classification
Title: load_tests not invoked in package/__init__.py
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: barry Nosy List: Anthony Sottile, barry, chris.jerdonek, elopio, haypo, michael.foord, python-dev, r.david.murray, rbcollins, vila, zach.ware
Priority: normal Keywords: patch

Created on 2012-12-11 09:14 by rbcollins, last changed 2016-04-26 11:39 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
16662.diff barry, 2013-07-31 21:17 review
16662_with_doc.diff zach.ware, 2013-09-03 16:35 Barry's patch + doc changes
16662_passing_tests.diff rbcollins, 2014-08-27 04:25 patch
16662_passing_tests_full.diff rbcollins, 2014-08-28 00:36 updated patch. review
fix-windows-tests.diff rbcollins, 2014-09-25 01:01 review
Repositories containing patches
https://bitbucket.org/rbtcollins/cpython
Messages (34)
msg177327 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2012-12-11 09:14
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).
msg177340 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-12-11 14:41
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.
msg177346 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-12-11 18:59
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.
msg177350 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2012-12-11 20:32
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.
msg191024 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-06-12 12:55
I agree that load_tests *should* be used in packages and that not doing this from the start was a mistake.
msg194019 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-07-31 21:13
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.
msg194022 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-07-31 21:17
Seems like this patch does the trick for my very limited testing.
msg194032 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-08-01 07:14
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.
msg196853 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-09-03 16:35
I took a stab at the doc changes, attached here and including Barry's patch.
msg197203 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-09-07 23:54
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']

----------------------------------------------------------------------
msg200271 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-10-18 15:16
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?
msg200272 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-10-18 15:18
On the second failure, the expected output just needs to be updated.  Is that the right thing to do?
msg200274 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2013-10-18 15:31
use_load_tests was deliberately undocumented. IIRC it only exists to allow us to load tests from a package module (__init__.py) without invoking load_tests - it maybe that it can just go away altogether now. 

I'll need to look at the code to confirm.
msg225940 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-08-27 01:52
I think we rather need a test that using a load_tests hook to recursively load and transform a subdir works. Hacking on that now.
msg225943 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-08-27 04:25
Ok, implementation I'm happy with is up in https://bitbucket.org/rbtcollins/cpython/commits/bbf2eb26dda8f3538893bf3dc33154089f37f99d
msg226008 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-08-28 00:36
The doc part of the patch was assuming this would be in 3.4 which it wasn't. Updated to 3.5. Also found a corner case - when packages were imported the _get_module_from_name method was not guarded for un-importable modules. This is strictly a separate issue, but since we'll now import considerably more modules, it seemed prudent to fix it at the same time.
msg226585 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-09-08 15:33
One thing I really do not like about Rob's last patch is that it exacerbates the documentation discrepancy for loadTestsFromModule().  As previously mentioned, use_load_tests arg was already not documented, and now the patch adds another undocumented pattern default arg.  Undocumented "unofficial" APIs are really a fib - we treat them as official APIs for backward compatibility reasons anyway, so I think it behooves us to document them.

In the same vein, the load_tests Protocol really should tell the truth about its third argument - i.e. it will not always be None.

As Michael suggests in http://bugs.python.org/issue16662#msg200274 I think we should just remove use_load_tests.  We'll still need to document the new pattern=None, unless there's a better way to handle that.
msg226589 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-09-08 16:57
So, I think what I'm going to do is change the sig of the method to:

    def loadTestsFromModule(self, module, *args, pattern=None, **kws):

I.e. the new `pattern` arg will be keyword-only. *args and **kws will be parsed for use_load_tests usage and a deprecation warning will be issued if found, but the argument will be ignored.  load_tests() will always be called if it's found.
msg226590 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-09-08 16:58
pattern will have to be documented and accepted as official API
msg226596 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-08 18:24
New changeset d0ff527c53da by Barry Warsaw in branch 'default':
- Issue #16662: load_tests() is now unconditionally run when it is present in
http://hg.python.org/cpython/rev/d0ff527c53da
msg226600 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-08 19:32
Thanks for landing this barry, there's a couple quirks with your improvements - loadTestsFromModule(mod, foo, bar) will raise a TypeError but not warn about foo the way loadTestsFromModule(mod, foo) will.

Secondly, the TypeError has an off-by-one error in its output:

loadTestsFromModule(mod, foo, bar) will claim 2 arguments were passed. Three were.


diff -r d0ff527c53da Lib/unittest/loader.py
--- a/Lib/unittest/loader.py	Mon Sep 08 14:21:37 2014 -0400
+++ b/Lib/unittest/loader.py	Tue Sep 09 07:32:05 2014 +1200
@@ -79,12 +79,12 @@
         # use_load_tests argument.  For backward compatibility, we still
         # accept the argument (which can also be the first position) but we
         # ignore it and issue a deprecation warning if it's present.
-        if len(args) == 1 or 'use_load_tests' in kws:
+        if len(args) or 'use_load_tests' in kws:
             warnings.warn('use_load_tests is deprecated and ignored',
                           DeprecationWarning)
             kws.pop('use_load_tests', None)
         if len(args) > 1:
-            raise TypeError('loadTestsFromModule() takes 1 positional argument but {} were given'.format(len(args)))
+            raise TypeError('loadTestsFromModule() takes 1 positional argument but {} were given'.format(len(args) + 1))
         if len(kws) != 0:
             # Since the keyword arguments are unsorted (see PEP 468), just
             # pick the alphabetically sorted first argument to complain about,
diff -r d0ff527c53da Lib/unittest/test/test_loader.py
--- a/Lib/unittest/test/test_loader.py	Mon Sep 08 14:21:37 2014 -0400
+++ b/Lib/unittest/test/test_loader.py	Tue Sep 09 07:32:05 2014 +1200
@@ -272,7 +272,7 @@
         # however use_load_tests (which sorts first) is ignored.
         self.assertEqual(
             str(cm.exception),
-            'loadTestsFromModule() takes 1 positional argument but 2 were given')
+            'loadTestsFromModule() takes 1 positional argument but 3 were given')
 
     @warningregistry
     def test_loadTestsFromModule__use_load_tests_other_bad_keyword(self):
msg226601 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-08 19:36
OH! One more thing I just spotted, which is that this change causes non-'discover' unittest test loading to invoke load_tests.

IMO this is the Right Thing - its what I intended when I described the protocol a few years back, but we should document it, no?
msg226604 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2014-09-08 19:43
I agree, load_tests should be honoured even when not invoked through discovery. If that wasn't the case it was an unfortunate oversight on my part!
msg226611 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-08 20:55
@michael - ah I think I inverted the sense of the old parameter. It was defaulting True. So - no need to document anything extra:)
msg226613 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-08 21:29
New changeset 92b292d68104 by Barry Warsaw in branch 'default':
A few tweaks for issue16662 based on feedback from Robert Collins.
http://hg.python.org/cpython/rev/92b292d68104
msg226747 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-09-11 07:23
The changeset d0ff527c53da5b925b61a8a70afc686ca6e05960 related to this issue introduced a regression in test_unittest. The test now fails on Windows. Example:

http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5065/steps/test/logs/stdio


======================================================================
ERROR: test_discover_with_init_module_that_raises_SkipTest_on_import (unittest.test.test_discovery.TestDiscovery)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\unittest\test\test_discovery.py", line 451, in test_discover_with_init_module_that_raises_SkipTest_on_import
    suite = loader.discover('/foo')
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\unittest\loader.py", line 284, in discover
    tests = list(self._find_tests(start_dir, pattern))
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\unittest\loader.py", line 319, in _find_tests
    paths = sorted(os.listdir(start_dir))
  File "C:\buildbot.python.org\3.x.kloth-win64\build\lib\unittest\test\test_discovery.py", line 388, in list_dir
    return list(vfs[path])
KeyError: 'C:\\foo'
msg226802 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2014-09-11 21:21
On Sep 11, 2014, at 07:23 AM, STINNER Victor wrote:

>The changeset d0ff527c53da5b925b61a8a70afc686ca6e05960 related to this issue
>introduced a regression in test_unittest. The test now fails on
>Windows.

Darn.  I don't have Windows handy to work out a fix.  I'll try to get a VM
running but wouldn't mind if someone beats me to it. :)
msg227331 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-23 09:11
I've managed to get a windows setup working. Its my mini-vfs which needs to be Windows aware (because the abs path of /foo is C:\\foo). I'll work up a patch tomorrowish.
msg227496 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-25 01:00
Fix up the tests patch - tested on windows 7.
msg227497 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-09-25 01:01
bah, wrong extension to trigger review code :)
msg227865 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-30 01:54
New changeset 090dc85f4226 by Benjamin Peterson in branch 'default':
fix windows tests (#16662)
https://hg.python.org/cpython/rev/090dc85f4226
msg229594 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-10-17 19:42
Closing as the fix to the test suite is applied.
msg264204 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2016-04-26 01:53
I have a hunch that this fix here may be causing this: https://github.com/spotify/dh-virtualenv/issues/148

Minimally:

echo 'from setuptools import setup; setup(name="demo")' > setup.py
echo 'import pytest' > tests/__init__.py

$ python setup.py test
running test
running egg_info
writing dependency_links to demo.egg-info/dependency_links.txt
writing demo.egg-info/PKG-INFO
writing top-level names to demo.egg-info/top_level.txt
reading manifest file 'demo.egg-info/SOURCES.txt'
writing manifest file 'demo.egg-info/SOURCES.txt'
running build_ext

----------------------------------------------------------------------
Ran 0 tests in 0.000s

OK


$ python3.5 setup.py test
running test
running egg_info
writing demo.egg-info/PKG-INFO
writing dependency_links to demo.egg-info/dependency_links.txt
writing top-level names to demo.egg-info/top_level.txt
reading manifest file 'demo.egg-info/SOURCES.txt'
writing manifest file 'demo.egg-info/SOURCES.txt'
running build_ext
tests (unittest.loader._FailedTest) ... ERROR

======================================================================
ERROR: tests (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests
Traceback (most recent call last):
  File "/usr/lib/python3.5/unittest/loader.py", line 462, in _find_test_path
    package = self._get_module_from_name(name)
  File "/usr/lib/python3.5/unittest/loader.py", line 369, in _get_module_from_name
    __import__(name)
  File "/tmp/foo/tests/__init__.py", line 1, in <module>
    import pytest
ImportError: No module named 'pytest'


----------------------------------------------------------------------
Ran 1 test in 0.000s

FAILED (errors=1)
msg264253 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-04-26 11:39
> ImportError: No module named 'pytest'

It looks like you must install pytest dependency. If you consider that it's really a bug in Python, please open an issue: this issue is now closed.
History
Date User Action Args
2016-04-26 11:39:16hayposetmessages: + msg264253
2016-04-26 01:53:34Anthony Sottilesetnosy: + Anthony Sottile
messages: + msg264204
2014-11-02 15:20:47ezio.melottisetstage: resolved
2014-10-17 19:42:38rbcollinssetstatus: open -> closed
resolution: fixed
messages: + msg229594
2014-09-30 01:54:57python-devsetmessages: + msg227865
2014-09-25 01:01:20rbcollinssetfiles: - fix-windows-tests.patch
2014-09-25 01:01:04rbcollinssetfiles: + fix-windows-tests.diff

messages: + msg227497
2014-09-25 01:00:18rbcollinssetfiles: + fix-windows-tests.patch

messages: + msg227496
2014-09-23 09:11:59rbcollinssetmessages: + msg227331
2014-09-11 21:21:36barrysetmessages: + msg226802
2014-09-11 07:23:36hayposetstatus: closed -> open

nosy: + haypo
messages: + msg226747

resolution: fixed -> (no value)
2014-09-08 21:29:10python-devsetmessages: + msg226613
2014-09-08 20:55:34rbcollinssetmessages: + msg226611
2014-09-08 19:43:05michael.foordsetmessages: + msg226604
2014-09-08 19:36:10rbcollinssetmessages: + msg226601
2014-09-08 19:32:23rbcollinssetmessages: + msg226600
2014-09-08 18:41:12barrysetstatus: open -> closed
resolution: fixed
2014-09-08 18:24:26python-devsetnosy: + python-dev
messages: + msg226596
2014-09-08 16:58:09barrysetmessages: + msg226590
2014-09-08 16:57:40barrysetmessages: + msg226589
2014-09-08 15:33:11barrysetmessages: + msg226585
2014-09-08 14:46:24barrysetversions: - Python 3.4
2014-09-08 14:36:05barrysetassignee: michael.foord -> barry
2014-09-08 14:16:57barrysetversions: + Python 3.5
2014-08-28 00:37:00rbcollinssetfiles: + 16662_passing_tests_full.diff

messages: + msg226008
2014-08-27 04:25:29rbcollinssetfiles: + 16662_passing_tests.diff
hgrepos: + hgrepo269
messages: + msg225943
2014-08-27 01:52:48rbcollinssetmessages: + msg225940
2013-10-18 15:31:32michael.foordsetmessages: + msg200274
2013-10-18 15:18:10barrysetmessages: + msg200272
2013-10-18 15:16:12barrysetmessages: + msg200271
2013-09-07 23:54:57michael.foordsetmessages: + msg197203
2013-09-03 16:35:01zach.waresetfiles: + 16662_with_doc.diff
versions: + Python 3.4
messages: + msg196853

components: + Library (Lib)
type: enhancement
2013-08-01 07:14:25michael.foordsetmessages: + msg194032
2013-07-31 21:17:06barrysetfiles: + 16662.diff
keywords: + patch
messages: + msg194022
2013-07-31 21:13:34barrysetnosy: + barry
messages: + msg194019
2013-06-19 17:29:12zach.waresetnosy: + zach.ware
2013-06-12 12:55:30michael.foordsetmessages: + msg191024
2013-06-12 12:54:56michael.foordsetassignee: michael.foord

nosy: + michael.foord
2013-05-28 15:43:58elopiosetnosy: + elopio
2013-04-26 14:48:50vilasetnosy: + vila
2012-12-11 20:32:36rbcollinssetmessages: + msg177350
2012-12-11 18:59:09chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg177346
2012-12-11 14:41:52r.david.murraysetnosy: + r.david.murray
messages: + msg177340
2012-12-11 09:14:51rbcollinscreate