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

Conversation

hawkowl
Copy link
Member

@hawkowl hawkowl commented May 19, 2019

Copy link
Member

@markrwilliams markrwilliams left a comment

Choose a reason for hiding this comment

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

@hawkowl explained why Py3Loader has to exist. per this comment:

# Walk down the remaining modules. Hold on to the parent for
# methods, as on Python 3, you can no longer get the parent
# class from just holding onto the method.

Unbound methods are gone in Python 3, so the object returned by namedAny('twisted.trial.test.test_loader.FinderPy3Tests.test_findNonPackage') won't have a reference to its class. loadAnything needs that to filter down to subclasses of TestCase.

That means Py3TestLoader can't just used namedAny the way TestLoader does, but instead has to reimplement that logic here.

Now, the stack walking logic here will be slow on PyPy, but trial is already slow on PyPy; we also have other instances of stack walking (see _inlineCallbacks).

@hawkowl suggests reimplementing this with importlib.util.spec_from_file_location which seems good, but enough work that it should land in a separate PR. i'm approving and landing this because it fixes a bug ahead of a new release.

thanks @hawkowl !

# 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.

@markrwilliams markrwilliams merged commit ab6156f into trunk Jul 20, 2019
@markrwilliams markrwilliams deleted the 9628-trial3-importerror branch July 20, 2019 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants