Opened 10 years ago

Closed 10 years ago

#3202 enhancement closed fixed (fixed)

twisted.test.test_process.UtilTestCase should restore the rights of files it creates

Reported by: therve Owned by:
Priority: low Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone Branch: branches/test_process-perms-3202
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

In particular, one of the "executable" file is read-only under windows, so that creates some problems. We can also delete the tree of files.

Attachments (2)

3202.diff (712 bytes) - added by therve 10 years ago.
3202_2.diff (607 bytes) - added by therve 10 years ago.

Download all attachments as: .zip

Change History (21)

Changed 10 years ago by therve

Attachment: 3202.diff added

comment:1 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

Please review.

comment:2 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Humm. I don't understand the motivation. Can you elaborate a bit?

comment:3 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

In particular, it prevents trial from cleaning _trial_temp. Running trial successively in the same directory leaves plenty of old _trial_temp (because the read-only file can't be removed without a user interaction). It also crash combinator mkbranch on my machine.

comment:4 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

trial seems to also use shutil.rmtree to get rid of old _trial_temp directories. I think I still don't understand, because it seems like the call to rmtree in test_process is just duplication of what trial is doing already.

comment:5 Changed 10 years ago by therve

Hum good point. I'll investigate further.

Changed 10 years ago by therve

Attachment: 3202_2.diff added

comment:6 Changed 10 years ago by therve

Keywords: review added
Owner: therve deleted

OK you were right, I don't know what I thought. The latest patch corrects the problem though.

comment:7 Changed 10 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve

Looks good. Please apply.

Maybe we can also think about teaching trial to be super awesome at deleting things so individual tests don't have to think about it as much.

comment:8 Changed 10 years ago by therve

Resolution: fixed
Status: newclosed

(In [23444]) Fix twisted.test.test_process.UtilTestCase so that it doesn't leave read-only files in _trial_temp anymore.

Author: therve Reviewer: exarkun Fixes #3202

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

Resolution: fixed
Status: closedreopened

(In [23447]) Revert r23444 - test suite regression on FreeBSD

Reopens: #3202

This changeset introduced three test failures on FreeBSD by way of an exception in the modified tearDown method:

===============================================================================
[ERROR]: twisted.test.test_process.UtilTestCase.testWhich

Traceback (most recent call last):
  File "/usr/home/therve/Run/slave/freebsd-py2.4-select/Twisted/twisted/test/test_process.py", line 1963, in tearDown
    os.chmod(os.path.join(self.bazbar, "executable"), 700)
exceptions.OSError: [Errno 79] Inappropriate file type or format: 'twisted.test.test_process/UtilTestCase/testWhich/lo3a1X/temp/baz/bar/executable'
===============================================================================
[ERROR]: twisted.test.test_process.UtilTestCase.testWhichPathExt

Traceback (most recent call last):
  File "/usr/home/therve/Run/slave/freebsd-py2.4-select/Twisted/twisted/test/test_process.py", line 1963, in tearDown
    os.chmod(os.path.join(self.bazbar, "executable"), 700)
exceptions.OSError: [Errno 79] Inappropriate file type or format: 'twisted.test.test_process/UtilTestCase/testWhichPathExt/VchWFn/temp/baz/bar/executable'
===============================================================================
[ERROR]: twisted.test.test_process.UtilTestCase.test_whichWithoutPATH

Traceback (most recent call last):
  File "/usr/home/therve/Run/slave/freebsd-py2.4-select/Twisted/twisted/test/test_process.py", line 1963, in tearDown
    os.chmod(os.path.join(self.bazbar, "executable"), 700)
exceptions.OSError: [Errno 79] Inappropriate file type or format: 'twisted.test.test_process/UtilTestCase/test_whichWithoutPATH/OHS-2j/temp/baz/bar/executable'
-------------------------------------------------------------------------------

comment:10 Changed 10 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/test_process-perms-3202

(In [23448]) Branching to 'test_process-perms-3202'

comment:11 Changed 10 years ago by Jean-Paul Calderone

Owner: changed from therve to Jean-Paul Calderone
Status: reopenednew

Sorry I am a bad reviewer. I fixed the intent of the original patch in the branch, but now I'm not sure I understand the intent again. :) If I had a Windows machine I might be able to figure it out, but since I don't, can you give me a hint?

comment:12 Changed 10 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added

comment:13 in reply to:  3 Changed 10 years ago by therve

I think I've explained it here:

In particular, it prevents trial from cleaning _trial_temp. Running trial successively in the same directory leaves plenty of old _trial_temp (because the read-only file can't be removed without a user interaction).

The failed function is shutil.rmtree in _setUpTestdir. This case was mainly obvious on windows.

comment:14 Changed 10 years ago by Jean-Paul Calderone

I'm probably not understanding something about Windows. Do you need write permission on a file to delete it on Windows? On Linux, you only need write permission on the directory it is in.

comment:15 Changed 10 years ago by therve

Yes, you need write permission to delete a file on Windows.

comment:16 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

Okay. Amazing. Now the change makes sense. I adjusted tearDown to use S_IWUSR (and not set the file readable or executable as neither of those seems to be necessary for deletion) instead of an octal since that's a bit easier to read. Otherwise the change is basically the same as the original.

comment:17 Changed 10 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Please merge.

comment:18 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [23467]) Merge test_process-perms-3202

Author: exarkun Reviewer: therve Fixes: #3202

Restore write permission on a temporary directory created by the tests so that files in it can be deleted by trial when the next test run begins.

comment:19 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.