From 5a5b399f527de32d8bcb2efa677cf394bebbc711 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Sun, 19 May 2019 15:51:44 -0500 Subject: [PATCH 1/3] fix & tests --- src/twisted/newsfragments/9628.bugfix | 1 + src/twisted/trial/runner.py | 17 +++++++ src/twisted/trial/test/packages.py | 2 + src/twisted/trial/test/test_loader.py | 68 +++++++++++++++++++++------ 4 files changed, 74 insertions(+), 14 deletions(-) create mode 100644 src/twisted/newsfragments/9628.bugfix diff --git a/src/twisted/newsfragments/9628.bugfix b/src/twisted/newsfragments/9628.bugfix new file mode 100644 index 00000000000..92c1a94878d --- /dev/null +++ b/src/twisted/newsfragments/9628.bugfix @@ -0,0 +1 @@ +Trial on Python 3 will now properly re-raise ImportErrors that occur during the import of a module, rather than saying the module doesn't exist. diff --git a/src/twisted/trial/runner.py b/src/twisted/trial/runner.py index 92b70a24ff9..c2beddf9f9a 100644 --- a/src/twisted/trial/runner.py +++ b/src/twisted/trial/runner.py @@ -724,6 +724,23 @@ def findByName(self, _name, recurse=False): break except ImportError: + # Check to see where the ImportError happened. If it happened in + # this file, ignore it. + tb = sys.exc_info()[2] + + # Walk down to the deepest frame, where it actually happened. + while tb.tb_next is not None: + tb = tb.tb_next + + # Get the filename that the ImportError originated in. + filenameWhereHappened = tb.tb_frame.f_code.co_filename + + # If it originated in the reflect file, then it's because it + # doesn't exist. If it originates elsewhere, it's because an + # ImportError happened in a module that does exist. + if filenameWhereHappened != reflect.__file__: + raise + if remaining == "": raise reflect.ModuleNotFound("The module {} does not exist.".format(name)) diff --git a/src/twisted/trial/test/packages.py b/src/twisted/trial/test/packages.py index c6eafed60dc..0302bb0d0cd 100644 --- a/src/twisted/trial/test/packages.py +++ b/src/twisted/trial/test/packages.py @@ -102,6 +102,8 @@ class PackageTest(unittest.SynchronousTestCase): files = [ ('badpackage/__init__.py', 'frotz\n'), ('badpackage/test_module.py', ''), + ('unimportablepackage/__init__.py', ''), + ('unimportablepackage/test_module.py', 'import notarealmoduleok\n'), ('package2/__init__.py', ''), ('package2/test_module.py', 'import frotz\n'), ('package/__init__.py', ''), diff --git a/src/twisted/trial/test/test_loader.py b/src/twisted/trial/test/test_loader.py index b0d52afd4a1..cb18e7c7576 100644 --- a/src/twisted/trial/test/test_loader.py +++ b/src/twisted/trial/test/test_loader.py @@ -21,6 +21,7 @@ from twisted.python.modules import getModule from twisted.python.compat import _PY3 +from twisted.python.reflect import ModuleNotFound @@ -35,65 +36,104 @@ def testNames(tests): -class FinderTests(packages.PackageTest): +class FinderPy2Tests(unittest.SynchronousTestCase): """ Tests for L{runner.TestLoader.findByName}. """ + if _PY3: + skip = "Not relevant on Python 3" + def setUp(self): - packages.PackageTest.setUp(self) self.loader = runner.TestLoader() - def tearDown(self): - packages.PackageTest.tearDown(self) def test_findPackage(self): sample1 = self.loader.findByName('twisted') import twisted as sample2 self.assertEqual(sample1, sample2) + def test_findModule(self): sample1 = self.loader.findByName('twisted.trial.test.sample') from twisted.trial.test import sample as sample2 self.assertEqual(sample1, sample2) + def test_findFile(self): path = util.sibpath(__file__, 'sample.py') sample1 = self.loader.findByName(path) from twisted.trial.test import sample as sample2 self.assertEqual(sample1, sample2) + def test_findObject(self): sample1 = self.loader.findByName('twisted.trial.test.sample.FooTest') from twisted.trial.test import sample self.assertEqual(sample.FooTest, sample1) - if _PY3: - # In Python 3, `findByName` returns full TestCases, not the objects - # inside them. This because on Python 3, unbound methods don't exist, - # so you can't simply make a TestCase after finding it -- it's easier - # to just find it and put it in a TestCase immediately. - _Py3SkipMsg = ("Not relevant on Python 3") - test_findPackage.skip = _Py3SkipMsg - test_findModule.skip = _Py3SkipMsg - test_findFile.skip = _Py3SkipMsg - test_findObject.skip = _Py3SkipMsg def test_findNonModule(self): self.assertRaises(AttributeError, self.loader.findByName, 'twisted.trial.test.nonexistent') + def test_findNonPackage(self): self.assertRaises(ValueError, self.loader.findByName, 'nonextant') + def test_findNonFile(self): path = util.sibpath(__file__, 'nonexistent.py') self.assertRaises(ValueError, self.loader.findByName, path) +class FinderPy3Tests(packages.SysPathManglingTest): + + if not _PY3: + skip = "Not relevant on Python 2" + + def setUp(self): + super(FinderPy3Tests, self).setUp() + self.loader = runner.TestLoader() + + + def test_findNonModule(self): + self.assertRaises( + ModuleNotFound, self.loader.findByName, + 'twisted.trial.test.nonexistent' + ) + + + def test_findNonPackage(self): + self.assertRaises( + ModuleNotFound, self.loader.findByName, 'nonextant' + ) + + + def test_findNonFile(self): + """ + findByName, given a file path that doesn't exist, will raise a + ValueError saying that it is not a Python file. + """ + path = util.sibpath(__file__, 'nonexistent.py') + self.assertRaises(ValueError, self.loader.findByName, path) + + + def test_findFileWithImportError(self): + """ + findByName will re-raise ImportErrors inside modules that it has found + and imported. + """ + self.assertRaises( + ImportError, self.loader.findByName, + "unimportablepackage.test_module" + ) + + + class FileTests(packages.SysPathManglingTest): """ Tests for L{runner.filenameToModule}. From f8cce08e9ea4faf329475f6876d3f58130cde264 Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Sun, 19 May 2019 16:34:07 -0500 Subject: [PATCH 2/3] test fix --- src/twisted/trial/test/test_loader.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/twisted/trial/test/test_loader.py b/src/twisted/trial/test/test_loader.py index cb18e7c7576..0fcea6265b6 100644 --- a/src/twisted/trial/test/test_loader.py +++ b/src/twisted/trial/test/test_loader.py @@ -101,8 +101,13 @@ def setUp(self): def test_findNonModule(self): + """ + findByName, if given something findable up until the last entry, will + raise AttributeError (as it cannot tell if 'nonexistent' here is + supposed to be a module or a class). + """ self.assertRaises( - ModuleNotFound, self.loader.findByName, + AttributeError, self.loader.findByName, 'twisted.trial.test.nonexistent' ) From 13aa60c760a59a5bde61fd3c3ee4f029fdb3567d Mon Sep 17 00:00:00 2001 From: Amber Brown Date: Sun, 19 May 2019 17:30:11 -0500 Subject: [PATCH 3/3] cleanup --- src/twisted/trial/runner.py | 8 +++++--- src/twisted/trial/test/test_loader.py | 14 ++++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/twisted/trial/runner.py b/src/twisted/trial/runner.py index c2beddf9f9a..b1fd81ad2dd 100644 --- a/src/twisted/trial/runner.py +++ b/src/twisted/trial/runner.py @@ -724,8 +724,8 @@ def findByName(self, _name, recurse=False): break except ImportError: - # Check to see where the ImportError happened. If it happened in - # this file, ignore it. + # Check to see where the ImportError happened. If it happened + # in this file, ignore it. tb = sys.exc_info()[2] # Walk down to the deepest frame, where it actually happened. @@ -742,7 +742,9 @@ def findByName(self, _name, recurse=False): raise if remaining == "": - raise reflect.ModuleNotFound("The module {} does not exist.".format(name)) + raise reflect.ModuleNotFound( + "The module {} does not exist.".format(name) + ) if obj is None: # If it's none here, we didn't get to import anything. diff --git a/src/twisted/trial/test/test_loader.py b/src/twisted/trial/test/test_loader.py index 0fcea6265b6..d11f495b441 100644 --- a/src/twisted/trial/test/test_loader.py +++ b/src/twisted/trial/test/test_loader.py @@ -73,15 +73,17 @@ def test_findObject(self): def test_findNonModule(self): - self.assertRaises(AttributeError, - self.loader.findByName, - 'twisted.trial.test.nonexistent') + self.assertRaises( + AttributeError, self.loader.findByName, + 'twisted.trial.test.nonexistent' + ) def test_findNonPackage(self): - self.assertRaises(ValueError, - self.loader.findByName, - 'nonextant') + self.assertRaises( + ValueError, self.loader.findByName, + 'nonextant' + ) def test_findNonFile(self):