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

Adds a feature to control how directories created by the tmp_path fixture are kept. #10442

Merged
merged 24 commits into from
Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
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 @@ -372,6 +372,7 @@ Xixi Zhao
Xuan Luong
Xuecong Liao
Yoav Caspi
Yusuke Kadowaki
Yuval Shimon
Zac Hatfield-Dodds
Zachary Kneupper
Expand Down
2 changes: 2 additions & 0 deletions changelog/8141.feature.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Added `--tmp-path-retention-count` and `--tmp-path-retention-policy` options to control how directories created by the `tmp_path` fixture are kept.
Copy link
Member

Choose a reason for hiding this comment

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

We should also mention explicitly that the default has changed to remove all directories if all tests passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done
ce71d7b

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Added `--tmp-path-retention-count` and `--tmp-path-retention-policy` options to control how directories created by the `tmp_path` fixture are kept.
Added :confval:`tmp_path_retention_count` and :confval:`tmp_path_retention_policy` configuration options to control how directories created by the :fixture:`tmp_path` fixture are kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with c7eb5a3.

The default behavior has changed to remove all directories if all tests passed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The default behavior has changed to remove all directories if all tests passed.
The default behavior has changed to keep only directories for failed tests, equivalent to `tmp_path_retention_policy="failed"`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with c7eb5a3.

34 changes: 34 additions & 0 deletions doc/en/reference/reference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1723,6 +1723,40 @@ passed multiple times. The expected format is ``name=value``. For example::
directories when executing from the root directory.


.. confval:: tmp_path_retention_count



How many sessions should we keep the `tmp_path` directories,
according to `tmp_path_retention_policy`.

.. code-block:: ini

[pytest]
tmp_path_retention_count = 3

Default: 3


.. confval:: tmp_path_retention_policy



Controls which directories created by the `tmp_path` fixture are kept around,
based on test outcome.

* `all`: retains directories for all tests, regardless of the outcome.
* `failed`: retains directories only for tests with outcome `error` or `failed`.
* `none`: directories are always removed after each test ends, regardless of the outcome.

.. code-block:: ini

[pytest]
tmp_path_retention_policy = "all"

Default: failed


.. confval:: usefixtures

List of fixtures that will be applied to all test functions; this is semantically the same to apply
Expand Down
14 changes: 12 additions & 2 deletions src/_pytest/pathlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -339,11 +339,19 @@ def cleanup_numbered_dir(
root: Path, prefix: str, keep: int, consider_lock_dead_if_created_before: float
) -> None:
"""Cleanup for lock driven numbered directories."""
if not root.exists():
return
for path in cleanup_candidates(root, prefix, keep):
try_cleanup(path, consider_lock_dead_if_created_before)
for path in root.glob("garbage-*"):
try_cleanup(path, consider_lock_dead_if_created_before)

# remove dead symlink
for left_dir in root.iterdir():
if left_dir.is_symlink():
if not left_dir.resolve().exists():
left_dir.unlink()


def make_numbered_dir_with_cleanup(
root: Path,
Expand All @@ -357,8 +365,10 @@ def make_numbered_dir_with_cleanup(
for i in range(10):
try:
p = make_numbered_dir(root, prefix, mode)
lock_path = create_cleanup_lock(p)
register_cleanup_lock_removal(lock_path)
# Only lock the current dir when keep is not 0
if keep != 0:
lock_path = create_cleanup_lock(p)
register_cleanup_lock_removal(lock_path)
except Exception as exc:
e = exc
else:
Expand Down
101 changes: 98 additions & 3 deletions src/_pytest/tmpdir.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,29 @@
import sys
import tempfile
from pathlib import Path
from shutil import rmtree
from typing import Generator
from typing import Optional
from typing import TYPE_CHECKING
from typing import Union

if TYPE_CHECKING:
from typing_extensions import Literal

RetentionType = Literal["all", "failed", "none"]


import attr
from _pytest.config.argparsing import Parser

from .pathlib import LOCK_TIMEOUT
from .pathlib import make_numbered_dir
from .pathlib import make_numbered_dir_with_cleanup
from .pathlib import rm_rf
from _pytest.compat import final
from _pytest.config import Config
from _pytest.config import ExitCode
from _pytest.config import hookimpl
from _pytest.deprecated import check_ispytest
from _pytest.fixtures import fixture
from _pytest.fixtures import FixtureRequest
Expand All @@ -31,10 +44,14 @@ class TempPathFactory:
_given_basetemp = attr.ib(type=Optional[Path])
_trace = attr.ib()
_basetemp = attr.ib(type=Optional[Path])
_retention_count = attr.ib(type=int)
_retention_policy = attr.ib(type="RetentionType")

def __init__(
self,
given_basetemp: Optional[Path],
retention_count: int,
retention_policy: "RetentionType",
trace,
basetemp: Optional[Path] = None,
*,
Expand All @@ -49,6 +66,8 @@ def __init__(
# Path.absolute() exists, but it is not public (see https://bugs.python.org/issue25012).
self._given_basetemp = Path(os.path.abspath(str(given_basetemp)))
self._trace = trace
self._retention_count = retention_count
self._retention_policy = retention_policy
self._basetemp = basetemp

@classmethod
Expand All @@ -63,9 +82,23 @@ def from_config(
:meta private:
"""
check_ispytest(_ispytest)
count = int(config.getini("tmp_path_retention_count"))
if count < 0:
raise ValueError(
f"tmp_path_retention_count must be >= 0. Current input: {count}."
)

policy = config.getini("tmp_path_retention_policy")
if policy not in ("all", "failed", "none"):
raise ValueError(
f"tmp_path_retention_policy must be either all, failed, none. Current intput: {policy}."
)

return cls(
given_basetemp=config.option.basetemp,
trace=config.trace.get("tmpdir"),
retention_count=count,
retention_policy=policy,
_ispytest=True,
)

Expand Down Expand Up @@ -146,10 +179,13 @@ def getbasetemp(self) -> Path:
)
if (rootdir_stat.st_mode & 0o077) != 0:
os.chmod(rootdir, rootdir_stat.st_mode & ~0o077)
keep = self._retention_count
if self._retention_policy == "none":
keep = 0
basetemp = make_numbered_dir_with_cleanup(
prefix="pytest-",
root=rootdir,
keep=3,
keep=keep,
lock_timeout=LOCK_TIMEOUT,
mode=0o700,
)
Expand Down Expand Up @@ -184,6 +220,21 @@ def pytest_configure(config: Config) -> None:
mp.setattr(config, "_tmp_path_factory", _tmp_path_factory, raising=False)


def pytest_addoption(parser: Parser) -> None:
parser.addini(
"tmp_path_retention_count",
help="How many sessions should we keep the `tmp_path` directories, according to `tmp_path_retention_policy`.",
default=3,
)

parser.addini(
"tmp_path_retention_policy",
help="Controls which directories created by the `tmp_path` fixture are kept around, based on test outcome. "
"(all/failed/none)",
default="failed",
)


@fixture(scope="session")
def tmp_path_factory(request: FixtureRequest) -> TempPathFactory:
"""Return a :class:`pytest.TempPathFactory` instance for the test session."""
Expand All @@ -200,7 +251,9 @@ def _mk_tmp(request: FixtureRequest, factory: TempPathFactory) -> Path:


@fixture
def tmp_path(request: FixtureRequest, tmp_path_factory: TempPathFactory) -> Path:
def tmp_path(
request: FixtureRequest, tmp_path_factory: TempPathFactory
) -> Generator[Path, None, None]:
"""Return a temporary directory path object which is unique to each test
function invocation, created as a sub directory of the base temporary
directory.
Expand All @@ -213,4 +266,46 @@ def tmp_path(request: FixtureRequest, tmp_path_factory: TempPathFactory) -> Path
The returned object is a :class:`pathlib.Path` object.
"""

return _mk_tmp(request, tmp_path_factory)
path = _mk_tmp(request, tmp_path_factory)
yield path

# Remove the tmpdir if the policy is "failed" and the test passed.
tmp_path_factory: TempPathFactory = request.session.config._tmp_path_factory # type: ignore
policy = tmp_path_factory._retention_policy
if policy == "failed" and request.node.result_call.passed:
rmtree(path)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rmtree(path)
# We do a "best effort" to remove files, but it might not be possible due to some leaked resource,
# permissions, etc, in which case we ignore it.
rmtree(path, ignore_errors=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with 94740c0.


# remove dead symlink
basetemp = tmp_path_factory._basetemp
if basetemp is None:
return
for left_dir in basetemp.iterdir():
Copy link
Member

Choose a reason for hiding this comment

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

Lets move this block of code to a function in pathlib.py and reuse it in cleanup_numbered_dir. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with b04bbb1.

if left_dir.is_symlink():
if not left_dir.resolve().exists():
left_dir.unlink()


def pytest_sessionfinish(session, exitstatus: Union[int, ExitCode]):
"""After each session, remove base directory if all the tests passed,
the policy is "failed", and the basetemp is not specified by a user.
"""
tmp_path_factory: TempPathFactory = session.config._tmp_path_factory
if tmp_path_factory._basetemp is None:
return
policy = tmp_path_factory._retention_policy
Copy link
Member

Choose a reason for hiding this comment

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

I was under the impression that the failed policy would remove all passed tests, even if some tests failed.

Here it will remove all directories only if all tests pass.

Or is that handled somewhere else that I am missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it will remove all directories only if all tests pass.

This is actually how I implemented it. And I added some implementation to match the behavior you mentioned.
0abbf8c

if (
exitstatus == 0
and policy == "failed"
and tmp_path_factory._given_basetemp is None
):
# Register to remove the base directory before starting cleanup_numbered_dir
passed_dir = tmp_path_factory._basetemp
if passed_dir.exists():
rmtree(passed_dir)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rmtree(passed_dir)
# We do a "best effort" to remove files, but it might not be possible due to some leaked resource,
# permissions, etc, in which case we ignore it.
rmtree(path, ignore_errors=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with 94740c0.



@hookimpl(tryfirst=True, hookwrapper=True)
def pytest_runtest_makereport(item, call):
outcome = yield
result = outcome.get_result()
setattr(item, "result_" + result.when, result)
Copy link
Member

Choose a reason for hiding this comment

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

Let us make this private to not encourage users and plugins to depend on it.

Suggested change
setattr(item, "result_" + result.when, result)
setattr(item, "_tmp_path_result_" + result.when, result)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with dc11283.

67 changes: 62 additions & 5 deletions testing/test_tmpdir.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,14 @@ def trace(self):
def get(self, key):
return lambda *k: None

def getini(self, name):
if name == "tmp_path_retention_count":
return 3
elif name == "tmp_path_retention_policy":
return "failed"
else:
assert False
Copy link
Member

Choose a reason for hiding this comment

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

👍


@property
def option(self):
return self
Expand Down Expand Up @@ -84,6 +92,50 @@ def test_1(tmp_path):
assert mytemp.exists()
assert not mytemp.joinpath("hello").exists()

def test_policy_failed_removes_only_passed_dir(self, pytester: Pytester) -> None:
p = pytester.makepyfile(
"""
def test_1(tmp_path):
assert 0 == 0
def test_2(tmp_path):
assert 0 == 1
"""
)

pytester.inline_run(p)
root = pytester._test_tmproot

for child in root.iterdir():
base_dir = list(
filter(lambda x: x.is_dir() and not x.is_symlink(), child.iterdir())
)
assert len(base_dir) == 1
test_dir = list(
filter(
lambda x: x.is_dir() and not x.is_symlink(), base_dir[0].iterdir()
)
)
# Check only the failed one remains
assert len(test_dir) == 1
assert test_dir[0].name == "test_20"

def test_policy_failed_removes_basedir_when_all_passed(
self, pytester: Pytester
) -> None:
p = pytester.makepyfile(
"""
def test_1(tmp_path):
assert 0 == 0
"""
)

pytester.inline_run(p)
root = pytester._test_tmproot
for child in root.iterdir():
base_dir = filter(lambda x: not x.is_symlink(), child.iterdir())
Copy link
Member

Choose a reason for hiding this comment

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

Don't we want to ensure the symlinks are removed too?

Suggested change
base_dir = filter(lambda x: not x.is_symlink(), child.iterdir())
base_dir = list(child.iterdir())

Copy link
Contributor Author

@ykadowak ykadowak Nov 3, 2022

Choose a reason for hiding this comment

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

This symlink will be deleted by cleanup_numbered_dir after the test finishes because it's triggered by atexit. So you cannot really test it here.
Added comments about it here instead.

f853117

# Check the base dir itself is gone
assert len(list(base_dir)) == 0


testdata = [
("mypath", True),
Expand Down Expand Up @@ -275,12 +327,12 @@ def test_lock_register_cleanup_removal(self, tmp_path: Path) -> None:

assert not lock.exists()

def _do_cleanup(self, tmp_path: Path) -> None:
def _do_cleanup(self, tmp_path: Path, keep: int = 2) -> None:
self.test_make(tmp_path)
cleanup_numbered_dir(
root=tmp_path,
prefix=self.PREFIX,
keep=2,
keep=keep,
consider_lock_dead_if_created_before=0,
)

Expand All @@ -289,6 +341,11 @@ def test_cleanup_keep(self, tmp_path):
a, b = (x for x in tmp_path.iterdir() if not x.is_symlink())
print(a, b)

def test_cleanup_keep_0(self, tmp_path: Path):
self._do_cleanup(tmp_path, 0)
dir_num = len(list(tmp_path.iterdir()))
assert dir_num == 0

def test_cleanup_locked(self, tmp_path):
p = make_numbered_dir(root=tmp_path, prefix=self.PREFIX)

Expand Down Expand Up @@ -446,7 +503,7 @@ def test_tmp_path_factory_create_directory_with_safe_permissions(
"""Verify that pytest creates directories under /tmp with private permissions."""
# Use the test's tmp_path as the system temproot (/tmp).
monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path))
tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True)
tmp_factory = TempPathFactory(None, 3, "failed", lambda *args: None, _ispytest=True)
basetemp = tmp_factory.getbasetemp()

# No world-readable permissions.
Expand All @@ -466,14 +523,14 @@ def test_tmp_path_factory_fixes_up_world_readable_permissions(
"""
# Use the test's tmp_path as the system temproot (/tmp).
monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path))
tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True)
tmp_factory = TempPathFactory(None, 3, "failed", lambda *args: None, _ispytest=True)
basetemp = tmp_factory.getbasetemp()

# Before - simulate bad perms.
os.chmod(basetemp.parent, 0o777)
assert (basetemp.parent.stat().st_mode & 0o077) != 0

tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True)
tmp_factory = TempPathFactory(None, 3, "failed", lambda *args: None, _ispytest=True)
basetemp = tmp_factory.getbasetemp()

# After - fixed.
Expand Down