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

Improve handling of pyargs #3010

Merged
merged 4 commits into from
Dec 13, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ Grig Gheorghiu
Grigorii Eremeev (budulianin)
Guido Wesdorp
Harald Armin Massa
Henk-Jaap Wagenaar
Hugo van Kemenade
Hui Wang (coldnight)
Ian Bicking
Expand Down
19 changes: 19 additions & 0 deletions _pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,25 @@ def _tryconvertpyarg(self, x):

"""
import pkgutil

if six.PY2: # python 3.4+ uses importlib instead
def find_module_patched(self, fullname, path=None):
subname = fullname.split(".")[-1]
if subname != fullname and self.path is None:
return None
if self.path is None:
path = None
else:
path = [self.path]
try:
file, filename, etc = pkgutil.imp.find_module(subname,
path)
except ImportError:
return None
return pkgutil.ImpLoader(fullname, file, filename, etc)

pkgutil.ImpImporter.find_module = find_module_patched
Copy link
Member

@nicoddemus nicoddemus Dec 11, 2017

Choose a reason for hiding this comment

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

I'm a little concerned about patching this globally like it is done here... who knows what we might break by patching this.

I would rather be conservative and patch this only while we are calling find_loader in the lines below this one. I think a nice way to implement this would be by using a contextmanager:

try:
    with _patched_find_module():
        loader = pkgutil.find_loader(x)
except ImportError:

And _patched_find_module can be implemented like so:

@contextlib.contextmanager
def _patched_find_module():
    """ ... """ 
    if six.PY2:  # python 3.4+ uses importlib instead
        def find_module_patched(self, fullname, path=None):
            subname = fullname.split(".")[-1]
            if subname != fullname and self.path is None:
                return None
            if self.path is None:
                path = None
            else:
                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

We can then even use _patched_find_module()'s docstring to better explain what's going on.


try:
loader = pkgutil.find_loader(x)
except ImportError:
Expand Down
1 change: 1 addition & 0 deletions changelog/2985.bugfix
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.
65 changes: 65 additions & 0 deletions testing/acceptance_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import os
import sys

import six

import _pytest._code
import py
import pytest
Expand Down Expand Up @@ -645,6 +647,69 @@ def join_pythonpath(*dirs):
"*1 passed*"
])

@pytest.mark.skipif(not hasattr(os, "symlink"), reason="requires symlinks")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 -> ../world
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is incorrect, there's no world directory being created

# └── lib
# └── foo
# ├── __init__.py
# └── bar
# ├── __init__.py
# ├── conftest.py
# └── test_world.py
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here, the code above is creating test_bar.py


def join_pythonpath(*dirs):
cur = py.std.os.environ.get('PYTHONPATH')
Copy link
Member

Choose a reason for hiding this comment

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

You can use os.environ directly, we are planning on dropping py.std usage eventually

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

@cryvate
Is the different order between PYTHONPATH and sys.path intentional here?
Fixed it in 2f058ad, which is required to fix the test failure when using realpath() on the args (18e7358), but have not really investigated why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blueyed
Though git blame will assign the 'blame' to me, actually this part of the code was a copy-paste from the test_cmdline_python_namespace_package test so I'd pass on the 'blame' to this PR. Regardless, I'd think that the different order (the opposite way around if I'm reading it correctly) is accidental here as well as in the other test.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cryvate
Thanks and sorry for not looking good enough.

as well as in the other test

Which one do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blueyed
Not a problem at all! I meant test_cmdline_python_namespace_package where I copy-pasted from.


# 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
Expand Down