Test discovery in unittest will only attempt to import modules that are importable; i.e. their names are valid Python identifiers. If an import fails during discovery this will be recorded as an error and test discovery will continue. Issue 6568.

4 files changed, 85 insertions, 46 deletions

diff --git a/Doc/library/unittest.rst b/Doc/library/unittest.rst
index 61c4308..f9e1725 100644
--- a/Doc/library/unittest.rst
+++ b/Doc/library/unittest.rst
@@ -1257,12 +1257,17 @@ Loading and running tests
 
       Find and return all test modules from the specified start directory,
       recursing into subdirectories to find them. Only test files that match
-      *pattern* will be loaded. (Using shell style pattern matching.)
+      *pattern* will be loaded. (Using shell style pattern matching.) Only
+      module names that are importable (i.e. are valid Python identifiers) will
+      be loaded.
 
       All test modules must be importable from the top level of the project. If
       the start directory is not the top level directory then the top level
       directory must be specified separately.
 
+      If importing a module fails, for example due to a syntax error, then this
+      will be recorded as a single error and discovery will continue.
+
       If a test package name (directory with :file:`__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*,
diff --git a/Lib/test/test_unittest.py b/Lib/test/test_unittest.py
index 79ee982..459334b 100644
--- a/Lib/test/test_unittest.py
+++ b/Lib/test/test_unittest.py
@@ -3469,31 +3469,19 @@ class Test_TextTestRunner(TestCase):
 class TestDiscovery(TestCase):
 
     # Heavily mocked tests so I can avoid hitting the filesystem
-    def test_get_module_from_path(self):
+    def test_get_name_from_path(self):
         loader = unittest.TestLoader()
 
-        old_import = __import__
-        def restore_import():
-            __builtin__.__import__ = old_import
-        __builtin__.__import__ = lambda *_: None
-        self.addCleanup(restore_import)
-
-        expected_module = object()
-        def del_module():
-            del sys.modules['bar.baz']
-        sys.modules['bar.baz'] = expected_module
-        self.addCleanup(del_module)
-
         loader._top_level_dir = '/foo'
-        module = loader._get_module_from_path('/foo/bar/baz.py')
-        self.assertEqual(module, expected_module)
+        name = loader._get_name_from_path('/foo/bar/baz.py')
+        self.assertEqual(name, 'bar.baz')
 
         if not __debug__:
             # asserts are off
             return
 
         with self.assertRaises(AssertionError):
-            loader._get_module_from_path('/bar/baz.py')
+            loader._get_name_from_path('/bar/baz.py')
 
     def test_find_tests(self):
         loader = unittest.TestLoader()
@@ -3509,7 +3497,7 @@ class TestDiscovery(TestCase):
             os.path.isdir = original_isdir
 
         path_lists = [['test1.py', 'test2.py', 'not_a_test.py', 'test_dir',
-                       'test.foo', 'another_dir'],
+                       'test.foo', 'test-not-a-module.py', 'another_dir'],
                       ['test3.py', 'test4.py', ]]
         os.listdir = lambda path: path_lists.pop(0)
         self.addCleanup(restore_listdir)
@@ -3525,16 +3513,16 @@ class TestDiscovery(TestCase):
         os.path.isfile = isfile
         self.addCleanup(restore_isfile)
 
-        loader._get_module_from_path = lambda path: path + ' module'
+        loader._get_module_from_name = lambda path: path + ' module'
         loader.loadTestsFromModule = lambda module: module + ' tests'
 
         loader._top_level_dir = '/foo'
         suite = list(loader._find_tests('/foo', 'test*.py'))
 
-        expected = [os.path.join('/foo', name) + ' module tests' for name in
-                    ('test1.py', 'test2.py')]
-        expected.extend([os.path.join('/foo', 'test_dir', name) + ' module tests' for name in
-                    ('test3.py', 'test4.py')])
+        expected = [name + ' module tests' for name in
+                    ('test1', 'test2')]
+        expected.extend([('test_dir.%s' % name) + ' module tests' for name in
+                    ('test3', 'test4')])
         self.assertEqual(suite, expected)
 
     def test_find_tests_with_package(self):
@@ -3577,7 +3565,7 @@ class TestDiscovery(TestCase):
             def __eq__(self, other):
                 return self.path == other.path
 
-        loader._get_module_from_path = lambda path: Module(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')
@@ -3592,15 +3580,12 @@ class TestDiscovery(TestCase):
         # We should have loaded tests from the test_directory package by calling load_tests
         # and directly from the test_directory2 package
         self.assertEqual(suite,
-                         ['load_tests',
-                          os.path.join('/foo', 'test_directory2') + ' module tests'])
-        self.assertEqual(Module.paths, [os.path.join('/foo', 'test_directory'),
-                                        os.path.join('/foo', 'test_directory2')])
+                         ['load_tests', 'test_directory2' + ' module tests'])
+        self.assertEqual(Module.paths, ['test_directory', 'test_directory2'])
 
         # load_tests should have been called once with loader, tests and pattern
         self.assertEqual(Module.load_tests_args,
-                         [(loader, os.path.join('/foo', 'test_directory') + ' module tests',
-                           'test*')])
+                         [(loader, 'test_directory' + ' module tests', 'test*')])
 
     def test_discover(self):
         loader = unittest.TestLoader()
@@ -3640,6 +3625,25 @@ class TestDiscovery(TestCase):
         self.assertEqual(loader._top_level_dir, top_level_dir)
         self.assertEqual(_find_tests_args, [(start_dir, 'pattern')])
 
+    def test_discover_with_modules_that_fail_to_import(self):
+        loader = unittest.TestLoader()
+
+        listdir = os.listdir
+        os.listdir = lambda _: ['test_this_does_not_exist.py']
+        isfile = os.path.isfile
+        os.path.isfile = lambda _: True
+        def restore():
+            os.path.isfile = isfile
+            os.listdir = listdir
+        self.addCleanup(restore)
+
+        suite = loader.discover('.')
+        self.assertEqual(suite.countTestCases(), 1)
+        test = list(list(suite)[0])[0] # extract test from suite
+
+        with self.assertRaises(ImportError):
+            test.test_this_does_not_exist()
+
     def test_command_line_handling_parseArgs(self):
         # Haha - take that uninstantiable class
         program = object.__new__(TestProgram)
diff --git a/Lib/unittest/loader.py b/Lib/unittest/loader.py
index 21520f5..31c343b 100644
--- a/Lib/unittest/loader.py
+++ b/Lib/unittest/loader.py
@@ -1,7 +1,9 @@
 """Loading unittests."""
 
 import os
+import re
 import sys
+import traceback
 import types
 
 from fnmatch import fnmatch
@@ -19,6 +21,26 @@ def _CmpToKey(mycmp):
     return K
 
 
+# what about .pyc or .pyo (etc)
+# we would need to avoid loading the same tests multiple times
+# from '.py', '.pyc' *and* '.pyo'
+VALID_MODULE_NAME = re.compile(r'[_a-z]\w*\.py$', re.IGNORECASE)
+
+
+def _make_failed_import_test(name, suiteClass):
+    message = 'Failed to import test module: %s' % name
+    if hasattr(traceback, 'format_exc'):
+        # Python 2.3 compatibility
+        # format_exc returns two frames of discover.py as well
+        message += '\n%s' % traceback.format_exc()
+
+    def testImportFailure(self):
+        raise ImportError(message)
+    attrs = {name: testImportFailure}
+    ModuleImportFailure = type('ModuleImportFailure', (case.TestCase,), attrs)
+    return suiteClass((ModuleImportFailure(name),))
+
+
 class TestLoader(object):
     """
     This class is responsible for loading tests according to various criteria
@@ -161,17 +183,17 @@ class TestLoader(object):
         tests = list(self._find_tests(start_dir, pattern))
         return self.suiteClass(tests)
 
-
-    def _get_module_from_path(self, path):
-        """Load a module from a path relative to the top-level directory
-        of a project. Used by discovery."""
+    def _get_name_from_path(self, path):
         path = os.path.splitext(os.path.normpath(path))[0]
 
-        relpath = os.path.relpath(path, self._top_level_dir)
-        assert not os.path.isabs(relpath), "Path must be within the project"
-        assert not relpath.startswith('..'), "Path must be within the project"
+        _relpath = os.path.relpath(path, self._top_level_dir)
+        assert not os.path.isabs(_relpath), "Path must be within the project"
+        assert not _relpath.startswith('..'), "Path must be within the project"
+
+        name = _relpath.replace(os.path.sep, '.')
+        return name
 
-        name = relpath.replace(os.path.sep, '.')
+    def _get_module_from_name(self, name):
         __import__(name)
         return sys.modules[name]
 
@@ -181,14 +203,20 @@ class TestLoader(object):
 
         for path in paths:
             full_path = os.path.join(start_dir, path)
-            # what about __init__.pyc or pyo (etc)
-            # we would need to avoid loading the same tests multiple times
-            # from '.py', '.pyc' *and* '.pyo'
-            if os.path.isfile(full_path) and path.lower().endswith('.py'):
+            if os.path.isfile(full_path):
+                if not VALID_MODULE_NAME.match(path):
+                    # valid Python identifiers only
+                    continue
+
                 if fnmatch(path, pattern):
                     # if the test file matches, load it
-                    module = self._get_module_from_path(full_path)
-                    yield self.loadTestsFromModule(module)
+                    name = self._get_name_from_path(full_path)
+                    try:
+                        module = self._get_module_from_name(name)
+                    except:
+                        yield _make_failed_import_test(name, self.suiteClass)
+                    else:
+                        yield self.loadTestsFromModule(module)
             elif os.path.isdir(full_path):
                 if not os.path.isfile(os.path.join(full_path, '__init__.py')):
                     continue
@@ -197,7 +225,8 @@ class TestLoader(object):
                 tests = None
                 if fnmatch(path, pattern):
                     # only check load_tests if the package directory itself matches the filter
-                    package = self._get_module_from_path(full_path)
+                    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)
 
diff --git a/Lib/unittest/suite.py b/Lib/unittest/suite.py
index 5fba259..730b4d9 100644
--- a/Lib/unittest/suite.py
+++ b/Lib/unittest/suite.py
@@ -1,6 +1,7 @@
 """TestSuite"""
 
 from . import case
+from . import util
 
 
 class TestSuite(object):
@@ -17,7 +18,7 @@ class TestSuite(object):
         self.addTests(tests)
 
     def __repr__(self):
-        return "<%s tests=%s>" % (_strclass(self.__class__), list(self))
+        return "<%s tests=%s>" % (util.strclass(self.__class__), list(self))
 
     def __eq__(self, other):
         if not isinstance(other, self.__class__):