- Issue #16662: load_tests() is now unconditionally run when it is present in

  a package's __init__.py.  TestLoader.loadTestsFromModule() still accepts
  use_load_tests, but it is deprecated and ignored.  A new keyword-only
  attribute `pattern` is added and documented.  Patch given by Robert Collins,
  tweaked by Barry Warsaw.

3 files changed, 427 insertions, 33 deletions

diff --git a/Lib/unittest/loader.py b/Lib/unittest/loader.py
index 808c50e..590e227 100644
--- a/Lib/unittest/loader.py
+++ b/Lib/unittest/loader.py
@@ -6,6 +6,7 @@ import sys
 import traceback
 import types
 import functools
+import warnings
 
 from fnmatch import fnmatch
 
@@ -70,8 +71,27 @@ class TestLoader(object):
         loaded_suite = self.suiteClass(map(testCaseClass, testCaseNames))
         return loaded_suite
 
-    def loadTestsFromModule(self, module, use_load_tests=True):
+    # XXX After Python 3.5, remove backward compatibility hacks for
+    # use_load_tests deprecation via *args and **kws.  See issue 16662.
+    def loadTestsFromModule(self, module, *args, pattern=None, **kws):
         """Return a suite of all tests cases contained in the given module"""
+        # This method used to take an undocumented and unofficial
+        # 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:
+            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)))
+        if len(kws) != 0:
+            # Since the keyword arguments are unsorted (see PEP 468), just
+            # pick the alphabetically sorted first argument to complain about,
+            # if multiple were given.  At least the error message will be
+            # predictable.
+            complaint = sorted(kws)[0]
+            raise TypeError("loadTestsFromModule() got an unexpected keyword argument '{}'".format(complaint))
         tests = []
         for name in dir(module):
             obj = getattr(module, name)
@@ -80,9 +100,9 @@ class TestLoader(object):
 
         load_tests = getattr(module, 'load_tests', None)
         tests = self.suiteClass(tests)
-        if use_load_tests and load_tests is not None:
+        if load_tests is not None:
             try:
-                return load_tests(self, tests, None)
+                return load_tests(self, tests, pattern)
             except Exception as e:
                 return _make_failed_load_tests(module.__name__, e,
                                                self.suiteClass)
@@ -325,7 +345,7 @@ class TestLoader(object):
                         msg = ("%r module incorrectly imported from %r. Expected %r. "
                                "Is this module globally installed?")
                         raise ImportError(msg % (mod_name, module_dir, expected_dir))
-                    yield self.loadTestsFromModule(module)
+                    yield self.loadTestsFromModule(module, pattern=pattern)
             elif os.path.isdir(full_path):
                 if (not namespace and
                     not os.path.isfile(os.path.join(full_path, '__init__.py'))):
@@ -333,26 +353,27 @@ class TestLoader(object):
 
                 load_tests = None
                 tests = None
-                if fnmatch(path, pattern):
-                    # only check load_tests if the package directory itself matches the filter
-                    name = self._get_name_from_path(full_path)
+                name = self._get_name_from_path(full_path)
+                try:
                     package = self._get_module_from_name(name)
+                except case.SkipTest as e:
+                    yield _make_skipped_test(name, e, self.suiteClass)
+                except:
+                    yield _make_failed_import_test(name, self.suiteClass)
+                else:
                     load_tests = getattr(package, 'load_tests', None)
-                    tests = self.loadTestsFromModule(package, use_load_tests=False)
-
-                if load_tests is None:
+                    tests = self.loadTestsFromModule(package, pattern=pattern)
                     if tests is not None:
                         # tests loaded from package file
                         yield tests
+
+                    if load_tests is not None:
+                        # loadTestsFromModule(package) has load_tests for us.
+                        continue
                     # recurse into the package
                     yield from self._find_tests(full_path, pattern,
                                                 namespace=namespace)
-                else:
-                    try:
-                        yield load_tests(self, tests, pattern)
-                    except Exception as e:
-                        yield _make_failed_load_tests(package.__name__, e,
-                                                      self.suiteClass)
+
 
 defaultTestLoader = TestLoader()
 
diff --git a/Lib/unittest/test/test_discovery.py b/Lib/unittest/test/test_discovery.py
index abb9066..851f9f7 100644
--- a/Lib/unittest/test/test_discovery.py
+++ b/Lib/unittest/test/test_discovery.py
@@ -68,7 +68,13 @@ class TestDiscovery(unittest.TestCase):
         self.addCleanup(restore_isfile)
 
         loader._get_module_from_name = lambda path: path + ' module'
-        loader.loadTestsFromModule = lambda module: module + ' tests'
+        orig_load_tests = loader.loadTestsFromModule
+        def loadTestsFromModule(module, pattern=None):
+            # This is where load_tests is called.
+            base = orig_load_tests(module, pattern=pattern)
+            return base + [module + ' tests']
+        loader.loadTestsFromModule = loadTestsFromModule
+        loader.suiteClass = lambda thing: thing
 
         top_level = os.path.abspath('/foo')
         loader._top_level_dir = top_level
@@ -76,9 +82,9 @@ class TestDiscovery(unittest.TestCase):
 
         # The test suites found should be sorted alphabetically for reliable
         # execution order.
-        expected = [name + ' module tests' for name in
-                    ('test1', 'test2')]
-        expected.extend([('test_dir.%s' % name) + ' module tests' for name in
+        expected = [[name + ' module tests'] for name in
+                    ('test1', 'test2', 'test_dir')]
+        expected.extend([[('test_dir.%s' % name) + ' module tests'] for name in
                     ('test3', 'test4')])
         self.assertEqual(suite, expected)
 
@@ -116,34 +122,204 @@ class TestDiscovery(unittest.TestCase):
                 if os.path.basename(path) == 'test_directory':
                     def load_tests(loader, tests, pattern):
                         self.load_tests_args.append((loader, tests, pattern))
-                        return 'load_tests'
+                        return [self.path + ' load_tests']
                     self.load_tests = load_tests
 
             def __eq__(self, other):
                 return self.path == other.path
 
         loader._get_module_from_name = lambda name: Module(name)
-        def loadTestsFromModule(module, use_load_tests):
-            if use_load_tests:
-                raise self.failureException('use_load_tests should be False for packages')
-            return module.path + ' module tests'
+        orig_load_tests = loader.loadTestsFromModule
+        def loadTestsFromModule(module, pattern=None):
+            # This is where load_tests is called.
+            base = orig_load_tests(module, pattern=pattern)
+            return base + [module.path + ' module tests']
         loader.loadTestsFromModule = loadTestsFromModule
+        loader.suiteClass = lambda thing: thing
 
         loader._top_level_dir = '/foo'
         # this time no '.py' on the pattern so that it can match
         # a test package
         suite = list(loader._find_tests('/foo', 'test*'))
 
-        # We should have loaded tests from the test_directory package by calling load_tests
-        # and directly from the test_directory2 package
+        # We should have loaded tests from the a_directory and test_directory2
+        # directly and via load_tests for the test_directory package, which
+        # still calls the baseline module loader.
         self.assertEqual(suite,
-                         ['load_tests', 'test_directory2' + ' module tests'])
+                         [['a_directory module tests'],
+                          ['test_directory load_tests',
+                           'test_directory module tests'],
+                          ['test_directory2 module tests']])
+
+
         # The test module paths should be sorted for reliable execution order
-        self.assertEqual(Module.paths, ['test_directory', 'test_directory2'])
+        self.assertEqual(Module.paths,
+                         ['a_directory', 'test_directory', 'test_directory2'])
 
         # load_tests should have been called once with loader, tests and pattern
+        # (but there are no tests in our stub module itself, so thats [] at the
+        # time of call.
         self.assertEqual(Module.load_tests_args,
-                         [(loader, 'test_directory' + ' module tests', 'test*')])
+                         [(loader, [], 'test*')])
+
+    def test_find_tests_default_calls_package_load_tests(self):
+        loader = unittest.TestLoader()
+
+        original_listdir = os.listdir
+        def restore_listdir():
+            os.listdir = original_listdir
+        original_isfile = os.path.isfile
+        def restore_isfile():
+            os.path.isfile = original_isfile
+        original_isdir = os.path.isdir
+        def restore_isdir():
+            os.path.isdir = original_isdir
+
+        directories = ['a_directory', 'test_directory', 'test_directory2']
+        path_lists = [directories, [], [], []]
+        os.listdir = lambda path: path_lists.pop(0)
+        self.addCleanup(restore_listdir)
+
+        os.path.isdir = lambda path: True
+        self.addCleanup(restore_isdir)
+
+        os.path.isfile = lambda path: os.path.basename(path) not in directories
+        self.addCleanup(restore_isfile)
+
+        class Module(object):
+            paths = []
+            load_tests_args = []
+
+            def __init__(self, path):
+                self.path = path
+                self.paths.append(path)
+                if os.path.basename(path) == 'test_directory':
+                    def load_tests(loader, tests, pattern):
+                        self.load_tests_args.append((loader, tests, pattern))
+                        return [self.path + ' load_tests']
+                    self.load_tests = load_tests
+
+            def __eq__(self, other):
+                return self.path == other.path
+
+        loader._get_module_from_name = lambda name: Module(name)
+        orig_load_tests = loader.loadTestsFromModule
+        def loadTestsFromModule(module, pattern=None):
+            # This is where load_tests is called.
+            base = orig_load_tests(module, pattern=pattern)
+            return base + [module.path + ' module tests']
+        loader.loadTestsFromModule = loadTestsFromModule
+        loader.suiteClass = lambda thing: thing
+
+        loader._top_level_dir = '/foo'
+        # this time no '.py' on the pattern so that it can match
+        # a test package
+        suite = list(loader._find_tests('/foo', 'test*.py'))
+
+        # We should have loaded tests from the a_directory and test_directory2
+        # directly and via load_tests for the test_directory package, which
+        # still calls the baseline module loader.
+        self.assertEqual(suite,
+                         [['a_directory module tests'],
+                          ['test_directory load_tests',
+                           'test_directory module tests'],
+                          ['test_directory2 module tests']])
+        # The test module paths should be sorted for reliable execution order
+        self.assertEqual(Module.paths,
+                         ['a_directory', 'test_directory', 'test_directory2'])
+
+
+        # load_tests should have been called once with loader, tests and pattern
+        self.assertEqual(Module.load_tests_args,
+                         [(loader, [], 'test*.py')])
+
+    def test_find_tests_customise_via_package_pattern(self):
+        # This test uses the example 'do-nothing' load_tests from
+        # https://docs.python.org/3/library/unittest.html#load-tests-protocol
+        # to make sure that that actually works.
+        # Housekeeping
+        original_listdir = os.listdir
+        def restore_listdir():
+            os.listdir = original_listdir
+        self.addCleanup(restore_listdir)
+        original_isfile = os.path.isfile
+        def restore_isfile():
+            os.path.isfile = original_isfile
+        self.addCleanup(restore_isfile)
+        original_isdir = os.path.isdir
+        def restore_isdir():
+            os.path.isdir = original_isdir
+        self.addCleanup(restore_isdir)
+        self.addCleanup(sys.path.remove, '/foo')
+
+        # Test data: we expect the following:
+        # a listdir to find our package, and a isfile and isdir check on it.
+        # a module-from-name call to turn that into a module
+        # followed by load_tests.
+        # then our load_tests will call discover() which is messy
+        # but that finally chains into find_tests again for the child dir -
+        # which is why we don't have a infinite loop.
+        # We expect to see:
+        # the module load tests for both package and plain module called,
+        # and the plain module result nested by the package module load_tests
+        # indicating that it was processed and could have been mutated.
+        vfs = {'/foo': ['my_package'],
+               '/foo/my_package': ['__init__.py', 'test_module.py']}
+        def list_dir(path):
+            return list(vfs[path])
+        os.listdir = list_dir
+        os.path.isdir = lambda path: not path.endswith('.py')
+        os.path.isfile = lambda path: path.endswith('.py')
+
+        class Module(object):
+            paths = []
+            load_tests_args = []
+
+            def __init__(self, path):
+                self.path = path
+                self.paths.append(path)
+                if path.endswith('test_module'):
+                    def load_tests(loader, tests, pattern):
+                        self.load_tests_args.append((loader, tests, pattern))
+                        return [self.path + ' load_tests']
+                else:
+                    def load_tests(loader, tests, pattern):
+                        self.load_tests_args.append((loader, tests, pattern))
+                        # top level directory cached on loader instance
+                        __file__ = '/foo/my_package/__init__.py'
+                        this_dir = os.path.dirname(__file__)
+                        pkg_tests = loader.discover(
+                            start_dir=this_dir, pattern=pattern)
+                        return [self.path + ' load_tests', tests
+                            ] + pkg_tests
+                self.load_tests = load_tests
+
+            def __eq__(self, other):
+                return self.path == other.path
+
+        loader = unittest.TestLoader()
+        loader._get_module_from_name = lambda name: Module(name)
+        loader.suiteClass = lambda thing: thing
+
+        loader._top_level_dir = '/foo'
+        # this time no '.py' on the pattern so that it can match
+        # a test package
+        suite = list(loader._find_tests('/foo', 'test*.py'))
+
+        # We should have loaded tests from both my_package and
+        # my_pacakge.test_module, and also run the load_tests hook in both.
+        # (normally this would be nested TestSuites.)
+        self.assertEqual(suite,
+                         [['my_package load_tests', [],
+                          ['my_package.test_module load_tests']]])
+        # Parents before children.
+        self.assertEqual(Module.paths,
+                         ['my_package', 'my_package.test_module'])
+
+        # load_tests should have been called twice with loader, tests and pattern
+        self.assertEqual(Module.load_tests_args,
+                         [(loader, [], 'test*.py'),
+                          (loader, [], 'test*.py')])
 
     def test_discover(self):
         loader = unittest.TestLoader()
@@ -203,6 +379,17 @@ class TestDiscovery(unittest.TestCase):
             sys.path[:] = orig_sys_path
         self.addCleanup(restore)
 
+    def setup_import_issue_package_tests(self, vfs):
+        self.addCleanup(setattr, os, 'listdir', os.listdir)
+        self.addCleanup(setattr, os.path, 'isfile', os.path.isfile)
+        self.addCleanup(setattr, os.path, 'isdir', os.path.isdir)
+        self.addCleanup(sys.path.__setitem__, slice(None), list(sys.path))
+        def list_dir(path):
+            return list(vfs[path])
+        os.listdir = list_dir
+        os.path.isdir = lambda path: not path.endswith('.py')
+        os.path.isfile = lambda path: path.endswith('.py')
+
     def test_discover_with_modules_that_fail_to_import(self):
         loader = unittest.TestLoader()
 
@@ -216,6 +403,25 @@ class TestDiscovery(unittest.TestCase):
         with self.assertRaises(ImportError):
             test.test_this_does_not_exist()
 
+    def test_discover_with_init_modules_that_fail_to_import(self):
+        vfs = {'/foo': ['my_package'],
+               '/foo/my_package': ['__init__.py', 'test_module.py']}
+        self.setup_import_issue_package_tests(vfs)
+        import_calls = []
+        def _get_module_from_name(name):
+            import_calls.append(name)
+            raise ImportError("Cannot import Name")
+        loader = unittest.TestLoader()
+        loader._get_module_from_name = _get_module_from_name
+        suite = loader.discover('/foo')
+
+        self.assertIn('/foo', sys.path)
+        self.assertEqual(suite.countTestCases(), 1)
+        test = list(list(suite)[0])[0] # extract test from suite
+        with self.assertRaises(ImportError):
+            test.my_package()
+        self.assertEqual(import_calls, ['my_package'])
+
     def test_discover_with_module_that_raises_SkipTest_on_import(self):
         loader = unittest.TestLoader()
 
@@ -232,6 +438,26 @@ class TestDiscovery(unittest.TestCase):
         suite.run(result)
         self.assertEqual(len(result.skipped), 1)
 
+    def test_discover_with_init_module_that_raises_SkipTest_on_import(self):
+        vfs = {'/foo': ['my_package'],
+               '/foo/my_package': ['__init__.py', 'test_module.py']}
+        self.setup_import_issue_package_tests(vfs)
+        import_calls = []
+        def _get_module_from_name(name):
+            import_calls.append(name)
+            raise unittest.SkipTest('skipperoo')
+        loader = unittest.TestLoader()
+        loader._get_module_from_name = _get_module_from_name
+        suite = loader.discover('/foo')
+
+        self.assertIn('/foo', sys.path)
+        self.assertEqual(suite.countTestCases(), 1)
+        result = unittest.TestResult()
+        suite.run(result)
+        self.assertEqual(len(result.skipped), 1)
+        self.assertEqual(result.testsRun, 1)
+        self.assertEqual(import_calls, ['my_package'])
+
     def test_command_line_handling_parseArgs(self):
         program = TestableTestProgram()
 
diff --git a/Lib/unittest/test/test_loader.py b/Lib/unittest/test/test_loader.py
index 3e013af..d363394 100644
--- a/Lib/unittest/test/test_loader.py
+++ b/Lib/unittest/test/test_loader.py
@@ -1,9 +1,26 @@
 import sys
 import types
-
+import warnings
 
 import unittest
 
+# Decorator used in the deprecation tests to reset the warning registry for
+# test isolation and reproducibility.
+def warningregistry(func):
+    def wrapper(*args, **kws):
+        missing = object()
+        saved = getattr(warnings, '__warningregistry__', missing).copy()
+        try:
+            return func(*args, **kws)
+        finally:
+            if saved is missing:
+                try:
+                    del warnings.__warningregistry__
+                except AttributeError:
+                    pass
+            else:
+                warnings.__warningregistry__ = saved
+
 
 class Test_TestLoader(unittest.TestCase):
 
@@ -150,6 +167,7 @@ class Test_TestLoader(unittest.TestCase):
 
     # Check that loadTestsFromModule honors (or not) a module
     # with a load_tests function.
+    @warningregistry
     def test_loadTestsFromModule__load_tests(self):
         m = types.ModuleType('m')
         class MyTestCase(unittest.TestCase):
@@ -168,10 +186,139 @@ class Test_TestLoader(unittest.TestCase):
         suite = loader.loadTestsFromModule(m)
         self.assertIsInstance(suite, unittest.TestSuite)
         self.assertEqual(load_tests_args, [loader, suite, None])
+        # With Python 3.5, the undocumented and unofficial use_load_tests is
+        # ignored (and deprecated).
+        load_tests_args = []
+        with warnings.catch_warnings(record=False):
+            warnings.simplefilter('never')
+            suite = loader.loadTestsFromModule(m, use_load_tests=False)
+            self.assertEqual(load_tests_args, [loader, suite, None])
+
+    @warningregistry
+    def test_loadTestsFromModule__use_load_tests_deprecated_positional(self):
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            m = types.ModuleType('m')
+            class MyTestCase(unittest.TestCase):
+                def test(self):
+                    pass
+            m.testcase_1 = MyTestCase
+
+            load_tests_args = []
+            def load_tests(loader, tests, pattern):
+                self.assertIsInstance(tests, unittest.TestSuite)
+                load_tests_args.extend((loader, tests, pattern))
+                return tests
+            m.load_tests = load_tests
+            # The method still works.
+            loader = unittest.TestLoader()
+            # use_load_tests=True as a positional argument.
+            suite = loader.loadTestsFromModule(m, False)
+            self.assertIsInstance(suite, unittest.TestSuite)
+            # load_tests was still called because use_load_tests is deprecated
+            # and ignored.
+            self.assertEqual(load_tests_args, [loader, suite, None])
+        # We got a warning.
+        self.assertIs(w[-1].category, DeprecationWarning)
+        self.assertEqual(str(w[-1].message),
+                             'use_load_tests is deprecated and ignored')
+
+    @warningregistry
+    def test_loadTestsFromModule__use_load_tests_deprecated_keyword(self):
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter('always')
+            m = types.ModuleType('m')
+            class MyTestCase(unittest.TestCase):
+                def test(self):
+                    pass
+            m.testcase_1 = MyTestCase
+
+            load_tests_args = []
+            def load_tests(loader, tests, pattern):
+                self.assertIsInstance(tests, unittest.TestSuite)
+                load_tests_args.extend((loader, tests, pattern))
+                return tests
+            m.load_tests = load_tests
+            # The method still works.
+            loader = unittest.TestLoader()
+            suite = loader.loadTestsFromModule(m, use_load_tests=False)
+            self.assertIsInstance(suite, unittest.TestSuite)
+            # load_tests was still called because use_load_tests is deprecated
+            # and ignored.
+            self.assertEqual(load_tests_args, [loader, suite, None])
+            # We got a warning.
+            self.assertIs(w[-1].category, DeprecationWarning)
+            self.assertEqual(str(w[-1].message),
+                                 'use_load_tests is deprecated and ignored')
+
+    def test_loadTestsFromModule__too_many_positional_args(self):
+        m = types.ModuleType('m')
+        class MyTestCase(unittest.TestCase):
+            def test(self):
+                pass
+        m.testcase_1 = MyTestCase
+
+        load_tests_args = []
+        def load_tests(loader, tests, pattern):
+            self.assertIsInstance(tests, unittest.TestSuite)
+            load_tests_args.extend((loader, tests, pattern))
+            return tests
+        m.load_tests = load_tests
+        loader = unittest.TestLoader()
+        with self.assertRaises(TypeError) as cm:
+            loader.loadTestsFromModule(m, False, 'testme.*')
+        self.assertEqual(type(cm.exception), TypeError)
+        # The error message names the first bad argument alphabetically,
+        # however use_load_tests (which sorts first) is ignored.
+        self.assertEqual(
+            str(cm.exception),
+            'loadTestsFromModule() takes 1 positional argument but 2 were given')
+
+    @warningregistry
+    def test_loadTestsFromModule__use_load_tests_other_bad_keyword(self):
+        m = types.ModuleType('m')
+        class MyTestCase(unittest.TestCase):
+            def test(self):
+                pass
+        m.testcase_1 = MyTestCase
+
+        load_tests_args = []
+        def load_tests(loader, tests, pattern):
+            self.assertIsInstance(tests, unittest.TestSuite)
+            load_tests_args.extend((loader, tests, pattern))
+            return tests
+        m.load_tests = load_tests
+        loader = unittest.TestLoader()
+        with warnings.catch_warnings():
+            warnings.simplefilter('never')
+            with self.assertRaises(TypeError) as cm:
+                loader.loadTestsFromModule(
+                    m, use_load_tests=False, very_bad=True, worse=False)
+        self.assertEqual(type(cm.exception), TypeError)
+        # The error message names the first bad argument alphabetically,
+        # however use_load_tests (which sorts first) is ignored.
+        self.assertEqual(
+            str(cm.exception),
+            "loadTestsFromModule() got an unexpected keyword argument 'very_bad'")
+
+    def test_loadTestsFromModule__pattern(self):
+        m = types.ModuleType('m')
+        class MyTestCase(unittest.TestCase):
+            def test(self):
+                pass
+        m.testcase_1 = MyTestCase
 
         load_tests_args = []
-        suite = loader.loadTestsFromModule(m, use_load_tests=False)
-        self.assertEqual(load_tests_args, [])
+        def load_tests(loader, tests, pattern):
+            self.assertIsInstance(tests, unittest.TestSuite)
+            load_tests_args.extend((loader, tests, pattern))
+            return tests
+        m.load_tests = load_tests
+
+        loader = unittest.TestLoader()
+        suite = loader.loadTestsFromModule(m, pattern='testme.*')
+        self.assertIsInstance(suite, unittest.TestSuite)
+        self.assertEqual(load_tests_args, [loader, suite, 'testme.*'])
 
     def test_loadTestsFromModule__faulty_load_tests(self):
         m = types.ModuleType('m')