Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9628] Fix trial3 not knowing the difference between ImportErrors and modules not existing #1143

Merged
merged 7 commits into from Jul 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions 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.
21 changes: 20 additions & 1 deletion src/twisted/trial/runner.py
Expand Up @@ -724,8 +724,27 @@ 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 == "":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can never be true. remaining is assigned in two places: to None in line 710 and to remainingName (the class, method, function etc. part of the fqpn) on line 723. in the second case it will be a string, but: a) the break prevents this block from running after that assignment, and b) remaining can only be "" with an fqpn that ends in a dot, which is invalid.

the ModuleNotFound exception caught by the test comes from namedAny on line 752. this should be changed to if remaining is None: for it to have the intended effect.

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.
Expand Down
2 changes: 2 additions & 0 deletions src/twisted/trial/test/packages.py
Expand Up @@ -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', ''),
Expand Down
87 changes: 67 additions & 20 deletions src/twisted/trial/test/test_loader.py
Expand Up @@ -21,6 +21,7 @@

from twisted.python.modules import getModule
from twisted.python.compat import _PY3
from twisted.python.reflect import ModuleNotFound



Expand All @@ -35,65 +36,111 @@ 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')
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):
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):
"""
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(
AttributeError, 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}.
Expand Down