Opened 11 years ago
Closed 9 years ago
#5228 defect closed fixed (fixed)
twisted.test.test_paths.FilePathTestCase.test_getPermissions_Windows creates undeleteable file
Reported by: | Jean-Paul Calderone | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | high | Milestone: | |
Component: | core | Keywords: | easy |
Cc: | Branch: | ||
Author: |
Description
This test method sets the mode of sub1 such that it cannot be deleted in the usual way:
[Error 5] Access is denied: '_trial_temp
twisted.test.test_paths
FilePathTestCase
test_getPermissions_Windows
bvk9lu
temp
sub1'
The test should ensure that regardless of the test outcome, this file ends up deletable, or it should delete it itself.
Attachments (5)
Change History (20)
comment:1 Changed 10 years ago by
Owner: | set to gdeng |
---|
Changed 9 years ago by
Attachment: | changed_file_permission_to_777.patch added |
---|
comment:2 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | changed from gdeng to Jean-Paul Calderone |
comment:3 follow-up: 4 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to Indradhanush Gupta |
Thanks for working on this dhanush. I'm not sure why this is assigned to me, though. The general procedure is to assign tickets for review to noone (the empty entry at the top of the "assign to" dropdown). I filed this ticket, but that doesn't mean I have to review it.
However, since I'm here and the patch is very short, I guess I will.
My only comment on the patch is that it doesn't seem to address the issue. It changes an assertion being made in the test method. This doesn't seem correct to me, the assertion being made before was correct, so the new assertion after this patch is probably incorrect. The tests only runs on Windows so I can't easily run it, but I suppose that if it were run, it would fail.
Beyond that, the problem described in the ticket description still exists. The file is still created with permissions that make it undeleteable.
comment:4 Changed 9 years ago by
Thanks for your feedback. I will stick to the general procedure for assigning tickets to review. Regarding the patch, as the issue still stands, I'll try to take a plunge in more detail into the code and try to fix this up in a future patch.
Changed 9 years ago by
Attachment: | create_deleteable_files.patch added |
---|
Change the final chmod command in test_getPermissions_Windows to allow sub1 to be deleted, while maintaining the user permissions = group permissions = other permissions test.
comment:5 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Indradhanush Gupta deleted |
Change the final chmod command in test_getPermissions_Windows to allow sub1 to be deleted, while maintaining the user permissions = group permissions = other permissions test.
comment:6 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Max Haarhaus |
The change looks good, but it could probably be better. If the assertion triggers the second time through the loop, it will still leave an undeletable file.
TestCase.addCleanup could be used to avoid this. It would also be useful to add a comment as to why it needs to be done, so future maintainers don't break it again.
comment:7 Changed 9 years ago by
Maybe I'm looking at it wrong, but isn't the final chmod (to 511, making it undeleteable) occur outside of the loop? The mode is changed to 777 and tested then 555 and tested inside the loop, then with my change, 711 and tested outside.
Regardless, I'll look into doing it with addCleanup instead.
Changed 9 years ago by
Attachment: | change_permissions_after_test.patch added |
---|
Change the file permissions during cleanup to allow for deletion.
comment:8 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Max Haarhaus deleted |
comment:9 follow-up: 10 Changed 9 years ago by
Maybe I'm looking at it wrong, but isn't the final chmod (to 511, making it undeleteable) occur outside of the loop? The mode is changed to 777 and tested then 555 and tested inside the loop, then with my change, 711 and tested outside.
If none of the assertions fail, then that is the case. But if one the assertions fails, then the code will interrupted, and the file could be left in an undeletable state.
comment:10 Changed 9 years ago by
Replying to tom.prince:
Maybe I'm looking at it wrong, but isn't the final chmod (to 511, making it undeleteable) occur outside of the loop? The mode is changed to 777 and tested then 555 and tested inside the loop, then with my change, 711 and tested outside.
If none of the assertions fail, then that is the case. But if one the assertions fails, then the code will interrupted, and the file could be left in an undeletable state.
Okay, I understand. The new patch uses addCleanup, so it should avoid that issue.
Changed 9 years ago by
Attachment: | change_permissions_after_test2.patch added |
---|
Include the news file.
comment:11 follow-up: 13 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Max Haarhaus |
The problem with doing the cleanup registration at the end of the function is that if one of the assertion fails earlier one, or the test is interrupted, it will never be reached. So cleanup will still not happen if an assertion fails early on.
comment:12 Changed 9 years ago by
Oh, and the change is minor enough the news file could just be a .misc with no content.
Changed 9 years ago by
Attachment: | 5228.patch added |
---|
comment:13 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Max Haarhaus deleted |
Replying to itamar:
The problem with doing the cleanup registration at the end of the function is that if one of the assertion fails earlier one, or the test is interrupted, it will never be reached. So cleanup will still not happen if an assertion fails early on.
Yeah, silly mistake. Fixed that and changed the news file.
comment:14 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
Thanks! This looks like the right fix. I'll apply it to trunk.
comment:15 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(In [38294]) Clean up test state in the FilePath
tests on Windows so that _trial_temp
can be deleted
Author: mhaarhaus Reviewer: tom.prince, itamar, exarkun Fixes: #5228
Previously a FilePath
test left some files in _trial_temp
without permissions
necessary to delete them. This made clean up _trial_temp a pain. The test now
makes sure to restore permissions on the dummy files after the test is over.
Changed the file permission to 777 in lieu of the sanity check.