diff --git a/AUTHORS b/AUTHORS index 1d1af001595..14b02e3fb36 100644 --- a/AUTHORS +++ b/AUTHORS @@ -383,6 +383,7 @@ Omri Golan Ondřej Súkup Onkesh Bansal Oscar Benjamin +Panayiotis Michalos Paneendra Kumar Parth Patel Patrick Hayes diff --git a/changelog/14998.bugfix.rst b/changelog/14998.bugfix.rst new file mode 100644 index 00000000000..31d63d17a68 --- /dev/null +++ b/changelog/14998.bugfix.rst @@ -0,0 +1,2 @@ +Fixed ``tmp_path_retention_policy="failed"`` incorrectly removing temporary +directories when fixture setup or teardown raises an error. diff --git a/src/_pytest/tmpdir.py b/src/_pytest/tmpdir.py index 616a09d1b27..afc55b76192 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -37,6 +37,7 @@ tmppath_result_key = StashKey[dict[str, bool]]() +tmppath_path_key = StashKey[Path]() RetentionType = Literal["all", "failed", "none"] @@ -292,18 +293,11 @@ def tmp_path( as discussed in :ref:`temporary directory location and retention`. """ path = _mk_tmp(request, tmp_path_factory) - yield path - - # Remove the tmpdir if the policy is "failed" and the test passed. - policy = tmp_path_factory._retention_policy - result_dict = request.node.stash[tmppath_result_key] - if policy == "failed" and result_dict.get("call", True): - # 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) + # Store the path so cleanup can happen after the teardown report. + request.node.stash[tmppath_path_key] = path - del request.node.stash[tmppath_result_key] + yield path def pytest_sessionfinish(session, exitstatus: int | ExitCode): @@ -340,6 +334,25 @@ def pytest_runtest_makereport( ) -> Generator[None, TestReport, TestReport]: rep = yield assert rep.when is not None - empty: dict[str, bool] = {} - item.stash.setdefault(tmppath_result_key, empty)[rep.when] = rep.passed + + # Record whether each phase failed. + result_dict = item.stash.setdefault(tmppath_result_key, {}) + result_dict[rep.when] = rep.failed + + # Wait until teardown has finished before deciding whether the temporary directory should be retained. + if rep.when == "teardown": + path = item.stash.get(tmppath_path_key, None) + + if path is not None: + tmp_path_factory: TempPathFactory = item.config._tmp_path_factory # type: ignore[attr-defined] + policy = tmp_path_factory._retention_policy + + # Retain the directory if any phase failed. Otherwise, remove it under the "failed" policy. + if policy == "failed" and not any(result_dict.values()): + rmtree(path, ignore_errors=True) + + del item.stash[tmppath_path_key] + + del item.stash[tmppath_result_key] + return rep diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 0b33a74b926..fd0ea0bcb47 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -197,6 +197,53 @@ def test_2(tmp_path): assert len(test_dir) == 1 assert test_dir[0].name == "test_20" + def test_policy_failed_keeps_setup_and_teardown_errors( + self, pytester: Pytester + ) -> None: + """Regression test for #14998.""" + pytester.makepyprojecttoml( + """ + [tool.pytest.ini_options] + tmp_path_retention_policy = "failed" + """ + ) + + pytester.makepyfile( + """ + import pytest + + @pytest.fixture + def setup_error(tmp_path): + (tmp_path / "evidence.txt").write_text("setup", encoding='utf-8') + raise RuntimeError("setup failed") + + @pytest.fixture + def teardown_error(tmp_path): + (tmp_path / "evidence.txt").write_text("teardown", encoding='utf-8') + yield + raise RuntimeError("teardown failed") + + def test_setup(setup_error): + pass + + def test_call(tmp_path): + (tmp_path / "evidence.txt").write_text("call", encoding='utf-8') + assert False + + def test_teardown(teardown_error): + pass + """ + ) + + result = pytester.runpytest("--basetemp=retained") + result.assert_outcomes(passed=1, failed=1, errors=2) + + retained = pytester.path / "retained" + + assert (retained / "test_setup0" / "evidence.txt").exists() + assert (retained / "test_call0" / "evidence.txt").exists() + assert (retained / "test_teardown0" / "evidence.txt").exists() + def test_policy_failed_removes_basedir_when_all_passed( self, pytester: Pytester ) -> None: