twisted.test.test_paths.FilePathTestCase.test_getPermissions_Windows creates undeleteable file

[Jean-Paul Calderone]

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.

[Indradhanush Gupta]

Changed the file permission to 777 in lieu of the sanity check.

[Jean-Paul Calderone]

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.

[Indradhanush Gupta]

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.

[Max Haarhaus]

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.

[Max Haarhaus]

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.

[Tom Prince]

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.

[Max Haarhaus]

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.

[Max Haarhaus]

Change the file permissions during cleanup to allow for deletion.

[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.

[Max Haarhaus]

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.

[Max Haarhaus]

Include the news file.

[Itamar Turner-Trauring]

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.

[Itamar Turner-Trauring]

Oh, and the change is minor enough the news file could just be a .misc with no content.

[Max Haarhaus]

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.

[Jean-Paul Calderone]

Thanks! This looks like the right fix. I'll apply it to trunk.

[Jean-Paul Calderone]

(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.