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.

3 files changed, 79 insertions, 45 deletions

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__):