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

[The-Compiler]

With #10442, tmp_path paths are not necessarily unique anymore - in other words, this passed previously, but fails now:

some_global_state = set()


def test_some_very_long_test_name_first(tmp_path):
    print(tmp_path)
    some_global_state.add(tmp_path)


def test_some_very_long_test_name_second(tmp_path):
    print(tmp_path)
    assert tmp_path not in some_global_state

This is because those tests used to use unique temp directory names:

x.py::test_some_very_long_test_name_first /tmp/pytest-of-florian/pytest-125/test_some_very_long_test_name_0
PASSED
x.py::test_some_very_long_test_name_second /tmp/pytest-of-florian/pytest-125/test_some_very_long_test_name_1
FAILED

but now don't do so anymore, instead, the second test reuses the path the first test did:

x.py::test_some_very_long_test_name_first /tmp/pytest-of-florian/pytest-125/test_some_very_long_test_name_0
PASSED
x.py::test_some_very_long_test_name_second /tmp/pytest-of-florian/pytest-125/test_some_very_long_test_name_0
FAILED

In my case, this caused issues with using Qt's QSqlDatabase, which has a list of connections as global state. Since with sqlite, a connection is based on its path, two tests used the same connection name.

I fixed it by closing the database connection properly after the test. This is definitely something I should have done before already.

However, this still violates a basic expectation on how tmp_path should behave, which is pointed out in the first sentence of the docs (emphasis mine):

You can use the tmp_path fixture which will provide a temporary directory unique to the test invocation

It's of course questionable whether the "unique" refers to the directory contents, or the path to it - but as shown above, assuming the latter isn't really something out of the ordinary, especially when passing that path to a library somewhere.

---

I've also run into another issue with the same commit, which I don't even know how to describe. The gist of it that I had a Qt QFileSystemModel watching the temporary directory for changes - and when it was deleted after the test, Qt/PyQt somehow tried to call a callback lambda on a Python object which was already deleted, thus segfaulting...

This one smells like a PyQt bug, but again demonstrates how files and file paths are inherently attached to some side effects, and deleting them after the test has finished might be too early, depending on what code in the background is doing with it. This also makes me a bit worried about #10546, which I haven't tried yet.

---

On a more personal note, I also wonder if it's really the right decision to change the default behavior (policy failed), even more so in a non-major release. As demonstrated above, it can have very subtle changes on a testsuite.

But, perhaps even more important than that, this makes something I consider a great feature of pytest less discoverable. In various places (books, but also my trainings for example) the current behavior of pytest is taught to people, and by the time they discover those files aren't actually kept around anymore, it might already be too late (if e.g. debugging a flaky test). It seems to me that this could lead to much more confusion and frustration compared to it being problematic when storing lots of big files there. The latter seems like a non-issue to me for most cases where I see tmp_path being used.

@yusuke-kadowaki @nicoddemus @RonnyPfannschmidt @hmaarrfk what do you think?

[RonnyPfannschmidt]

im stil of the oppinion that we shoudl always keep by default

as for the non-unique names, thats a but caused by using the numbered dirctory helper thats not aware of deletes

imho we should return to keeping it all and then revise the filename generator for per test folders

[hmaarrfk]

I am somewhat agnostic to the default value. I can see how that can create a lot of confusion and downstream problems.

I do think that the default should be for pytest to clean itself up after success. Eventually.maybe with a more substantial version change.

Have you tried to change the pytest behavior by setting up the option provided in the config file.

[hmaarrfk]

Have you tried to change the pytest behavior by setting up the option provided in the config file.

I ask this, since I believe that his option should continue to exist for those that depend on global state.

[RonnyPfannschmidt]

IMHO keeping it all is the safe default, we clean up deferred after all

We clearly demonstrated that the more eager cleanup trigger undesirable issues /details as its using helpers that are not prepared for the new on Filesystem behaviour

IMHO we should leave eager cleanup as opt in for data intensive testsuites

It my personal impression that most if not all testsuites I'm involved in have better wins by leaving around debug data rather than dropping data early

From my pov "Oops the debug data is gone" is a bit of a worse discovery than "I'm so data intensive, i have to opt into early dropping"

In addition we missed the accidental logic bomb from make_numbered_dir as before we never dropped early and the main concern was races with other pytest processes, not reuse of the same number in the same process

I kinda want to a extraction of the logic from pytest itself at some point

[nicoddemus]

Given all the information on the thread, I agree it is better to return to the old default. It has been working like that for many years and personally I also had no problems with it. The fact that this might cause headaches for many people once released is a real red flag: we can take the data point that just @The-Compiler already found two hard-to-track problems in his test suite alone. We should be conservative here and keep the old default and possible prevent a myriad of bug report / issues with users.

So IMHO we should do two things:

1. Change the default settings to mimic the old behavior.
2. Fix the issue that we broke the expectation of unique directories. While arguable if this is a feature or not, I would prefer to keep the previous behavior, otherwise people might get surprising test failures if the behavior changes between "keep" modes.

If nobody opposes it, we should create separate issues (linking to this one) and get volunteers to work on them, and close this.

[ykadowak]

So IMHO we should do two things:

1. Change the default settings to mimic the old behavior.
2. Fix the issue that we broke the expectation of unique directories. While arguable if this is a feature or not, I would prefer to keep the previous behavior, otherwise people might get surprising test failures if the behavior changes between "keep" modes.

Agreed on both.

Some comments:
For fixing the issue, we should probably stop removing any directory while test session is running. Saying this because just making it possible to distinguish the directories' name cannot avoid situations like the second issue that @The-Compiler found.

To do this, we would register this rmtree to atexit.

pytest/src/_pytest/tmpdir.py

Lines 287 to 290 in cab02e6

	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)

However, we should recognize that this has a drawback on testability like I explained here #10545. For example, all of these tests will have to be removed because it won't pass anymore.

pytest/testing/test_tmpdir.py

Lines 95 to 204 in cab02e6

	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():
	# 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())
	# Check the base dir itself is gone
	assert len(list(base_dir)) == 0
	# issue #10502
	def test_policy_failed_removes_dir_when_skipped_from_fixture(
	self, pytester: Pytester
	) -> None:
	p = pytester.makepyfile(
	"""
	import pytest
	@pytest.fixture
	def fixt(tmp_path):
	pytest.skip()
	def test_fixt(fixt):
	pass
	"""
	)
	pytester.inline_run(p)
	# 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
	# issue #10502
	def test_policy_all_keeps_dir_when_skipped_from_fixture(
	self, pytester: Pytester
	) -> None:
	p = pytester.makepyfile(
	"""
	import pytest
	@pytest.fixture
	def fixt(tmp_path):
	pytest.skip()
	def test_fixt(fixt):
	pass
	"""
	)
	pytester.makepyprojecttoml(
	"""
	[tool.pytest.ini_options]
	tmp_path_retention_policy = "all"
	"""
	)
	pytester.inline_run(p)
	# Check if the whole directory is kept
	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()
	)
	)
	assert len(test_dir) == 1

And this is where the PR(#10546) comes in. With the PR, we can register the rmtree to ExitStack and that'll make the deletion happens at the end of the session, which also probably solves the issue without losing testability.

Of course removing at the end of the session still has more side effects than removing at exit as @The-Compiler is worried. But at the same time, introducing these configurations without having those tests makes it difficult to maintain the feature.
Apparently both have pros and cons, but I'd say it's better to merge the PR and we test in real situations as much as possible to check the side effects before releasing if we think about maintaining the feature for a long time.

[RonnyPfannschmidt]

the remvoe moe should still remove folders, just no longer be the default

having a tool for unique folder names will be a separate task that would allow the test to have unique folder names even if preexisting ones where removed

[ykadowak]

the remvoe moe should still remove folders, just no longer be the default

Yeah it still deletes later, or is there any reason that it should be removed immediately that much?

My intension here is that test results should not differ depending on the configuration, so even if we change the default policy to all, we should still make sure that changing the policy to failed does not change the test results.
And as far as I understand from @The-Compiler's comment, removing folder early could result in a weird behavior that changes the test results. So I proposed to remove any folder at exit or hopefully at the end of the session.

[RonnyPfannschmidt]

@yusuke-kadowaki the problem is that the current numbering tool will reuse folder numbers in case they get deleted

the solution is to fix the numbering tool, not to defer the deletion

[ykadowak]

Are you saying that deletion needs to be deferred and also the tool needs to be changed?
If that's the case, I don't really see a point to change the tool since nothing gets deleted early so no chance to use the same folder name no?

[RonnyPfannschmidt]

No, I'm saying to return to the keep mechanism as default, but also fixing the numbering system to work correctly in case people opt into early delete

[ykadowak]

also fixing the numbering system to work correctly in case people opt into early delete

I'm not sure if this is an enough fix after looking at the issue below. IMHO, deferring deletion is universally safer.
Plus why would one want to delete folders early instead of deleting at the end of the session? Both will have the same result in terms of the data usage when you see it after the test session finishes.

I've also run into another issue with the same commit, which I don't even know how to describe. The gist of it that I had a Qt QFileSystemModel watching the temporary directory for changes - and when it was deleted after the test, Qt/PyQt somehow tried to call a callback lambda on a Python object which was already deleted, thus segfaulting...