pathlib: Fix module path used when using importlib with namespaces#12313
pathlib: Fix module path used when using importlib with namespaces#12313leameow wants to merge 1 commit intopytest-dev:mainfrom
Conversation
7b2632e to
7ef1897
Compare
|
On a local test: Without the fix: With the fix: |
|
Thanks @isra17 for the proof of concept. I do think at the very least we're going to want a regression test to capture expectation that was missed prior to your patch. Is that something that you'd be interested in developing? |
Sure, I can work on this next week 👍 |
|
I can confirm that this un-breaks assertion re-writing for Matplotlib with an editable install. |
|
@jaraco @isra17 I very much like this PR, because it would unblock us. You said that a regression test is needed: Could you give me some more detailed instructions how such a test could/should look like, and where it should be located? Perhaps I would manage to draft such a test. Though, I have to admit that I have never contributed to pytest before. |
|
I'd model a test after some of the existing tests for importlib imports. Instead of checking that assert rewrites work, which would be much more involved, figure out what is the condition where |
There was a problem hiding this comment.
Thanks @isra17 for the PR, seems I have missed this notification.
As @jaraco mentions, it is important to have one or more regression tests for us to merge this in. Unfortunately the entire importlib mechanism feels somewhat brittle to me, given the scarce documentation and wild configuration variations in the wild where things can break or misbehave.
Closes #12044
Will do other steps from the template, but before I would like a confirmation that this fix make sense.
It does fix the issue of assertion rewriting mixed with namespace package on in my environment.
If fixes both the cases where the tests lives in a package with
__init__.pyand with the configconsider_namespace_packages = true. I am not familiar at all with this part of the code (or any to be honest), so I'm unsure what are the consequence, but from my understanding thefrom_spec(module, path)path parameter should take the module parent path, not the source root.From the doc:
@nicoddemus since you seem the main maintainer of this part.
Thanks!