Conversation
|
|
||
| def test_x_to_x_permissions(self): | ||
| os.chmod(FILE_PATH, stat.S_IXUSR) | ||
| if platform.system() != 'Windows': |
There was a problem hiding this comment.
Mention the reason wherever you have done this.
There was a problem hiding this comment.
No. Get rid of it. If this bear doesnt support Windows, that fact shouldnt be hidden inside the logic of the tests.
A .coafile is a cross-platform specification, and it must not cause failures on only one platform.
There was a problem hiding this comment.
Perhaps create a separate test class for executable permissions, and the class should be skipped on Windows for tests which dont work on Windows, and potentially we need a second extra test class which does Windows specific testing to show that specifying executable bits in the .coafile results in a mild note in the coala log, but does not cause it to fail.
bkhanale
force-pushed
the
filemodebear
branch
2 times, most recently
from
70a08d9 to
b9b0d97
Compare
abhishalya
left a comment
abhishalya
left a comment
There was a problem hiding this comment.
Few improvements, other things look good.
bkhanale
force-pushed
the
filemodebear
branch
2 times, most recently
from
84ec0de to
1a12c51
Compare
|
Current codecov failure at 99.91% can be ignored - https://travis-ci.org/coala/coala-bears/jobs/561920971 shows the new bear is getting 100% test coverage |
|
|
||
| def test_x_to_x_permissions(self): | ||
| os.chmod(FILE_PATH, stat.S_IXUSR) | ||
| if platform.system() != 'Windows': |
There was a problem hiding this comment.
Perhaps create a separate test class for executable permissions, and the class should be skipped on Windows for tests which dont work on Windows, and potentially we need a second extra test class which does Windows specific testing to show that specifying executable bits in the .coafile results in a mild note in the coala log, but does not cause it to fail.
|
It should be possible to get executable bit working correctly on Windows, and this PR cant close the issue without executable support, as executable support is the primary motivation for this bear, and the need to include Windows was a consistent message throughout the comments of the issue. |
|
@jayvdb The bear works fine for Windows (as it only reads the file permissions and doesn't change them), the problem was I couldn't test it properly on Windows since Python can only set read-only flags on it. One way to test this could be to rename the file to the extension |
|
|
||
|
|
||
| FILE_PATH = os.path.join(os.path.dirname(__file__), | ||
| 'filemode_test_files', 'test_file.txt') |
There was a problem hiding this comment.
the issue asked for "...on scripts" . Why are you using .txt ?
There was a problem hiding this comment.
I guess having no extension at all would've been better. This is because the bear is supposed to check for all file permissions as mentioned here and is not just restricted scripts.
There was a problem hiding this comment.
Start with the realistic scenario of scripts having .sh / .bash etc extensions and make sure you can get that working and tested properly, before trying extensionless voodoo.
If you can not test it, you have no way to determine that is works, and you should not attempt to argue that it does work. I can see a mile away it does not achieve what the issue requested. No you do not get to create a useless test by renaming files to .exe . That isnt what the issue asked for. And even ignoring the fact the bear doesnt work, the test method os.chmod(FILE_PATH, stat.S_IXUSR)
os.chmod(FILE_PATH, stat.S_IRUSR)That is not a test of the bear, it it. That is wrong. Fix it. https://github.com/coala/coala-bears/pull/2912/files#r294035442 asked you two months ago to document why you were doing that. You havent done that. Why not? Why are you using custom test logic instead of |
I guess that he did to roll back changes made to the file permissions (which I guess defaults to read permission).
Yep, you can use |
3 participants
Closes #2370
For short term contributors: we understand that getting your commits well
defined like we require is a hard task and takes some learning. If you
look to help without wanting to contribute long term there's no need
for you to learn this. Just drop us a message and we'll take care of brushing
up your stuff for merge!
Checklist
them.
individually. It is not sufficient to have "fixup commits" on your PR,
our bot will still report the issues for the previous commit.) You will
likely receive a lot of bot comments and build failures if coala does not
pass on every single commit!
After you submit your pull request, DO NOT click the 'Update Branch' button.
When asked for a rebase, consult coala.io/rebase
instead.
Please consider helping us by reviewing other peoples pull requests as well:
corobo mark wip <URL>to get it outof the review queue.
The more you review, the more your score will grow at coala.io and we will
review your PRs faster!