[jira] [Commented] (HADOOP-13388) Clean up TestLocalFileSystemPermission

["Anu Engineer (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HADOOP-13388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15425375#comment-15425375 ] 

Anu Engineer commented on HADOOP-13388:
---------------------------------------

[~boky01] Thank you for the patch.

some minor comments/ nitpicks:
# import static org.junit.Assert.*; in most places I see you have removed "\*" imports
but this place you seem to have done the reverse. Just making sure that it is not your editor automatically doing this.
# Please feel free to ignore this comment. I am not sure that replacing assertTrue(!fs.exists(name)); with assertFalse(fs.exists(name)); adds to more readability. I agree it is one character less, but not sure if it makes code comprehension easier.
# I realize that you did not write this code. However the cleanup logic in testLocalFSsetPermission does not look correct to me. in the finally we are attempting to cleanup all file artifacts. However we have a set of  file creates happening outside the try, f,f1 and f2. it is possible that we might fail when creating f1, which case the cleanup of f will not happen.
I am presuming that you really do want to cleanup all file resources even if the test fails ?
# in testLocalFSsetOwner we still have LOGGER.error / return pattern. Just making sure that it is intentional.


> Clean up TestLocalFileSystemPermission
> --------------------------------------
>
>                 Key: HADOOP-13388
>                 URL: https://issues.apache.org/jira/browse/HADOOP-13388
>             Project: Hadoop Common
>          Issue Type: Bug
>          Components: fs
>            Reporter: Andras Bokor
>            Assignee: Andras Bokor
>            Priority: Trivial
>             Fix For: 3.0.0-alpha2
>
>         Attachments: HADOOP-13388.01.patch
>
>
> I see more problems with {{TestLocalFileSystemPermission}}:
> * Many checkstyle warnings
> * Relays on JUnit3 so Assume framework cannot be used for Windows checks.
> * In the tests in case of exception we get an error message but the test itself will pass (because of the return).



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org