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

Ensure that a module within a namespace package can be found by --pyargs #1597

Merged
merged 3 commits into from Jun 19, 2016

Conversation

Projects
None yet
4 participants
@taschini
Copy link
Contributor

taschini commented Jun 8, 2016

This PR supersedes #1568 and addresses the problem highlighted in #1567 and in part #478.

See #1568 for the description of the test setup.

Unlike #1568, this PR does not attempt to emulate the import machinery to determine the filename of a package or directory, delegating instead most of the work to the pkgutil module of the standard library.

NB In case of a non-top-level module or package, pkgutil.find_loader will import the parent package or packages.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 8, 2016

Coverage Status

Coverage increased (+0.04%) to 92.246% when pulling 2481b7a on taschini:pyargs-fix into 70fdab4 on pytest-dev:master.

return x
try:
path = loader.get_filename()
except:

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 8, 2016

Member

It is usually better to avoid bare except clauses like this... could this be replaced by except ImportError?

try:
path = loader.get_filename()
except:
path = loader.modules[x][0].co_filename

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 8, 2016

Member

In which situations is this necessary? A think a comment here would help.

I tried to play a bit with this to see what it supposed to do, but didn't obtain much success:

Python 3.5.0 (v3.5.0:374f501f4567, Sep 13 2015, 02:16:59) [MSC v.1900 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import pkgutil
>>> loader = pkgutil.find_loader('py')
>>> loader.modules['py']
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'SourceFileLoader' object has no attribute 'modules'
"*collected 0 items*"
])

# Depending on whether the process running the test is the

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 8, 2016

Member

You can force testdir to call pytest as a subprocess by calling testdir.runpytest_subprocess... seems better than try to accommodate both possibilities for testdir.runpytest.

cur = py.std.os.environ.get('PYTHONPATH')
if cur:
dirs += (cur,)
return ':'.join(str(p) for p in dirs)

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 8, 2016

Member

Instead of : I think this should be os.pathsep (which is ; on Windows)

taschini added a commit to taschini/pytest that referenced this pull request Jun 9, 2016

@taschini

This comment has been minimized.

Copy link
Contributor Author

taschini commented Jun 9, 2016

Appveyor caught a little problem with a test setup using PyPy on Windows: I'll fix that.

@nicoddemus I'll incorporate your feedback. The reason why that try-except clause is needed is that sometimes _parsearg() gets invoked with AssertionRewritingHook, which does not define a get_filename method, already in place. Hence, the exception you'll get is an AttributeError.

I'll catch explicitly that exception and add a comment to explain why that is necessary.

taschini added a commit to taschini/pytest that referenced this pull request Jun 13, 2016

Incorporated feedback (pytest-dev#1597).
Fixed problem caused in a test on Windows by file left open by PyPy and not immediately garbage collected.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 13, 2016

Coverage Status

Coverage increased (+0.005%) to 92.21% when pulling 5403f27 on taschini:pyargs-fix into 70fdab4 on pytest-dev:master.

@taschini

This comment has been minimized.

Copy link
Contributor Author

taschini commented Jun 13, 2016

I incorporated the feedback from @nicoddemus and also fixed a problem caused in a test on Windows by a file left open by PyPy and not immediately garbage collected.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 13, 2016

Thanks! 😁

LGTM. Could someone else take a look as well?

Also, we need a CHANGELOG entry and add @taschini to AUTHORS before merging.

@taschini

This comment has been minimized.

Copy link
Contributor Author

taschini commented Jun 13, 2016

Shall I add the entries in CHANGELOG and AUTHORS or is it something that typically you guys do?

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 13, 2016

Please do. We usually ask for PR submitters to do it, but this has gone back and forth a few times already and I didn't want to ask for one more, hehehe. 😅

taschini added a commit to taschini/pytest that referenced this pull request Jun 13, 2016

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 13, 2016

It seems this is now in conflict. Would you please merge/rebase to fix it? Thanks!

taschini added some commits Jun 8, 2016

Incorporated feedback (#1597).
Fixed problem caused in a test on Windows by file left open by PyPy and not immediately garbage collected.

@taschini taschini force-pushed the taschini:pyargs-fix branch from 351d3c2 to 1218392 Jun 14, 2016

@taschini

This comment has been minimized.

Copy link
Contributor Author

taschini commented Jun 14, 2016

Rebased the PR.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 14, 2016

Coverage Status

Coverage decreased (-0.004%) to 92.21% when pulling 1218392 on taschini:pyargs-fix into 66e66f6 on pytest-dev:master.

@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 14, 2016

Thanks! 😁

I will merge this later unless someone says otherwise.

@nicoddemus nicoddemus merged commit d81ee9a into pytest-dev:master Jun 19, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicoddemus

This comment has been minimized.

Copy link
Member

nicoddemus commented Jun 19, 2016

Thanks, sorry for the delay! 😁

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented on testing/acceptance_test.py in e2e6e31 Oct 11, 2018

@taschini
Is the difference in order between PYTHONPATH and sys.path intentional here?
I hope you remember it (asking via #3010 (comment)).. :)

This comment has been minimized.

Copy link
Contributor Author

taschini replied Oct 11, 2018

As far as I can remember, the idea was that we have to make sure the directories ./hello and ./world (both created in line 578) have the highest possible priority when resolving ns_pkg.hello, so both must override whatever could be set in PYTHONPATH.

This comment has been minimized.

Copy link
Contributor

blueyed replied Oct 11, 2018

So it sounds like it was intentional then?
I understand it as set PYTHONPATH=A:B, and sys.path to B:A to ensure that B is picked up before A correctly?

This comment has been minimized.

Copy link
Contributor Author

taschini replied Oct 12, 2018

Yes, that's right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.