Description
FileContextURI unit tests.
Attachments
Attachments
- HADOOP-6261.patch
- 21 kB
- Ravi Phulari
- HADOOP-6261.patch
- 21 kB
- Ravi Phulari
- HADOOP-6261.patch
- 21 kB
- Ravi Phulari
Issue Links
- is blocked by
-
HADOOP-4952 Improved files system interface for the application writer.
- Closed
- is related to
-
HADOOP-6274 TestLocalFSFileContextMainOperations tests wrongly expect a certain order to be returned.
- Closed
Activity
A couple of issues:
- indentation: it supposes to be 2 spaces, not 4
- what is the point of making the parent class abstract. If you want to emphasize the fact that some of the methods are going to be overridden you can {{@Override} annotation where it's needed
- I'd suggest to change the name of FileContextURIBaseTest to FileContextURIBase because the first reaction is to suggest to rename the class into Test... so it'd be picked up by a hardness
- do you want to make this a @BeforeClass method? Otherwise it kinda stands there along
{ for (int i = 0; i < data.length; i++) { data[i] = (byte) (i % 10); } }
- I think createFile has been written line 137 times already - I think you don't need an extra one
- in qualifiedPath(String pathString, FileContext fc you, perhaps, should avoid using a full type name as a part of the var. name. You might want to use hungarian notation, e.g. prefix a String variable with 's', etc. Also, what if fc would be equals to null?
- generally, you might want to make a method's parameters final to suggest that there's no side-effects involved. And it's more secure code, BTW
- it isn't necessary to prefix your test methods with word 'test' since you have the annotation in place. Although, there's no single rule about this
- tearDown takes care about cleaning of fs2. Do you need to do the same for fs1?
- there's a number of places in the code where you have things like
String fileName = "testFile"; Path testPath = qualifiedPath(fileName, fc2);
and which is an only one usage of fileName variable. It's better be declared final in this case or simply use the literal constant in the call directly.
- you might want to add some sort of messages to ease assertion identifications. Simply havinf assertTrue(op1, op2) doesn't mean much when a fault is happening and one needs to read the source code to understand the cause of it (e.g. spend time on an unnecessary analysis)
- public methods are suppose to have some Javadoc for them. Besides, it is great to have some sort of documentation or comments about that a test does. This way it is easier to control if specs were changed and/or maintain the test in a long run.
- in the testListStatus you are likely to get an assertion in the following line
Assert .assertEquals(qualifiedPath("test/hadoop/c", fc1), paths[2].getPath());
because the string literal is different from what has been defined by testDirs
- also I'd suggest to change this last method to something like
public void testListStatus() throws Exception { final String hPrefix = "test/hadoop"; final String [] dirs = { hPrefix + "/a", hPrefix + "/b", hPrefix + "/c" }; ArrayList<Path> testDirs = new ArrayList<Path>(); for (String d : dirs) { testDirs.add(qualifiedPath(d, fc2)); } Assert.assertFalse(fc1.exists(testDirs.get(0))); for (Path path : testDirs) { Assert.assertTrue(fc1.mkdirs(path, FsPermission.getDefault())); } FileStatus[] paths = fc1.listStatus(qualifiedPath("test", fc1)); Assert.assertEquals(1, paths.length); Assert.assertEquals(qualifiedPath(hPrefix, fc1), paths[0].getPath()); paths = fc1.listStatus(qualifiedPath(hPrefix, fc1)); Assert.assertEquals(3, paths.length); for (int i=0; i<paths.length; i++) { Assert .assertEquals(i + " path element is wrong", qualifiedPath(dirs[i], fc1), paths[i].getPath()); } paths = fc1.listStatus(qualifiedPath(dirs[0], fc1)); Assert.assertEquals(0, paths.length); }
this way you have less code duplication
in qualifiedPath(String pathString, FileContext fc you, perhaps, should avoid using a full type name as a part of the var. name. You might want to use hungarian notation, e.g. prefix a String variable with 's', etc.
Let's avoid Hungarian notation, shall we? Just dropping the type from the variable name should be enough.
Cos,thanks for valuable comments and reviewing patch.
indentation: it supposes to be 2 spaces, not 4
Corrected in next patch.
what is the point of making the parent class abstract. If you want to emphasize the fact that some of the methods are going to be overridden you can {{@Override} annotation where it's needed
I'd suggest to change the name of FileContextURIBaseTest to FileContextURIBase because the first reaction is to suggest to rename the class into Test... so it'd be picked up by a hardness
do you want to make this a @BeforeClass method? Otherwise it kinda stands there along
{
for (int i = 0; i < data.length; i++)Unknown macro: { data[i] = (byte) (i % 10); }}
I think createFile has been written line 137 times already - I think you don't need an extra one
As per our discussion you suggested to disregard above comments .
in qualifiedPath(String pathString, FileContext fc you, perhaps, should avoid using a full type name as a part of the var. name. You might want to use hungarian notation, e.g. prefix a String variable with 's', etc. Also, what if fc would be equals to null?
I haven't seen hungarian notation used in most of Hadoop code and there is strong opposition to use hungarian notation.
it isn't necessary to prefix your test methods with word 'test' since you have the annotation in place. Although, there's no single rule about this
Changed .
tearDown takes care about cleaning of fs2. Do you need to do the same for fs1?
I guess you meant fc2 and fc1. These 2 are file context pointing to same Dir/Files so cleaning fc2 in tearDown is enough, no need to clean fc1.
there's a number of places in the code where you have things like
String fileName = "testFile";
Path testPath = qualifiedPath(fileName, fc2);
and which is an only one usage of fileName variable. It's better be declared final in this case or simply use the literal constant in the call directly.
This was intentional for clarity.
I agree with your way of writing testListStatus, I will update that in patch.
Cos I agree with changing name of FileContextURIBaseTest to FileContestURIBase.
In previous comment mistakenly it was included in disregarding section.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12420101/HADOOP-6261.patch
against trunk revision 816794.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 19 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/12/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/12/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/12/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/12/console
This message is automatically generated.
Patch review:
System.getProperty("test.build.data") + "/testContextURI";
needs a default option, see
HADOOP-5916- Remove the TODO comment
- Local and S3 implementations, you're catching but not handling an IOException. The tests shouldn't proceed if one is thrown; don't catch it.
- FileContextURIBase.java:121 Says an IOException is expected but catches an IllegalArgumentException.
Jakob, Thanks for reviewing patch. I have updated patch as per your suggestion.
[exec]
[exec]
[exec] There appear to be 4 release audit warnings before the patch and 4 release audit warnings after applying the patch.
[exec]
[exec]
[exec]
[exec]
[exec] +1 overall.
[exec]
[exec] +1 @author. The patch does not contain any @author tags.
[exec]
[exec] +1 tests included. The patch appears to include 20 new or modified tests.
[exec]
[exec] +1 javadoc. The javadoc tool did not generate any warning messages.
[exec]
[exec] +1 javac. The applied patch does not increase the total number of javac compiler warnings.
[exec]
[exec] +1 findbugs. The patch does not introduce any new Findbugs warnings.
[exec]
[exec] +1 release audit. The applied patch does not increase the total number of release audit warnings.
[exec]
[exec]
[exec]
[exec]
[exec] ======================================================================
[exec] ======================================================================
[exec] Finished build.
[exec] ======================================================================
[exec] ======================================================================
[exec]
[exec]
BUILD SUCCESSFUL
Total time: 20 minutes 15 seconds
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12420119/HADOOP-6261.patch
against trunk revision 816847.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 20 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/16/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/16/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/16/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/16/console
This message is automatically generated.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12420144/HADOOP-6261.patch
against trunk revision 816847.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 20 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/56/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/56/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/56/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/56/console
This message is automatically generated.
Above failure is due to testListStatusFilterWithAnArrayOrPaths test in fs.
http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/56/testReport/org.apache.hadoop.fs/TestLocalFSFileContextMainOperations/testListStatusFilterWithAnArrayOrPaths
Above failure is not related to patch.
This is mine. The test was expecting a certain order to the returned list. I should have a fix shortly. I am creating a jira for the patch.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12420144/HADOOP-6261.patch
against trunk revision 817416.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 20 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/60/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/60/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/60/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/60/console
This message is automatically generated.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12420144/HADOOP-6261.patch
against trunk revision 817496.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 20 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/62/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/62/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/62/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/62/console
This message is automatically generated.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12420144/HADOOP-6261.patch
against trunk revision 817496.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 20 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/17/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/17/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/17/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/17/console
This message is automatically generated.
Committed this change to both trunk and release 21. Thank you Ravi.
Integrated in Hadoop-Common-trunk-Commit #49 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/49/)
. Add URI based tests for FileContext. Contributed by Ravi Pulari.
Integrated in Hadoop-Common-trunk #106 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/106/)
. Add URI based tests for FileContext. Contributed by Ravi Pulari.
Attaching patch for review.