[
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: [email protected]
For additional commands, e-mail: [email protected]
