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 4425: resolve --basetemp to absolute paths #4427

Merged
merged 5 commits into from
Nov 22, 2018
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 changelog/4425.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Ensure we resolve the absolute path when the given ``--basetemp`` is a relative path.
6 changes: 5 additions & 1 deletion src/_pytest/monkeypatch.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@

import pytest
from _pytest.fixtures import fixture
from _pytest.pathlib import Path

RE_IMPORT_ERROR_NAME = re.compile("^No module named (.*)$")
RE_IMPORT_ERROR_NAME = re.compile(r"^No module named (.*)$")


@fixture
Expand Down Expand Up @@ -267,6 +268,9 @@ def chdir(self, path):
self._cwd = os.getcwd()
if hasattr(path, "chdir"):
path.chdir()
elif isinstance(path, Path):
# modern python uses the fspath protocol here LEGACY
os.chdir(str(path))
else:
os.chdir(path)

Expand Down
12 changes: 10 additions & 2 deletions src/_pytest/tmpdir.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import attr
import py
import six

import pytest
from .pathlib import ensure_reset_dir
Expand All @@ -26,7 +27,14 @@ class TempPathFactory(object):

The base directory can be configured using the ``--basetemp`` option."""

_given_basetemp = attr.ib()
_given_basetemp = attr.ib(
# using os.path.abspath() to get absolute path instead of resolve() as it
# does not work the same in all platforms (see #4427)
# Path.absolute() exists, but it is not public (see https://bugs.python.org/issue25012)
convert=attr.converters.optional(
lambda p: Path(os.path.abspath(six.text_type(p)))

Choose a reason for hiding this comment

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

Brief note (was stalking IRC so I saw this comment) -- resolve() doesn't only resolve symlinks, but on windows it also resolves UNC paths from mounts (e.g. if you mount a network share at R:\\Some\\path but it really points at \\10.1.100.100\Some\path, Path.resolve() captures that but os.path.abspath() will return R:\\blah -- despite absolute not being public we typically handle this by trying resolve and falling back on absolute in pipenv at least

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @techalchemy for the note, that's interesting! 👍

)
)
_trace = attr.ib()
_basetemp = attr.ib(default=None)

Expand All @@ -53,7 +61,7 @@ def getbasetemp(self):
""" return base temporary directory. """
if self._basetemp is None:
if self._given_basetemp is not None:
basetemp = Path(self._given_basetemp)
basetemp = self._given_basetemp
ensure_reset_dir(basetemp)
else:
from_env = os.environ.get("PYTEST_DEBUG_TEMPROOT")
Expand Down
33 changes: 30 additions & 3 deletions testing/test_tmpdir.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import sys

import attr
import six

import pytest
Expand All @@ -25,12 +26,29 @@ def test_ensuretemp(recwarn):
assert d1.check(dir=1)


@attr.s
class FakeConfig(object):
Copy link
Member

Choose a reason for hiding this comment

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

TBH not sure if this fake actually improves things... the "option returns self" and get are quite the hack. 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

its indeed quite a hack, we cant make a sane config object for use in such cases

basetemp = attr.ib()
trace = attr.ib(default=None)

@property
def trace(self):
return self

def get(self, key):
return lambda *k: None

@property
def option(self):
return self


class TestTempdirHandler(object):
def test_mktemp(self, testdir):
def test_mktemp(self, tmp_path):

from _pytest.tmpdir import TempdirFactory, TempPathFactory

config = testdir.parseconfig()
config.option.basetemp = testdir.mkdir("hello")
config = FakeConfig(tmp_path)
t = TempdirFactory(TempPathFactory.from_config(config))
tmp = t.mktemp("world")
assert tmp.relto(t.getbasetemp()) == "world0"
Expand All @@ -40,6 +58,15 @@ def test_mktemp(self, testdir):
assert tmp2.relto(t.getbasetemp()).startswith("this")
assert tmp2 != tmp

@pytest.mark.issue(4425)
def test_tmppath_relative_basetemp_absolute(self, tmp_path, monkeypatch):
from _pytest.tmpdir import TempPathFactory
Copy link
Member

Choose a reason for hiding this comment

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

I would add a docstring and a reference to the bug. 👍


monkeypatch.chdir(tmp_path)
config = FakeConfig("hello")
t = TempPathFactory.from_config(config)
assert t.getbasetemp().resolve() == (tmp_path / "hello").resolve()


class TestConfigTmpdir(object):
def test_getbasetemp_custom_removes_old(self, testdir):
Expand Down