[
https://issues.apache.org/jira/browse/HADOOP-6261?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12756879#action_12756879
]
Konstantin Boudnik commented on HADOOP-6261:
--------------------------------------------
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
{noformat}
{
for (int i = 0; i < data.length; i++) {
data[i] = (byte) (i % 10);
}
}
{noformat}
- 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
{noformat}
String fileName = "testFile";
Path testPath = qualifiedPath(fileName, fc2);
{noformat}
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
{noformat}
Assert
.assertEquals(qualifiedPath("test/hadoop/c", fc1),
paths[2].getPath());
{noformat}
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
{noformat}
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);
}
{noformat}
this way you have less code duplication
> Junit tests for FileContextURI
> ------------------------------
>
> Key: HADOOP-6261
> URL: https://issues.apache.org/jira/browse/HADOOP-6261
> Project: Hadoop Common
> Issue Type: Test
> Components: test
> Reporter: Ravi Phulari
> Assignee: Ravi Phulari
> Attachments: HADOOP-6261.patch
>
>
> FileContextURI unit tests.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
