Opened 6 years ago

Closed 6 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: exarkun Branch: branches/test_process-perms-3202
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

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 6 years ago.
3202_2.diff (607 bytes) - added by therve 6 years ago.

Download all attachments as: .zip

Change History (21)

Changed 6 years ago by therve

comment:1 Changed 6 years ago by therve

  • Keywords review added
  • Owner therve deleted

Please review.

comment:2 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

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

comment:3 follow-up: Changed 6 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 6 years ago by exarkun

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

Hum good point. I'll investigate further.

Changed 6 years ago by therve

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

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

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

(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 6 years ago by exarkun

  • Resolution fixed deleted
  • Status changed from closed to reopened

(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 6 years ago by exarkun

  • author set to exarkun
  • Branch set to branches/test_process-perms-3202

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

comment:11 Changed 6 years ago by exarkun

  • Owner changed from therve to exarkun
  • Status changed from reopened to new

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 6 years ago by exarkun

  • Cc exarkun added

comment:13 in reply to: ↑ 3 Changed 6 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 6 years ago by exarkun

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

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

comment:16 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun 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 6 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Please merge.

comment:18 Changed 6 years ago by exarkun

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

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

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