From 5ae5efc3324e0812ee6a64e98acdd02be2559c73 Mon Sep 17 00:00:00 2001 From: Yusuke Kadowaki Date: Sun, 27 Nov 2022 01:39:21 +0900 Subject: [PATCH 1/7] Refactor tmpdir directory removal process --- src/_pytest/callback.py | 16 ++++++++++++++++ src/_pytest/pathlib.py | 5 +++-- src/_pytest/tmpdir.py | 5 +++++ 3 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 src/_pytest/callback.py diff --git a/src/_pytest/callback.py b/src/_pytest/callback.py new file mode 100644 index 000000000..824e023d7 --- /dev/null +++ b/src/_pytest/callback.py @@ -0,0 +1,16 @@ +from typing import Any +from typing import Callable +from typing import List +from typing import Tuple + + +class Callback: + def __init__(self) -> None: + self._funcs: List[Tuple[Callable[..., Any], Any, Any]] = [] + + def register(self, func: Callable[..., Any], *args: Any, **kwargs: Any) -> None: + self._funcs.append((func, args, kwargs)) + + def execute(self) -> None: + for func, args, kwargs in reversed(self._funcs): + func(*args, **kwargs) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 533335c66..8ebcaef85 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -362,6 +362,7 @@ def make_numbered_dir_with_cleanup( keep: int, lock_timeout: float, mode: int, + register=atexit.register, ) -> Path: """Create a numbered dir with a cleanup lock and remove old ones.""" e = None @@ -371,13 +372,13 @@ def make_numbered_dir_with_cleanup( # 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) + register_cleanup_lock_removal(lock_path, register=register) except Exception as exc: e = exc else: consider_lock_dead_if_created_before = p.stat().st_mtime - lock_timeout # Register a cleanup for program exit - atexit.register( + register( cleanup_numbered_dir, root, prefix, diff --git a/src/_pytest/tmpdir.py b/src/_pytest/tmpdir.py index 48670b6c4..1f9d2ba33 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -37,6 +37,7 @@ from _pytest.deprecated import check_ispytest from _pytest.fixtures import fixture from _pytest.fixtures import FixtureRequest from _pytest.monkeypatch import MonkeyPatch +from _pytest.callback import Callback tmppath_result_key = StashKey[Dict[str, bool]]() @@ -77,6 +78,7 @@ class TempPathFactory: self._retention_count = retention_count self._retention_policy = retention_policy self._basetemp = basetemp + self._lock_and_dir_removal_callback = Callback() @classmethod def from_config( @@ -196,6 +198,7 @@ class TempPathFactory: keep=keep, lock_timeout=LOCK_TIMEOUT, mode=0o700, + register=self._lock_and_dir_removal_callback.register, ) assert basetemp is not None, basetemp self._basetemp = basetemp @@ -315,6 +318,8 @@ def pytest_sessionfinish(session, exitstatus: Union[int, ExitCode]): # permissions, etc, in which case we ignore it. rmtree(passed_dir, ignore_errors=True) + tmp_path_factory._lock_and_dir_removal_callback.execute() + @hookimpl(tryfirst=True, hookwrapper=True) def pytest_runtest_makereport(item: Item, call): From a0b5dbc11bd79c9046085d52e6978094554574ce Mon Sep 17 00:00:00 2001 From: Yusuke Kadowaki Date: Tue, 29 Nov 2022 00:36:14 +0900 Subject: [PATCH 2/7] Add integration tests for tmpdir --- testing/test_tmpdir.py | 73 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 57f442b04..38048731e 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -92,6 +92,74 @@ class TestConfigTmpPath: assert mytemp.exists() assert not mytemp.joinpath("hello").exists() + def test_policy_none_delete_all(self, pytester: Pytester) -> None: + p = pytester.makepyfile( + """ + def test_1(tmp_path): + assert 0 == 0 + """ + ) + p_failed = pytester.makepyfile( + """ + def test_1(tmp_path): + assert 0 == 1 + """ + ) + pytester.makepyprojecttoml( + """ + [tool.pytest.ini_options] + tmp_path_retention_policy = "none" + """ + ) + + pytester.inline_run(p) + pytester.inline_run(p_failed) + + root = pytester._test_tmproot + for child in root.iterdir(): + base_dir = filter(lambda x: x.is_dir(), child.iterdir()) + # Check the base dir itself is gone without depending on test results + assert len(list(base_dir)) == 0 + + @pytest.mark.parametrize("policy", ["failed", "all"]) + @pytest.mark.parametrize("count", [0, 1, 3]) + def test_retention_count(self, pytester: Pytester, policy, count) -> None: + p = pytester.makepyfile( + """ + def test_1(tmp_path): + assert 0 == 0 + """ + ) + p_failed = pytester.makepyfile( + """ + def test_1(tmp_path): + assert 0 == 1 + """ + ) + + pytester.makepyprojecttoml( + f""" + [tool.pytest.ini_options] + tmp_path_retention_policy = {policy} + tmp_path_retention_count = {count} + """ + ) + + pytester.inline_run(p) + pytester.inline_run(p_failed) + pytester.inline_run(p) + pytester.inline_run(p_failed) + pytester.inline_run(p) + pytester.inline_run(p_failed) + pytester.inline_run(p) + pytester.inline_run(p_failed) + + root = pytester._test_tmproot + for child in root.iterdir(): + base_dir = filter(lambda x: not x.is_symlink(), child.iterdir()) + # Check the base dir itself is gone + assert len(list(base_dir)) == count + def test_policy_failed_removes_only_passed_dir(self, pytester: Pytester) -> None: p = pytester.makepyfile( """ @@ -132,10 +200,7 @@ class TestConfigTmpPath: pytester.inline_run(p) root = pytester._test_tmproot for child in root.iterdir(): - # This symlink will be deleted by cleanup_numbered_dir **after** - # the test finishes because it's triggered by atexit. - # So it has to be ignored here. - base_dir = filter(lambda x: not x.is_symlink(), child.iterdir()) + base_dir = filter(lambda x: x.is_dir(), child.iterdir()) # Check the base dir itself is gone assert len(list(base_dir)) == 0 From 58c5d5089a35ae00a852ec9bbf655c7d3a863195 Mon Sep 17 00:00:00 2001 From: Yusuke Kadowaki Date: Wed, 30 Nov 2022 23:12:42 +0900 Subject: [PATCH 3/7] Change to use atexit --- src/_pytest/callback.py | 16 ---------------- src/_pytest/pathlib.py | 5 ++--- src/_pytest/tmpdir.py | 36 +++++++++++++++++++----------------- 3 files changed, 21 insertions(+), 36 deletions(-) delete mode 100644 src/_pytest/callback.py diff --git a/src/_pytest/callback.py b/src/_pytest/callback.py deleted file mode 100644 index 824e023d7..000000000 --- a/src/_pytest/callback.py +++ /dev/null @@ -1,16 +0,0 @@ -from typing import Any -from typing import Callable -from typing import List -from typing import Tuple - - -class Callback: - def __init__(self) -> None: - self._funcs: List[Tuple[Callable[..., Any], Any, Any]] = [] - - def register(self, func: Callable[..., Any], *args: Any, **kwargs: Any) -> None: - self._funcs.append((func, args, kwargs)) - - def execute(self) -> None: - for func, args, kwargs in reversed(self._funcs): - func(*args, **kwargs) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 8ebcaef85..796e2935d 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -1,4 +1,3 @@ -import atexit import contextlib import fnmatch import importlib.util @@ -244,7 +243,7 @@ def create_cleanup_lock(p: Path) -> Path: return lock_path -def register_cleanup_lock_removal(lock_path: Path, register=atexit.register): +def register_cleanup_lock_removal(lock_path: Path, register): """Register a cleanup function for removing a lock, by default on atexit.""" pid = os.getpid() @@ -362,7 +361,7 @@ def make_numbered_dir_with_cleanup( keep: int, lock_timeout: float, mode: int, - register=atexit.register, + register, ) -> Path: """Create a numbered dir with a cleanup lock and remove old ones.""" e = None diff --git a/src/_pytest/tmpdir.py b/src/_pytest/tmpdir.py index 1f9d2ba33..e87c5f081 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -3,6 +3,7 @@ import os import re import sys import tempfile +from contextlib import ExitStack from pathlib import Path from shutil import rmtree from typing import Dict @@ -37,7 +38,6 @@ from _pytest.deprecated import check_ispytest from _pytest.fixtures import fixture from _pytest.fixtures import FixtureRequest from _pytest.monkeypatch import MonkeyPatch -from _pytest.callback import Callback tmppath_result_key = StashKey[Dict[str, bool]]() @@ -78,7 +78,7 @@ class TempPathFactory: self._retention_count = retention_count self._retention_policy = retention_policy self._basetemp = basetemp - self._lock_and_dir_removal_callback = Callback() + self._exit_stack = ExitStack() @classmethod def from_config( @@ -198,7 +198,7 @@ class TempPathFactory: keep=keep, lock_timeout=LOCK_TIMEOUT, mode=0o700, - register=self._lock_and_dir_removal_callback.register, + register=self._exit_stack.callback, ) assert basetemp is not None, basetemp self._basetemp = basetemp @@ -304,21 +304,23 @@ def pytest_sessionfinish(session, exitstatus: Union[int, ExitCode]): 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 - if ( - exitstatus == 0 - and policy == "failed" - and tmp_path_factory._given_basetemp is None - ): - passed_dir = tmp_path_factory._basetemp - if passed_dir.exists(): - # 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(passed_dir, ignore_errors=True) - tmp_path_factory._lock_and_dir_removal_callback.execute() + # tmporal directory cleanup, which is registered to + # this ExitStack, will be executed at the end of this scope + with tmp_path_factory._exit_stack: + if tmp_path_factory._basetemp is None: + return + policy = tmp_path_factory._retention_policy + if ( + exitstatus == 0 + and policy == "failed" + and tmp_path_factory._given_basetemp is None + ): + passed_dir = tmp_path_factory._basetemp + if passed_dir.exists(): + # 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(passed_dir, ignore_errors=True) @hookimpl(tryfirst=True, hookwrapper=True) From 12cba4a50b7bdc211e6800cb385176a81761712c Mon Sep 17 00:00:00 2001 From: Yusuke Kadowaki Date: Wed, 30 Nov 2022 23:12:52 +0900 Subject: [PATCH 4/7] Refactor test --- testing/test_tmpdir.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 38048731e..c315055f9 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -117,9 +117,9 @@ class TestConfigTmpPath: root = pytester._test_tmproot for child in root.iterdir(): - base_dir = filter(lambda x: x.is_dir(), child.iterdir()) + base_dir = list(child.iterdir()) # Check the base dir itself is gone without depending on test results - assert len(list(base_dir)) == 0 + assert base_dir == [] @pytest.mark.parametrize("policy", ["failed", "all"]) @pytest.mark.parametrize("count", [0, 1, 3]) @@ -157,7 +157,6 @@ class TestConfigTmpPath: root = pytester._test_tmproot for child in root.iterdir(): base_dir = filter(lambda x: not x.is_symlink(), child.iterdir()) - # Check the base dir itself is gone assert len(list(base_dir)) == count def test_policy_failed_removes_only_passed_dir(self, pytester: Pytester) -> None: @@ -200,9 +199,9 @@ class TestConfigTmpPath: pytester.inline_run(p) root = pytester._test_tmproot for child in root.iterdir(): - base_dir = filter(lambda x: x.is_dir(), child.iterdir()) + base_dir = list(child.iterdir()) # Check the base dir itself is gone - assert len(list(base_dir)) == 0 + assert base_dir == [] # issue #10502 def test_policy_failed_removes_dir_when_skipped_from_fixture( @@ -225,10 +224,8 @@ class TestConfigTmpPath: # Check if the whole directory is removed 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) == 0 + base_dir = list(child.iterdir()) + assert base_dir == [] # issue #10502 def test_policy_all_keeps_dir_when_skipped_from_fixture( From 829f7ace228a3854b717dab720b47a82ca0c8372 Mon Sep 17 00:00:00 2001 From: Yusuke Kadowaki Date: Thu, 1 Dec 2022 00:12:50 +0900 Subject: [PATCH 5/7] Fix test that wasn't run at all --- testing/test_tmpdir.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index c315055f9..efff3251f 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -121,7 +121,7 @@ class TestConfigTmpPath: # Check the base dir itself is gone without depending on test results assert base_dir == [] - @pytest.mark.parametrize("policy", ["failed", "all"]) + @pytest.mark.parametrize("policy", ['"failed"', '"all"']) @pytest.mark.parametrize("count", [0, 1, 3]) def test_retention_count(self, pytester: Pytester, policy, count) -> None: p = pytester.makepyfile( From d43620a8daeee96c477b50795f675ec7b560f684 Mon Sep 17 00:00:00 2001 From: Yusuke Kadowaki Date: Thu, 1 Dec 2022 01:08:35 +0900 Subject: [PATCH 6/7] Distinguish test file name --- testing/test_tmpdir.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index efff3251f..6a29c03b1 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -131,7 +131,7 @@ class TestConfigTmpPath: """ ) p_failed = pytester.makepyfile( - """ + another_file_name=""" def test_1(tmp_path): assert 0 == 1 """ From 5e6ce8f43306be77d54cf131f53f5012f2d8a785 Mon Sep 17 00:00:00 2001 From: Yusuke Kadowaki Date: Sat, 3 Dec 2022 16:35:02 +0900 Subject: [PATCH 7/7] Refactor tests for tmpdir --- testing/test_tmpdir.py | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 6a29c03b1..1daf04613 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -100,7 +100,7 @@ class TestConfigTmpPath: """ ) p_failed = pytester.makepyfile( - """ + another_file_name=""" def test_1(tmp_path): assert 0 == 1 """ @@ -186,23 +186,6 @@ class TestConfigTmpPath: 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 = list(child.iterdir()) - # Check the base dir itself is gone - assert base_dir == [] - # issue #10502 def test_policy_failed_removes_dir_when_skipped_from_fixture( self, pytester: Pytester