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 all commits
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
50 changes: 47 additions & 3 deletions _pytest/main.py
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

Expand Down Expand Up @@ -206,6 +208,46 @@ def pytest_ignore_collect(path, config):
return False


@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


class FSHookProxy:
def __init__(self, fspath, pm, remove_mods):
self.fspath = fspath
Expand Down Expand Up @@ -728,17 +770,19 @@ def _tryconvertpyarg(self, x):
"""Convert a dotted module name to path.

"""
import pkgutil

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():
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'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
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 -> ../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)
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