tahoe-lafs/trac-2024-07-25

consistently use self.mktemp() to create a temporary directory in tests #2432

New issue

Open

opened 2015-05-26 15:17:27 +00:00 by daira · 9 comments

daira commented

2015-05-26 15:17:27 +00:00

Owner

Currently we use a hotchpotch of methods to create a temporary directory for a test -- specifying the name manually, using a workdir method, using self.mktemp() from twisted.trial.unittest.TestCase, etc. (It is usually consistent within a test module but not across modules.)

We should just use self.mktemp(), or if there is something wrong with that then we should consistently use our own mixin.

Currently we use a hotchpotch of methods to create a temporary directory for a test -- specifying the name manually, using a `workdir` method, using `self.mktemp()` from `twisted.trial.unittest.TestCase`, etc. (It is usually consistent within a test module but not across modules.) We should just use `self.mktemp()`, or if there is something wrong with that then we should consistently use our own mixin.

tahoe-lafs added the

labels 2015-05-26 15:17:27 +00:00

tahoe-lafs added this to the 1.11.0 milestone 2015-05-26 15:17:27 +00:00

warner commented

2015-05-26 16:01:59 +00:00

Author

Owner

I agree it's a bit of a mess. My only two concerns:

cleaning up the directory when we're done, so we don't litter /tmp/ (or wherever) with leftovers
being able to find the leftover state when something goes wrong

Trial puts everything in ./_trial_temp, and rmtrees it at the beginning of each test run. This provides a predictable location, and an upper bound (of size=1) on the litter.

So I guess I'm in favor of using our own mixin that uses the (fully-qualified) name of the test case to produce a subdirectory of _trial_temp.

I agree it's a bit of a mess. My only two concerns: * cleaning up the directory when we're done, so we don't litter /tmp/ (or wherever) with leftovers * being able to find the leftover state when something goes wrong Trial puts everything in `./_trial_temp`, and `rmtree`s it at the beginning of each test run. This provides a predictable location, and an upper bound (of size=1) on the litter. So I guess I'm in favor of using our own mixin that uses the (fully-qualified) name of the test case to produce a subdirectory of `_trial_temp`.

daira commented

2015-05-26 18:02:17 +00:00

Author

Owner

Replying to warner:

So I guess I'm in favor of using our own mixin that uses the (fully-qualified) name of the test case to produce a subdirectory of _trial_temp.

mktemp() from twisted.trial.unittest.TestCase already does that.

Replying to [warner](/tahoe-lafs/trac-2024-07-25/issues/2432#issuecomment-138780): > So I guess I'm in favor of using our own mixin that uses the (fully-qualified) name of the test case to produce a subdirectory of `_trial_temp`. `mktemp()` from `twisted.trial.unittest.TestCase` already does that.

daira commented

2015-05-26 18:15:13 +00:00

Author

Owner

The one thing I don't like about mktemp() is that the path it generates is not deterministic, due to its use of mkdtemp (implementation in Twisted v15.2.0). What I actually want is os.path.dirname(os.path.dirname(self.mktemp()), I think. Maybe it would be better to have a mixin to shorten that, so that if we want to change it, we only need to do so in one place.

The one thing I don't like about `mktemp()` is that the path it generates is not deterministic, due to its use of `mkdtemp` ([implementation in Twisted v15.2.0](https://twistedmatrix.com/trac/browser/tags/releases/twisted-15.2.0/twisted/trial/_synctest.py#L1221)). What I actually want is `os.path.dirname(os.path.dirname(self.mktemp())`, I think. Maybe it would be better to have a mixin to shorten that, so that if we want to change it, we only need to do so in one place.

daira commented

2015-05-26 18:18:02 +00:00

Author

Owner

Also, I think I'd prefer for our mixin method to return an absolute Unicode path, rather than a relative str path.

Also, I think I'd prefer for our mixin method to return an absolute Unicode path, rather than a relative `str` path.

warner commented

2016-03-22 05:02:52 +00:00

Author

Owner

Milestone renamed

tahoe-lafs modified the milestone from 1.11.0 to 1.12.0