Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup tests after run #65

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sneakers-the-rat
Copy link
Contributor

I was wondering why i was running out of space on my laptop, turns out that the tests were creating a ton of hatch environments that were taking up 9GB, and it seemed like every run was creating new environments.

So this PR just temporarily sets the HATCH_DATA_DIR to a tmpdir and cleans it up after the test run. pytest removes temporary directories itself, but it keeps them around for a bit, and since they are big i just manually removed them.

I added a --reuse-envs flag like nox in case someone wanted to keep those around for some reason. Note that these are not the envs created to run the tests in, but the envs that are created when running the tests for the generated packages.

Copy link
Contributor

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea hatch envs were so hungry. Thanks for discovering that.

Comment on lines 51 to 67

yield

shutil.rmtree(hatch_dir, ignore_errors=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be wrapped in a try, finally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can be if you'd like!

ignore_errors makes the onexc call here be just a pass, so it handles these errors: https://github.com/python/cpython/blob/43447cb63421fb4db5411c1e74bdd8a4cfcf9263/Lib/shutil.py#L670-740

but if you'd like to add a catchall here i don't see the harm in that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant

try:
  yield
finally:
  shutil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha, i have never seen that pattern before, i think pytest catches the error and then goes back and calls next() for all the fixtures, so any error that would happen in the test wouldn't stop the cleanup, but i think even if it did this would still be fine so np lemme try that out

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there it is :) 86b8c66

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have always used that pattern when testing with databases and rolling back transactions. I would really be interested to know if it's unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think if there was some unhandled exception within pytest itself that caused it to lose the reference to the called fixture object, but didn't completely crash the interpreter, that it would save you there? idk give it a shot, i think there's no reason not to do it, since the next thing after the yield is the finally anyway

@sneakers-the-rat
Copy link
Contributor Author

i think it ends up being a lot of space because when developing locally you run the tests a bunch of times as you make changes to the package, so the hashes (i'm assuming) of the pyproject keep changing, so i had like 30 envs for the package

sneakers-the-rat and others added 3 commits November 1, 2024 09:16
Co-authored-by: Moritz E. Beber <midnighter@posteo.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants