From 008555deb4c0e606550a1a39a09b2dea3f61c6d7 Mon Sep 17 00:00:00 2001 From: Fabio Zadrozny Date: Tue, 21 Jan 2020 09:14:57 -0300 Subject: [PATCH 1/3] Fix issue where working dir becomes wrong on symlinks. Fixes #5965 This addresses subst drive on Windows and symlinks on Linux. --- changelog/5965.bugfix.rst | 1 + src/_pytest/config/__init__.py | 6 +-- src/_pytest/fixtures.py | 2 +- src/_pytest/main.py | 1 - testing/acceptance_test.py | 4 +- testing/test_collection.py | 4 +- testing/test_conftest.py | 39 ++++++++-------- testing/test_link_resolve.py | 85 ++++++++++++++++++++++++++++++++++ 8 files changed, 113 insertions(+), 29 deletions(-) create mode 100644 changelog/5965.bugfix.rst create mode 100644 testing/test_link_resolve.py diff --git a/changelog/5965.bugfix.rst b/changelog/5965.bugfix.rst new file mode 100644 index 00000000000..ad9f1eca743 --- /dev/null +++ b/changelog/5965.bugfix.rst @@ -0,0 +1 @@ +No longer resolve symlinks so that the current directory remains correct with drives mapped with subst on Windows or symlinks on Linux. diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py index 27083900dfa..d469674c5fd 100644 --- a/src/_pytest/config/__init__.py +++ b/src/_pytest/config/__init__.py @@ -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() ), ) @@ -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") @@ -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() diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index fa7e3e1df16..8747f56f226 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -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__) except AttributeError: pass else: diff --git a/src/_pytest/main.py b/src/_pytest/main.py index 1c1cda18bdf..84ee008812f 100644 --- a/src/_pytest/main.py +++ b/src/_pytest/main.py @@ -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( diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index e2df92d8091..aba4d15c453 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -820,8 +820,8 @@ def test_cmdline_python_package_symlink(self, testdir, monkeypatch): 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*", + "local/lib/foo/bar/test_bar.py::test_bar PASSED*", + "local/lib/foo/bar/test_bar.py::test_other PASSED*", "*2 passed*", ] ) diff --git a/testing/test_collection.py b/testing/test_collection.py index 8e5d5aaccb0..6d880707514 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -1173,13 +1173,13 @@ def test_collect_symlink_file_arg(testdir): 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) 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*"]) assert result.ret == 0 diff --git a/testing/test_conftest.py b/testing/test_conftest.py index a07af60f6f9..f2349f33243 100644 --- a/testing/test_conftest.py +++ b/testing/test_conftest.py @@ -195,7 +195,21 @@ def pytest_addoption(parser): reason="symlink not available on this platform", ) def test_conftest_symlink(testdir): - """Ensure that conftest.py is used for resolved symlinks.""" + """ + Ensure that conftest.py is not found unless it's not a parent in the current + directory structure (i.e.: symlinks are not resolved). + """ + # Structure: + # /real + # /real/conftest.py + # /real/app + # /real/app/tests + # /real/app/tests/test_foo.py + + # Links: + # /symlinktests -> /real/app/tests (running at symlinktests should fail) + # /symlink -> /real (running at /symlink should work) + real = testdir.tmpdir.mkdir("real") realtests = real.mkdir("app").mkdir("tests") testdir.tmpdir.join("symlinktests").mksymlinkto(realtests) @@ -216,31 +230,16 @@ def fixture(): ), } ) + + # Should fail because conftest cannot be found from the link structure. result = testdir.runpytest("-vs", "symlinktests") - result.stdout.fnmatch_lines( - [ - "*conftest_loaded*", - "real/app/tests/test_foo.py::test1 fixture_used", - "PASSED", - ] - ) - assert result.ret == ExitCode.OK + result.stdout.fnmatch_lines(["*fixture 'fixture' not found*"]) + assert result.ret == ExitCode.TESTS_FAILED # Should not cause "ValueError: Plugin already registered" (#4174). result = testdir.runpytest("-vs", "symlink") assert result.ret == ExitCode.OK - realtests.ensure("__init__.py") - result = testdir.runpytest("-vs", "symlinktests/test_foo.py::test1") - result.stdout.fnmatch_lines( - [ - "*conftest_loaded*", - "real/app/tests/test_foo.py::test1 fixture_used", - "PASSED", - ] - ) - assert result.ret == ExitCode.OK - @pytest.mark.skipif( not hasattr(py.path.local, "mksymlinkto"), diff --git a/testing/test_link_resolve.py b/testing/test_link_resolve.py new file mode 100644 index 00000000000..3e9199dff56 --- /dev/null +++ b/testing/test_link_resolve.py @@ -0,0 +1,85 @@ +import os.path +import subprocess +import sys +import textwrap +from contextlib import contextmanager +from string import ascii_lowercase + +import py.path + +from _pytest import pytester + + +@contextmanager +def subst_path_windows(filename): + for c in ascii_lowercase[7:]: # Create a subst drive from H-Z. + c += ":" + if not os.path.exists(c): + drive = c + break + else: + raise AssertionError("Unable to find suitable drive letter for subst.") + + directory = filename.dirpath() + basename = filename.basename + + args = ["subst", drive, str(directory)] + subprocess.check_call(args) + assert os.path.exists(drive) + try: + filename = py.path.local(drive) / basename + yield filename + finally: + args = ["subst", "/D", drive] + subprocess.check_call(args) + + +@contextmanager +def subst_path_linux(filename): + directory = filename.dirpath() + basename = filename.basename + + target = directory / ".." / "sub2" + os.symlink(str(directory), str(target), target_is_directory=True) + try: + filename = target / basename + yield filename + finally: + # We don't need to unlink (it's all in the tempdir). + pass + + +def test_link_resolve(testdir: pytester.Testdir) -> None: + """ + See: https://github.com/pytest-dev/pytest/issues/5965 + """ + sub1 = testdir.mkpydir("sub1") + p = sub1.join("test_foo.py") + p.write( + textwrap.dedent( + """ + import pytest + def test_foo(): + raise AssertionError() + """ + ) + ) + + subst = subst_path_linux + if sys.platform == "win32": + subst = subst_path_windows + + with subst(p) as subst_p: + result = testdir.runpytest(str(subst_p), "-v") + # i.e.: Make sure that the error is reported as a relative path, not as a + # resolved path. + # See: https://github.com/pytest-dev/pytest/issues/5965 + stdout = result.stdout.str() + assert "sub1/test_foo.py" not in stdout + + # i.e.: Expect drive on windows because we just have drive:filename, whereas + # we expect a relative path on Linux. + expect = ( + "*{}*".format(subst_p) if sys.platform == "win32" else "*sub2/test_foo.py*" + ) + result.stdout.fnmatch_lines([expect]) From ff6af8ade17c16f9a311b6723346c8b25957c903 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Jun 2020 11:29:01 -0300 Subject: [PATCH 2/3] Do not use 'mksymlinkto' as basis for symlink support checks On Windows, symlinks are supported by Administrators and Power Users, so checking symlink sypport by the existence of py.path.local.mksymlinkto is not reliable. Introduce 'symlink_or_skip' which tries to make the symlink and skip the test if the symlink operation fails. --- src/_pytest/capture.py | 6 ++-- src/_pytest/fixtures.py | 2 +- src/_pytest/pathlib.py | 9 +++++ testing/acceptance_test.py | 74 +++++++++----------------------------- testing/test_collection.py | 28 +++++---------- testing/test_conftest.py | 21 ++++------- 6 files changed, 44 insertions(+), 96 deletions(-) diff --git a/src/_pytest/capture.py b/src/_pytest/capture.py index 98ba878b3f0..04104128456 100644 --- a/src/_pytest/capture.py +++ b/src/_pytest/capture.py @@ -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] - if not isinstance(raw_stdout, io._WindowsConsoleIO): + if not isinstance(raw_stdout, io._WindowsConsoleIO): # type: ignore[attr-defined] return def _reopen_stdio(f, mode): @@ -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, diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 8747f56f226..05f0ecb6a47 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1496,7 +1496,7 @@ def getfixtureinfo( def pytest_plugin_registered(self, plugin: _PluggyPlugin) -> None: nodeid = None try: - p = py.path.local(plugin.__file__) + p = py.path.local(plugin.__file__) # type: ignore[attr-defined] # noqa: F821 except AttributeError: pass else: diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 6878965e0c5..69f490a1d48 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -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): @@ -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)) diff --git a/testing/acceptance_test.py b/testing/acceptance_test.py index aba4d15c453..7dfd588a02b 100644 --- a/testing/acceptance_test.py +++ b/testing/acceptance_test.py @@ -1,6 +1,5 @@ import os import sys -import textwrap import types import attr @@ -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 @@ -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") @@ -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" @@ -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 @@ -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( - [ - "local/lib/foo/bar/test_bar.py::test_bar PASSED*", - "local/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") diff --git a/testing/test_collection.py b/testing/test_collection.py index 6d880707514..6644881ea44 100644 --- a/testing/test_collection.py +++ b/testing/test_collection.py @@ -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 @@ -1164,12 +1163,8 @@ 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): @@ -1177,16 +1172,12 @@ def test_nodeid(request): """ ) symlink = testdir.tmpdir.join("symlink.py") - symlink.mksymlinkto(real) + symlink_or_skip(real, symlink) result = testdir.runpytest("-v", symlink) result.stdout.fnmatch_lines(["symlink.py::test_nodeid PASSED*", "*1 passed in*"]) 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") @@ -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( @@ -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( diff --git a/testing/test_conftest.py b/testing/test_conftest.py index f2349f33243..0df303bc7cb 100644 --- a/testing/test_conftest.py +++ b/testing/test_conftest.py @@ -7,6 +7,7 @@ from _pytest.config import ExitCode from _pytest.config import PytestPluginManager from _pytest.pathlib import Path +from _pytest.pathlib import symlink_or_skip def ConftestWithSetinitial(path): @@ -190,14 +191,9 @@ def pytest_addoption(parser): result.stdout.no_fnmatch_line("*warning: could not load initial*") -@pytest.mark.skipif( - not hasattr(py.path.local, "mksymlinkto"), - reason="symlink not available on this platform", -) def test_conftest_symlink(testdir): """ - Ensure that conftest.py is not found unless it's not a parent in the current - directory structure (i.e.: symlinks are not resolved). + conftest.py discovery follows normal path resolution and does not resolve symlinks. """ # Structure: # /real @@ -212,8 +208,8 @@ def test_conftest_symlink(testdir): real = testdir.tmpdir.mkdir("real") realtests = real.mkdir("app").mkdir("tests") - testdir.tmpdir.join("symlinktests").mksymlinkto(realtests) - testdir.tmpdir.join("symlink").mksymlinkto(real) + symlink_or_skip(realtests, testdir.tmpdir.join("symlinktests")) + symlink_or_skip(real, testdir.tmpdir.join("symlink")) testdir.makepyfile( **{ "real/app/tests/test_foo.py": "def test1(fixture): pass", @@ -241,12 +237,9 @@ def fixture(): assert result.ret == ExitCode.OK -@pytest.mark.skipif( - not hasattr(py.path.local, "mksymlinkto"), - reason="symlink not available on this platform", -) def test_conftest_symlink_files(testdir): - """Check conftest.py loading when running in directory with symlinks.""" + """Symlinked conftest.py are found when pytest is executed in a directory with symlinked + files.""" real = testdir.tmpdir.mkdir("real") source = { "app/test_foo.py": "def test1(fixture): pass", @@ -270,7 +263,7 @@ def fixture(): build = testdir.tmpdir.mkdir("build") build.mkdir("app") for f in source: - build.join(f).mksymlinkto(real.join(f)) + symlink_or_skip(real.join(f), build.join(f)) build.chdir() result = testdir.runpytest("-vs", "app/test_foo.py") result.stdout.fnmatch_lines(["*conftest_loaded*", "PASSED"]) From 0f31d8a171c726110ead291fd499a81ee2064822 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Sat, 6 Jun 2020 11:42:42 -0300 Subject: [PATCH 3/3] Update CHANGELOG with more information --- changelog/5965.breaking.rst | 9 +++++++++ changelog/5965.bugfix.rst | 1 - 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 changelog/5965.breaking.rst delete mode 100644 changelog/5965.bugfix.rst diff --git a/changelog/5965.breaking.rst b/changelog/5965.breaking.rst new file mode 100644 index 00000000000..3ecb9486aed --- /dev/null +++ b/changelog/5965.breaking.rst @@ -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 `__ 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. diff --git a/changelog/5965.bugfix.rst b/changelog/5965.bugfix.rst deleted file mode 100644 index ad9f1eca743..00000000000 --- a/changelog/5965.bugfix.rst +++ /dev/null @@ -1 +0,0 @@ -No longer resolve symlinks so that the current directory remains correct with drives mapped with subst on Windows or symlinks on Linux.