Tmpdir stash key deletion question

[str0zzapreti]

Q: What is the reason for deleting the stashed result dict from each request in the tmp_path fixture from _pytest/tmpdir.py?

Context: The tmp_path fixture uses the following code to determine whether to run rmtree() during teardown:

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

    del request.node.stash[tmppath_result_key]

This result dict gets added during the runtest_makereport stage. Deleting entries from the stash doesn't seem to be particularly common and isn't directly required for the file cleanup here, so I'm curious if there's a subtler reason I'm not understanding. Thanks!

[str0zzapreti]

For reference, it looks like the stash check was added to fix a crash with the fixture in this PR: #10517

[The-Compiler]

Probably just to avoid keeping around data with every test that isn't needed anywhere anymore? Maybe @ykadowak who originally implemented this can say more.

[str0zzapreti]

Thanks, that'd be great! I've been maintaining a test retry plugin for Pytest that runs into an exception with the tmpdir fixture teardown and removing the stash key deletion line would fix it (but I was curious if there would be side effects). I could provide an alternative fixture or insert a dummy dict with the stash key if needed, but if the only purpose to that line is to avoid keeping data around, is there any chance it could be removed?

[RonnyPfannschmidt]

Currently tmpdir paths may be reused

[ykadowak]

Disclaimer: Honestly, I've been away from Python for a pretty long time, so my memory is quite vague...

Probably just to avoid keeping around data with every test that isn't needed anywhere anymore

As far as I can remember, the reason is just this.

So it might be okay to remove this line. However, there's some concerns:

1. This area has only a few test cases by nature, so we cannot fully trust the tests when changing the implementation.
2. I'm not sure if it's a good idea for the design/implementation to depend on this dict in the first place as this was introduced only for implementing this feature and not intended to be used by others (at least at that time).

[str0zzapreti]

Thanks @ykadowak, appreciate you coming back to coming back to take a look at this. To be clear, I'm not trying to use the dict myself, the current implementation just means that during the initial teardown stage, the dict is deleted and so the tmpdir fixture fails when trying to access it during retries. Since the dict is added to items during the report stage (where the main portion of my plugin runs), the value is never refreshed for subsequent runs of the fixture. The fixture is just essentially getting in its own way.

I don't wish to rock the boat here so if this feature hasn't been reasoned about recently and possesses limited test coverage, I'm inclined to just proceed with a workaround and/or disclaimer on my end. There's always pytest-rerunfailures, which doesn't exhibit this issue, as an alternative.