Opened 7 years ago

Closed 7 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: Launchpad Bug:

Description

Attachments (3)

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

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by therve

  • Summary changed from test_logfile uselessly call shutil.rmtree on tearDown to test_logfile uselessly calls shutil.rmtree on tearDown

Changed 7 years ago by therve

comment:2 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

Please review!

comment:3 Changed 7 years ago by exarkun

  • 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 7 years ago by therve

Changed 7 years ago by therve

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

  • 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 7 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

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