Opened 9 years ago

Closed 9 years ago

#3208 defect closed fixed (fixed)

test_logfile uselessly calls shutil.rmtree on tearDown

Reported by: therve Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

Attachments (3)

3208.diff (1.0 KB) - added by therve 9 years ago.
3208.2.diff (2.9 KB) - added by therve 9 years ago.
3208.3.diff (3.0 KB) - added by therve 9 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 9 years ago by therve

Summary: test_logfile uselessly call shutil.rmtree on tearDowntest_logfile uselessly calls shutil.rmtree on tearDown

Changed 9 years ago by therve

Attachment: 3208.diff added

comment:2 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

Please review!

comment:3 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

The try/finally in LogFileTestCase.testNoPermission seems to be unnecessary given LogFileTestCase.tearDown. What do you think? Can we get rid of that too?

Maybe the comment in LogFileTestCase.tearDown could be a docstring instead (and say for what it is necessary).

Basically great, though.

Changed 9 years ago by therve

Attachment: 3208.2.diff added

Changed 9 years ago by therve

Attachment: 3208.3.diff added

comment:4 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

I got rid of it, but added the chmod on self.dir conditionarly (I don't know if that could create a problem). Docstring done, too.

comment:5 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Great. Thanks. I only ran the tests on Linux. I don't see anything that should break on Windows, though. One other minor change that might be smart. If somehow we get to tearDown and self.path exists and self.dir is not executable, then os.path.exists will return False. Then we'll make the dir readable but the file will still be unreadable. If we fix the directory first, then this works better. So flip the order if you agree, then apply.

comment:6 Changed 9 years ago by therve

Resolution: fixed
Status: newclosed

(In [23430]) Remove useless shutil.rmtree in test_logfile. This removes some potential intermittent failures under Windows.

Author: therve Reviewer: exarkun Fixes #3208

comment:7 Changed 6 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.