Opened 10 years ago

Closed 10 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 10 years ago.
3208.2.diff (2.9 KB) - added by therve 10 years ago.
3208.3.diff (3.0 KB) - added by therve 10 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 10 years ago by therve

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

Changed 10 years ago by therve

Attachment: 3208.diff added

comment:2 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

Please review!

comment:3 Changed 10 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 10 years ago by therve

Attachment: 3208.2.diff added

Changed 10 years ago by therve

Attachment: 3208.3.diff added

comment:4 Changed 10 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 10 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 10 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 7 years ago by <automation>

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