-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Improve handling of pyargs #3010
Changes from 3 commits
b62fd79
dc19624
3ca1e4b
1e29553
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
""" core implementation of testing process: init, session, runtest loop. """ | ||
from __future__ import absolute_import, division, print_function | ||
|
||
import contextlib | ||
import functools | ||
import os | ||
import pkgutil | ||
import six | ||
import sys | ||
|
||
|
@@ -728,17 +730,57 @@ def _tryconvertpyarg(self, x): | |
"""Convert a dotted module name to path. | ||
|
||
""" | ||
import pkgutil | ||
@contextlib.contextmanager | ||
def _patched_find_module(): | ||
"""Patch bug in pkgutil.ImpImporter.find_module | ||
|
||
When using pkgutil.find_loader on python<3.4 it removes symlinks | ||
from the path due to a call to os.path.realpath. This is not consistent | ||
with actually doing the import (in these versions, pkgutil and __import__ | ||
did not share the same underlying code). This can break conftest | ||
discovery for pytest where symlinks are involved. | ||
|
||
The only supported python<3.4 by pytest is python 2.7. | ||
""" | ||
if six.PY2: # python 3.4+ uses importlib instead | ||
def find_module_patched(self, fullname, path=None): | ||
# Note: we ignore 'path' argument since it is only used via meta_path | ||
subname = fullname.split(".")[-1] | ||
if subname != fullname and self.path is None: | ||
return None | ||
if self.path is None: | ||
path = None | ||
else: | ||
# original: path = [os.path.realpath(self.path)] | ||
path = [self.path] | ||
try: | ||
file, filename, etc = pkgutil.imp.find_module(subname, | ||
path) | ||
except ImportError: | ||
return None | ||
return pkgutil.ImpLoader(fullname, file, filename, etc) | ||
|
||
old_find_module = pkgutil.ImpImporter.find_module | ||
pkgutil.ImpImporter.find_module = find_module_patched | ||
try: | ||
yield | ||
finally: | ||
pkgutil.ImpImporter.find_module = old_find_module | ||
else: | ||
yield | ||
|
||
try: | ||
loader = pkgutil.find_loader(x) | ||
with _patched_find_module(): | ||
loader = pkgutil.find_loader(x) | ||
except ImportError: | ||
return x | ||
if loader is None: | ||
return x | ||
# This method is sometimes invoked when AssertionRewritingHook, which | ||
# does not define a get_filename method, is already in place: | ||
try: | ||
path = loader.get_filename(x) | ||
with _patched_find_module(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% sure why it is needed here, but I do know the tests fail if you don't have patching in both places. |
||
path = loader.get_filename(x) | ||
except AttributeError: | ||
# Retrieve path from AssertionRewritingHook: | ||
path = loader.modules[x][0].co_filename | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Fix conversion of pyargs to filename to not convert symlinks and not use deprecated features on Python 3. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ | |
import os | ||
import sys | ||
|
||
import six | ||
|
||
import _pytest._code | ||
import py | ||
import pytest | ||
|
@@ -645,6 +647,69 @@ def join_pythonpath(*dirs): | |
"*1 passed*" | ||
]) | ||
|
||
@pytest.mark.skipif(not hasattr(os, "symlink"), reason="requires symlinks") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assumes if the python/os supports symlinks, so does the filesystem the temporary directory is created. This is not a given, but as this is in testing only, and generally this is the case (both in CI and locally) in most workflows, I think this is good enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree 😁 |
||
def test_cmdline_python_package_symlink(self, testdir, monkeypatch): | ||
""" | ||
test --pyargs option with packages with path containing symlink can | ||
have conftest.py in their package (#2985) | ||
""" | ||
monkeypatch.delenv('PYTHONDONTWRITEBYTECODE', raising=False) | ||
|
||
search_path = ["lib", os.path.join("local", "lib")] | ||
|
||
dirname = "lib" | ||
d = testdir.mkdir(dirname) | ||
foo = d.mkdir("foo") | ||
foo.ensure("__init__.py") | ||
lib = foo.mkdir('bar') | ||
lib.ensure("__init__.py") | ||
lib.join("test_bar.py"). \ | ||
write("def test_bar(): pass\n" | ||
"def test_other(a_fixture):pass") | ||
lib.join("conftest.py"). \ | ||
write("import pytest\n" | ||
"@pytest.fixture\n" | ||
"def a_fixture():pass") | ||
|
||
d_local = testdir.mkdir("local") | ||
symlink_location = os.path.join(str(d_local), "lib") | ||
if six.PY2: | ||
os.symlink(str(d), symlink_location) | ||
else: | ||
os.symlink(str(d), symlink_location, target_is_directory=True) | ||
|
||
# The structure of the test directory is now: | ||
# . | ||
# ├── local | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to keep close to the actual issue, and kept the virtualenv structure. |
||
# │ └── lib -> ../lib | ||
# └── lib | ||
# └── foo | ||
# ├── __init__.py | ||
# └── bar | ||
# ├── __init__.py | ||
# ├── conftest.py | ||
# └── test_bar.py | ||
|
||
def join_pythonpath(*dirs): | ||
cur = os.getenv('PYTHONPATH') | ||
if cur: | ||
dirs += (cur,) | ||
return os.pathsep.join(str(p) for p in dirs) | ||
|
||
monkeypatch.setenv('PYTHONPATH', join_pythonpath(*search_path)) | ||
for p in search_path: | ||
monkeypatch.syspath_prepend(p) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @blueyed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cryvate
Which one do you mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @blueyed |
||
|
||
# module picked up in symlink-ed directory: | ||
result = testdir.runpytest("--pyargs", "-v", "foo.bar") | ||
testdir.chdir() | ||
assert result.ret == 0 | ||
result.stdout.fnmatch_lines([ | ||
"*lib/foo/bar/test_bar.py::test_bar*PASSED*", | ||
"*lib/foo/bar/test_bar.py::test_other*PASSED*", | ||
"*2 passed*" | ||
]) | ||
|
||
def test_cmdline_python_package_not_exists(self, testdir): | ||
result = testdir.runpytest("--pyargs", "tpkgwhatv") | ||
assert result.ret | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably better in the global namespace than a local function because it clutters the code quite a bit. Sorry I wasn't clear about that before.