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

Fix issue where working dir becomes wrong on subst drive on Windows. Fixes #5965 #6523

Merged
merged 3 commits into from
Jun 8, 2020
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
9 changes: 9 additions & 0 deletions changelog/5965.breaking.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
symlinks are no longer resolved during collection and matching `conftest.py` files with test file paths.

Resolving symlinks for the current directory and during collection was introduced as a bugfix in 3.9.0, but it actually is a new feature which had unfortunate consequences in Windows and surprising results in other platforms.

The team decided to step back on resolving symlinks at all, planning to review this in the future with a more solid solution (see discussion in
`#6523 <https://github.com/pytest-dev/pytest/pull/6523>`__ for details).

This might break test suites which made use of this feature; the fix is to create a symlink
for the entire test tree, and not only to partial files/tress as it was possible previously.
6 changes: 3 additions & 3 deletions src/_pytest/capture.py
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@ def _py36_windowsconsoleio_workaround(stream: TextIO) -> None:
return

buffered = hasattr(stream.buffer, "raw")
raw_stdout = stream.buffer.raw if buffered else stream.buffer
raw_stdout = stream.buffer.raw if buffered else stream.buffer # type: ignore[attr-defined]
nicoddemus marked this conversation as resolved.
Show resolved Hide resolved

if not isinstance(raw_stdout, io._WindowsConsoleIO):
if not isinstance(raw_stdout, io._WindowsConsoleIO): # type: ignore[attr-defined]
return

def _reopen_stdio(f, mode):
Expand All @@ -135,7 +135,7 @@ def _reopen_stdio(f, mode):
buffering = -1

return io.TextIOWrapper(
open(os.dup(f.fileno()), mode, buffering),
open(os.dup(f.fileno()), mode, buffering), # type: ignore[arg-type]
f.encoding,
f.errors,
f.newlines,
Expand Down
6 changes: 3 additions & 3 deletions src/_pytest/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ def get_config(args=None, plugins=None):
config = Config(
pluginmanager,
invocation_params=Config.InvocationParams(
args=args or (), plugins=plugins, dir=Path().resolve()
args=args or (), plugins=plugins, dir=Path.cwd()
),
)

Expand Down Expand Up @@ -478,7 +478,7 @@ def _getconftestmodules(self, path):
# and allow users to opt into looking into the rootdir parent
# directories instead of requiring to specify confcutdir
clist = []
for parent in directory.realpath().parts():
for parent in directory.parts():
if self._confcutdir and self._confcutdir.relto(parent):
continue
conftestpath = parent.join("conftest.py")
Expand Down Expand Up @@ -799,7 +799,7 @@ def __init__(

if invocation_params is None:
invocation_params = self.InvocationParams(
args=(), plugins=None, dir=Path().resolve()
args=(), plugins=None, dir=Path.cwd()
)

self.option = argparse.Namespace()
Expand Down
2 changes: 1 addition & 1 deletion src/_pytest/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -1496,7 +1496,7 @@ def getfixtureinfo(
def pytest_plugin_registered(self, plugin: _PluggyPlugin) -> None:
nodeid = None
try:
p = py.path.local(plugin.__file__).realpath() # type: ignore[attr-defined] # noqa: F821
p = py.path.local(plugin.__file__) # type: ignore[attr-defined] # noqa: F821
except AttributeError:
pass
else:
Expand Down
1 change: 0 additions & 1 deletion src/_pytest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,6 @@ def _parsearg(self, arg: str) -> Tuple[py.path.local, List[str]]:
"file or package not found: " + arg + " (missing __init__.py?)"
)
raise UsageError("file not found: " + arg)
fspath = fspath.realpath()
return (fspath, parts)

def matchnodes(
Expand Down
9 changes: 9 additions & 0 deletions src/_pytest/pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from typing import TypeVar
from typing import Union

from _pytest.outcomes import skip
from _pytest.warning_types import PytestWarning

if sys.version_info[:2] >= (3, 6):
Expand Down Expand Up @@ -397,3 +398,11 @@ def fnmatch_ex(pattern: str, path) -> bool:
def parts(s: str) -> Set[str]:
parts = s.split(sep)
return {sep.join(parts[: i + 1]) or sep for i in range(len(parts))}


def symlink_or_skip(src, dst, **kwargs):
"""Makes a symlink or skips the test in case symlinks are not supported."""
try:
os.symlink(str(src), str(dst), **kwargs)
except OSError as e:
skip("symlinks not supported: {}".format(e))
74 changes: 16 additions & 58 deletions testing/acceptance_test.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import os
import sys
import textwrap
import types

import attr
Expand All @@ -9,6 +8,7 @@
import pytest
from _pytest.compat import importlib_metadata
from _pytest.config import ExitCode
from _pytest.pathlib import symlink_or_skip
from _pytest.pytester import Testdir


Expand Down Expand Up @@ -266,29 +266,6 @@ def test_conftest_printing_shows_if_error(self, testdir):
assert result.ret != 0
assert "should be seen" in result.stdout.str()

@pytest.mark.skipif(
not hasattr(py.path.local, "mksymlinkto"),
reason="symlink not available on this platform",
)
def test_chdir(self, testdir):
testdir.tmpdir.join("py").mksymlinkto(py._pydir)
p = testdir.tmpdir.join("main.py")
p.write(
textwrap.dedent(
"""\
import sys, os
sys.path.insert(0, '')
import py
print(py.__file__)
print(py.__path__)
os.chdir(os.path.dirname(os.getcwd()))
print(py.log)
"""
)
)
result = testdir.runpython(p)
assert not result.ret

def test_issue109_sibling_conftests_not_loaded(self, testdir):
sub1 = testdir.mkdir("sub1")
sub2 = testdir.mkdir("sub2")
Expand Down Expand Up @@ -762,19 +739,9 @@ def test():

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)
--pyargs with packages with path containing symlink can have conftest.py in
their package (#2985)
"""
# dummy check that we can actually create symlinks: on Windows `os.symlink` is available,
# but normal users require special admin privileges to create symlinks.
if sys.platform == "win32":
try:
os.symlink(
str(testdir.tmpdir.ensure("tmpfile")),
str(testdir.tmpdir.join("tmpfile2")),
)
except OSError as e:
pytest.skip(str(e.args[0]))
monkeypatch.delenv("PYTHONDONTWRITEBYTECODE", raising=False)

dirname = "lib"
Expand All @@ -790,13 +757,13 @@ def test_cmdline_python_package_symlink(self, testdir, monkeypatch):
"import pytest\n@pytest.fixture\ndef a_fixture():pass"
)

d_local = testdir.mkdir("local")
symlink_location = os.path.join(str(d_local), "lib")
os.symlink(str(d), symlink_location, target_is_directory=True)
d_local = testdir.mkdir("symlink_root")
symlink_location = d_local / "lib"
symlink_or_skip(d, symlink_location, target_is_directory=True)

# The structure of the test directory is now:
# .
# ├── local
# ├── symlink_root
# │ └── lib -> ../lib
# └── lib
# └── foo
Expand All @@ -807,32 +774,23 @@ def test_cmdline_python_package_symlink(self, testdir, monkeypatch):
# └── test_bar.py

# NOTE: the different/reversed ordering is intentional here.
search_path = ["lib", os.path.join("local", "lib")]
search_path = ["lib", os.path.join("symlink_root", "lib")]
monkeypatch.setenv("PYTHONPATH", prepend_pythonpath(*search_path))
for p in search_path:
monkeypatch.syspath_prepend(p)

# module picked up in symlink-ed directory:
# It picks up local/lib/foo/bar (symlink) via sys.path.
# It picks up symlink_root/lib/foo/bar (symlink) via sys.path.
result = testdir.runpytest("--pyargs", "-v", "foo.bar")
testdir.chdir()
assert result.ret == 0
if hasattr(py.path.local, "mksymlinkto"):
result.stdout.fnmatch_lines(
[
"lib/foo/bar/test_bar.py::test_bar PASSED*",
"lib/foo/bar/test_bar.py::test_other PASSED*",
"*2 passed*",
]
)
else:
result.stdout.fnmatch_lines(
[
"*lib/foo/bar/test_bar.py::test_bar PASSED*",
"*lib/foo/bar/test_bar.py::test_other PASSED*",
"*2 passed*",
]
)
result.stdout.fnmatch_lines(
[
"symlink_root/lib/foo/bar/test_bar.py::test_bar PASSED*",
"symlink_root/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")
Expand Down
32 changes: 10 additions & 22 deletions testing/test_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
import sys
import textwrap

import py

import pytest
from _pytest.config import ExitCode
from _pytest.main import _in_venv
from _pytest.main import Session
from _pytest.pathlib import symlink_or_skip
from _pytest.pytester import Testdir


Expand Down Expand Up @@ -1164,29 +1163,21 @@ def test_collect_pyargs_with_testpaths(testdir, monkeypatch):
result.stdout.fnmatch_lines(["*1 passed in*"])


@pytest.mark.skipif(
not hasattr(py.path.local, "mksymlinkto"),
reason="symlink not available on this platform",
)
def test_collect_symlink_file_arg(testdir):
"""Test that collecting a direct symlink, where the target does not match python_files works (#4325)."""
"""Collect a direct symlink works even if it does not match python_files (#4325)."""
real = testdir.makepyfile(
real="""
def test_nodeid(request):
assert request.node.nodeid == "real.py::test_nodeid"
assert request.node.nodeid == "symlink.py::test_nodeid"
"""
)
symlink = testdir.tmpdir.join("symlink.py")
symlink.mksymlinkto(real)
symlink_or_skip(real, symlink)
result = testdir.runpytest("-v", symlink)
result.stdout.fnmatch_lines(["real.py::test_nodeid PASSED*", "*1 passed in*"])
result.stdout.fnmatch_lines(["symlink.py::test_nodeid PASSED*", "*1 passed in*"])
Copy link
Member

Choose a reason for hiding this comment

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

i fear that this detail change may cause some troublr wrt resolution of symlinks

Copy link
Contributor Author

@fabioz fabioz May 7, 2020

Choose a reason for hiding this comment

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

@RonnyPfannschmidt, well, the whole point of the pull request is not resolving symlinks (I think my comment at #6523 (comment) explains the rationale).

I'll reword it here, so, hopefully it's clearer: if pytest does resolve symlinks, it's not possible to actually use symlinks to structure projects (be it subst drives, junctions or symlinks) because the paths reported by pytest never match the cwd being used for the run and the only usage for symlinks is as a helper to cd into a directory (which is what the current symlink resolution accomplishes).

So, what pytest maintainers need to decide is what is the wanted behavior for pytest (and I believe those are mutually exclusive, either users are allowed to use symlinks to structure the project or users are allowed to use symlinks as a helper to cd into a directory).

Copy link
Member

Choose a reason for hiding this comment

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

on linux/unix the resolve behaviour makes a lot of sense (and makes things better/avoids certain issues) on windows it seems like the resolve behavour should be to "not" do it if it can be avoided so its a topic where any binary choice is "bad" and a non-binary one may be even worse

in any case its a breaking change that requires a major version, im kinda onboard with making it happen, but i wonder if it should be controllable with a environment variable with sane defaults for unix to resolve and for windows to "not" resolve

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 ok in checking for an env variable and updating the patch (and possibly making the default different for each OS). How do you think it should be named?

Copy link

Choose a reason for hiding this comment

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

Not using Linux as a desktop, I can only speak to Windows, but, in my experience, most apps don't do any resolution of symlinks on Windows, so if pytest DOES resolve, paths to the same file will not match. IMHO, an env var is fine for people to configure for those edge cases, but the default for each OS should be the thing that will work for most people. So, in this case, resolve for Linux (as it currently is) and don't resolve for Windows (as it WAS pre-v5.1.0).

Given the above, I would argue that it isn't a breaking change - there is no change on Linux and it is fixing a bug that was introduced in v5.1.0 on Windows. It's probably not that common for people to use subst on Windows so probably not affecting most Windows users either. If there is anything I can do to help beyond giving my opinions, please let me know how. :-)

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 with Adam

assert result.ret == 0


@pytest.mark.skipif(
not hasattr(py.path.local, "mksymlinkto"),
reason="symlink not available on this platform",
)
def test_collect_symlink_out_of_tree(testdir):
"""Test collection of symlink via out-of-tree rootdir."""
sub = testdir.tmpdir.join("sub")
Expand All @@ -1204,7 +1195,7 @@ def test_nodeid(request):

out_of_tree = testdir.tmpdir.join("out_of_tree").ensure(dir=True)
symlink_to_sub = out_of_tree.join("symlink_to_sub")
symlink_to_sub.mksymlinkto(sub)
symlink_or_skip(sub, symlink_to_sub)
sub.chdir()
result = testdir.runpytest("-vs", "--rootdir=%s" % sub, symlink_to_sub)
result.stdout.fnmatch_lines(
Expand Down Expand Up @@ -1270,22 +1261,19 @@ def test_collect_pkg_init_only(testdir):
result.stdout.fnmatch_lines(["sub/__init__.py::test_init PASSED*", "*1 passed in*"])


@pytest.mark.skipif(
not hasattr(py.path.local, "mksymlinkto"),
reason="symlink not available on this platform",
)
@pytest.mark.parametrize("use_pkg", (True, False))
def test_collect_sub_with_symlinks(use_pkg, testdir):
"""Collection works with symlinked files and broken symlinks"""
sub = testdir.mkdir("sub")
if use_pkg:
sub.ensure("__init__.py")
sub.ensure("test_file.py").write("def test_file(): pass")
sub.join("test_file.py").write("def test_file(): pass")

# Create a broken symlink.
sub.join("test_broken.py").mksymlinkto("test_doesnotexist.py")
symlink_or_skip("test_doesnotexist.py", sub.join("test_broken.py"))

# Symlink that gets collected.
sub.join("test_symlink.py").mksymlinkto("test_file.py")
symlink_or_skip("test_file.py", sub.join("test_symlink.py"))

result = testdir.runpytest("-v", str(sub))
result.stdout.fnmatch_lines(
Expand Down
Loading