-
Notifications
You must be signed in to change notification settings - Fork 187
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
Refactor: remove unnecessary use of tempfile.mkdtemp
#5639
Refactor: remove unnecessary use of tempfile.mkdtemp
#5639
Conversation
e4eb8a4
to
7f7449e
Compare
Tests seem to randomly fail. See #5640 |
08004c4
to
5c28412
Compare
5c28412
to
b99d426
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sphuber thanks, looks all good just one minor request.
@@ -325,7 +325,7 @@ def create_profile(self): | |||
self.create_aiida_db() | |||
|
|||
if not self.root_dir: | |||
self.root_dir = tempfile.mkdtemp() | |||
self.root_dir = tempfile.TemporaryDirectory().name # pylint: disable=consider-using-with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is not cleanup after use? Why not use it inwith
?
@@ -325,7 +325,7 @@ def create_profile(self): | |||
self.create_aiida_db() | |||
|
|||
if not self.root_dir: | |||
self.root_dir = tempfile.mkdtemp() | |||
self.root_dir = tempfile.TemporaryDirectory().name # pylint: disable=consider-using-with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.root_dir = tempfile.TemporaryDirectory().name # pylint: disable=consider-using-with | |
temp_dir = tempfile.TemporaryDirectory() | |
self.root_dir = temp_dir.name # pylint: disable=consider-using-with | |
temp_dir.cleanup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot use it here, because it needs to be kept alive for the duration of the tests. The cleanup is called manually in destroy_all
which is called at the end of the test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, found that. Thanks!
b99d426
to
c7ff0e4
Compare
When it is used for unit tests, it should be replaced by the `tmp_path` fixture provided by `pytest` since it will take care of cleanup automatically. Then there are a few cases in the code where it was used where it was replaced with `tempfile.TemporaryDirectory`. The latter has the advantage that it will be automatically cleanup when the object goes out of scope. A few places in the code still use `mkdtemp` since it needs to control the cleanup manually. The `temp_dir` fixture is also deprecated. It works exactly as the `tmp_path` fixture which ships with `pytest` and should be used instead.
c7ff0e4
to
d3fdd20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sphuber, I just search for tmp_dir
and find there are some in tests/cmdline/commands/test_node.py
that can use this fixture. I think probably better to change all in this PR. What do you think?
@@ -325,7 +325,7 @@ def create_profile(self): | |||
self.create_aiida_db() | |||
|
|||
if not self.root_dir: | |||
self.root_dir = tempfile.mkdtemp() | |||
self.root_dir = tempfile.TemporaryDirectory().name # pylint: disable=consider-using-with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, found that. Thanks!
Thanks @unkcpz I have searched for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! cheers.
Fixes #4336
When it is used for unit tests, it should be replaced by the
tmp_path
fixture provided bypytest
since it will take care of cleanup automatically.Then there are a few cases in the code where it was used where it was replaced with
tempfile.TemporaryDirectory
. The latter has the advantage that it will be automatically cleanup when the object goes out of scope.A few places in the code still use
mkdtemp
since it needs to control the cleanup manually.The
temp_dir
fixture is also deprecated. It works exactly as thetmp_path
fixture which ships withpytest
and should be used instead.