Stop using atexit and add integration tests for tmpdir fixture#10546
Stop using atexit and add integration tests for tmpdir fixture#10546bluetech merged 1 commit intopytest-dev:mainfrom
atexit and add integration tests for tmpdir fixture#10546Conversation
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
it ts a good idea to make this boundary more clear 👍
lets try to go with existing tools from the stdlib and make sure we trigger all we need on errors as well
| # permissions, etc, in which case we ignore it. | ||
| rmtree(passed_dir, ignore_errors=True) | ||
|
|
||
| tmp_path_factory._lock_and_dir_removal_callback.execute() |
There was a problem hiding this comment.
i recall the atexit registration was done to ensure cleanup even when sessionfinish fails (rare, but can happen)
if we go with a exit-stack, we can still register its cleanup for atexit, but also enure we cleanly unwind it at session finish when possible
There was a problem hiding this comment.
i recall the atexit registration was done to ensure cleanup even when sessionfinish fails (rare, but can happen)
I wasn't aware of that situation. Now it's good I think.
58c5d50
RonnyPfannschmidt
left a comment
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
with the exitstack this looks good to me now 👍
lets register a followup to turn make_numbered_dir_with_cleanup into a actual context manager that one can push into the exitstack
but before we do more exit stacks, i think we want to take a look for pytest-core about where and how to use exit stacks / context managers in future/for code cleanup
| 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 |
There was a problem hiding this comment.
Removed this because it's now a duplicate of test_retention_count
I'm not really understanding the motivation of using context manager for it. |
|
I like the idea of removing reliance on atexit. I rebased the PR on current main. However, @RonnyPfannschmidt's first review mentioned that atexit was used as a fallback to remove the locks when even @RonnyPfannschmidt did you mean the code should also register an exit stack |
|
I recall that sometimes its necessary But registring the close for atexit is way cleaner than what we had |
…ixture Fix pytest-dev#10545 Co-authored-by: Ran Benita <ran@unusedvar.com>
|
Alright, added back a last-resort atexit, and made a few other tweaks to the PR. Can merge this now. |
3 participants
Closes #10545