Adds a feature to control how directories created by the `tmp_path` fixture are kept. by ykadowak · Pull Request #10442 · pytest-dev/pytest

ykadowak · 2022-10-27T13:16:54Z

I recognize that the tests are insufficient but I couldn't find a good reference. Is there any integration tests that actually make sure only 3 directories remain after executing a test that uses tmp_dir fixture for multiple times? I assume no and it's probably because it's difficult to deal with atexit.register?

nicoddemus

Thanks @yusuke-kadowaki for the contribution.

We also need to update the tmpdir.rst docs with the new options, and mention the configuration variables (as I requested them to be changed from command-line options to conf variables) in reference.rst.

nicoddemus · 2022-10-28T12:23:45Z

        ),
    )

+    group.addoption(


Let's make those configuration variables instead, seems like something users will configure once instead of passing them in the command-line all the time.

Done.
I was confused with command line options and configuration options.

nicoddemus · 2022-10-28T12:26:25Z

    _trace = attr.ib()
    _basetemp = attr.ib(type=Optional[Path])
+    _retention_count = attr.ib(type=int)
+    _retention_policy = attr.ib(type=str)


Suggested change

_retention_policy = attr.ib(type=str)

_retention_policy = attr.ib(type=Literal["none", "failed", "all"])

We should probably make a TypeAlias for that and reuse it everywhere it is declared.

I made this.

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

nicoddemus · 2022-10-28T12:28:15Z

+                rmtree(passed_dir)
+
+        # Register to remove the base directory before starting cleanup_numbered_dir
+        atexit.register(cleanup_passed_dir, tmp_path_factory._basetemp)


I would prefer to explicitly remove them at this point instead of relying on atexit.

pytest might be embedded, for example as part of user script or IDE, which might not actually exit the interpreter for a awhile.

Done.
Is there any reason for using atexit to execute cleanup_numbered_dir in the first place? That makes it really difficult to test the behavior.

nicoddemus · 2022-10-28T12:30:05Z

@@ -0,0 +1 @@
+Added `--tmp-path-retention-count` and `--tmp-path-retention-policy` options to control how directories created by the `tmp_path` fixture are kept.


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

Done
ce71d7b

nicoddemus · 2022-10-28T12:31:51Z

+    tmp_path_factory: TempPathFactory = session.config._tmp_path_factory
+    if tmp_path_factory._basetemp is None:
+        return
+    policy = tmp_path_factory._retention_policy


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?

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

ykadowak · 2022-10-30T12:08:29Z

Only the Windows tests are failing but I'm not familiar with Windows so I need some help.
Seems like monkeypatch holds the tmpdir when we try to rmtree it (and it only happens in Windows 🤔?).

My first guess is we need to stick to atexit but again I'm noob on Windows so I want someone else's opinion.

nicoddemus

Thanks @yusuke-kadowaki!

Left a few comments.

@RonnyPfannschmidt would also appreciate your review given your familiarity with this code.

nicoddemus · 2022-10-30T12:48:48Z

@@ -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.


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.

Done with c7eb5a3.

nicoddemus · 2022-10-30T12:50:19Z

@@ -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.
+The default behavior has changed to remove all directories if all tests passed.


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"`.

Done with c7eb5a3.

nicoddemus · 2022-10-30T12:56:46Z

+    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)


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)

Done with 94740c0.

nicoddemus · 2022-10-30T12:57:52Z

+def pytest_runtest_makereport(item, call):
+    outcome = yield
+    result = outcome.get_result()
+    setattr(item, "result_" + result.when, result)


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)

Done with dc11283.

nicoddemus · 2022-10-30T12:59:48Z

+        # Register to remove the base directory before starting cleanup_numbered_dir
+        passed_dir = tmp_path_factory._basetemp
+        if passed_dir.exists():
+            rmtree(passed_dir)


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)

Done with 94740c0.

nicoddemus · 2022-10-30T13:00:04Z

+        elif name == "tmp_path_retention_policy":
+            return "failed"
+        else:
+            assert False


nicoddemus · 2022-11-02T12:33:49Z

+    basetemp = tmp_path_factory._basetemp
+    if basetemp is None:
+        return
+    for left_dir in basetemp.iterdir():


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

Done with b04bbb1.

nicoddemus · 2022-11-02T12:42:21Z

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


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())

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

ykadowak · 2022-11-03T12:14:18Z

Sorry I accidentally removed @RonnyPfannschmidt from reviewers and cannot get it back.

nicoddemus · 2022-11-03T12:30:53Z

Thanks @yusuke-kadowaki, great work!

ykadowak · 2022-11-07T12:56:24Z

@RonnyPfannschmidt
Could you take a look at this please?

ykadowak · 2022-11-15T11:33:14Z

@nicoddemus
What should we do with this?

nicoddemus · 2022-11-15T12:09:20Z

Let's go ahead and merge this, @RonnyPfannschmidt can always take a look later and we can do a follow up if needed. 👍

nicoddemus · 2022-11-15T12:11:45Z

Thanks @yusuke-kadowaki!

RonnyPfannschmidt · 2022-11-15T14:15:43Z

thanks for landing this, this pr is good, i just dont have bandwidth for deep reviews for family reasons

ykadowak · 2022-11-15T14:22:07Z

Thank you for the reviews and also maintaining this great library!
Just lemme know when something needs to be updated. I'm here to work on it😉

The-Compiler · 2022-11-16T08:05:56Z

+def pytest_runtest_makereport(item, call):
+    outcome = yield
+    result = outcome.get_result()
+    setattr(item, "_tmp_path_result_" + result.when, result)


Those aren't guaranteed to exist if a phase wasn't run, see #10502.

But also, didn't we introduce the .stash stuff to avoid having to monkeypatch random attributes on items? It seems like we should use that here, no? Search for e.g. caplog_records_key in the codebase to see how this is done elsewhere, including storing things based on when.

@The-Compiler @nicoddemus

Made a fix for this #10517.

But also, didn't we introduce the .stash stuff to avoid having to monkeypatch random attributes on items? It seems like we should use that here, no?

Thanks for catching this @The-Compiler, completely missed that on my review. 👍

…ept (pytest-dev#10442) Close pytest-dev#8141

ykadowak added 13 commits October 23, 2022 01:03

Add retention cli options frontend

3043677

Fix type of tmp_path_retention_count option

92eb443

Add member vars of retention options to TempPathFactory class

7f80cc1

Apply retention_count as keep number

6bc424c

Fix option type

e9ee831

Implement retention policy none

1a358ee

Fix test for new options

5c9eeff

Introduce sessionfinish to cleanup

58ddc6d

Add cleanup dead symlink

ebf8761

Add test for retention count == 0

abf935b

Add changelog

dc1b419

Update AUTHORS

eab8b70

Refactor sessionfinish

321eeae

ykadowak changed the title ~~Tmp dir configure~~ Adds feature to control how directories created by the tmp_path fixture are kept. Oct 27, 2022

ykadowak changed the title ~~Adds feature to control how directories created by the tmp_path fixture are kept.~~ Adds a feature to control how directories created by the tmp_path fixture are kept. Oct 27, 2022

nicoddemus reviewed Oct 28, 2022

View reviewed changes

ykadowak added 6 commits October 30, 2022 00:53

Change options from cli to config

0eb8723

Fix Literal for older version

557c7ce

Change to remove all the testcase dir when policy is failed

0abbf8c

Add tests for policy failed

caeb50f

Update document

ce71d7b

Fix the way to import hookimpl

a25f8d5

ykadowak requested a review from nicoddemus October 30, 2022 08:20

nicoddemus requested changes Nov 2, 2022

View reviewed changes

nicoddemus requested a review from RonnyPfannschmidt November 2, 2022 12:43

ykadowak added 4 commits November 3, 2022 20:18

Update document

c7eb5a3

Add ignore_errors=True to rmtree

94740c0

Fix the name of the attribute

dc11283

Refactor the process of deleting dead symlink

b04bbb1

Add comments on why ignoring symlinks in some tests

f853117

ykadowak requested review from nicoddemus and removed request for RonnyPfannschmidt November 3, 2022 12:11

nicoddemus requested a review from RonnyPfannschmidt November 3, 2022 12:26

nicoddemus approved these changes Nov 3, 2022

View reviewed changes

nicoddemus merged commit cca029d into pytest-dev:main Nov 15, 2022

The-Compiler mentioned this pull request Nov 16, 2022

tmp_path_retention_policy crash when skipping from fixture #10502

Closed

The-Compiler reviewed Nov 16, 2022

View reviewed changes

ykadowak deleted the tmp_dir_configure branch November 23, 2022 12:12

This was referenced Nov 26, 2022

Modify documentation to use .stash when storing test results. #10535

Merged

Documentation update for the tmp_path configurations #10549

Merged

The-Compiler mentioned this pull request Dec 6, 2022

Deleting temporary directories early causes non-unique paths and other issues #10564

Open

raubitsj mentioned this pull request Dec 20, 2022

chore(sdk): use pytest tmp_path so we can inspect failures wandb/wandb#4664

Merged

2 tasks

soxofaan mentioned this pull request Mar 22, 2023

Configurability of temp directory cleanup #8410

Closed

The-Compiler mentioned this pull request Apr 11, 2023

Major pytest 7.3.0 performance regression on Windows #10896

Closed

spencerkclark mentioned this pull request Aug 28, 2023

Upgrade pytest in nix environment to at least version 7.3.0 ai2cm/fv3gfs-fortran#380

Open

imgbot Bot pushed a commit to kkipngenokoech/pytest that referenced this pull request Apr 18, 2026

Add configuration options to control how tmp_path directories are k…

6a88888

…ept (pytest-dev#10442) Close pytest-dev#8141

imgbot Bot pushed a commit to kkipngenokoech/pytest that referenced this pull request Apr 18, 2026

Add configuration options to control how tmp_path directories are k…

0b1f094

…ept (pytest-dev#10442) Close pytest-dev#8141

	_retention_policy = attr.ib(type=str)
	_retention_policy = attr.ib(type=Literal["none", "failed", "all"])

		@@ -0,0 +1 @@
		Added `--tmp-path-retention-count` and `--tmp-path-retention-policy` options to control how directories created by the `tmp_path` fixture are kept.

		@@ -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.

	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.

		@@ -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.
		The default behavior has changed to remove all directories if all tests passed.

	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"`.

-        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)

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

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

Conversation

ykadowak commented Oct 27, 2022

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykadowak commented Oct 30, 2022

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykadowak Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykadowak commented Nov 3, 2022

Uh oh!

nicoddemus commented Nov 3, 2022

Uh oh!

ykadowak commented Nov 7, 2022

Uh oh!

ykadowak commented Nov 15, 2022

Uh oh!

nicoddemus commented Nov 15, 2022

Uh oh!

nicoddemus commented Nov 15, 2022

Uh oh!

RonnyPfannschmidt commented Nov 15, 2022

Uh oh!

ykadowak commented Nov 15, 2022

Uh oh!

Choose a reason for hiding this comment

ykadowak Nov 3, 2022 •

edited

Loading