steveloughran commented on a change in pull request #4053:
URL: https://github.com/apache/hadoop/pull/4053#discussion_r822005739
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##########
@@ -640,7 +635,7 @@ public void testUnTar() throws IOException {
assertTrue(regularFile.exists());
try {
FileUtil.unTar(simpleTar, regularFile);
- assertTrue("An IOException expected.", false);
+ fail("An IOException expected.");
Review comment:
move to
```
LambdaTestUtils.intercept(IOException.class, () ->
FileUtil.unTar(simpleTar, regularFile));
```
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##########
@@ -294,7 +292,7 @@ public void testFullyDeleteSymlinks() throws IOException {
ret = FileUtil.fullyDelete(linkDir);
Assert.assertTrue(ret);
Assert.assertFalse(linkDir.exists());
- Assert.assertEquals(3, del.list().length);
+ Assert.assertEquals(3, Objects.requireNonNull(del.list()).length);
Review comment:
i'd be tempted to move to AssertJ here, e.g
```
Assertions.assertThat(del.list)
.describedAs("del list")
.isNotNulll()
.hasLength(3)
```
actually, given how many places it us used, I'd factor this out into its
own assertion used
through all the tests
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##########
@@ -160,7 +162,7 @@ public void setup() throws IOException {
new File(del, FILE).createNewFile();
File tmpFile = new File(tmp, FILE);
- tmpFile.createNewFile();
+ assertTrue(tmpFile.createNewFile());
Review comment:
there's enough use of the same file operations (createNewFile, mkdirs,
delete, exists..) that they could be factored out into helper methods which
include the assertion the return value *and* meaningful exception error messages
##########
File path:
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java
##########
@@ -1419,7 +1443,7 @@ private void putEntriesInTar(TarArchiveOutputStream tos,
File f)
if (f.isDirectory()) {
tos.putArchiveEntry(new TarArchiveEntry(f));
tos.closeArchiveEntry();
- for (File child : f.listFiles()) {
+ for (File child : Objects.requireNonNull(f.listFiles())) {
Review comment:
this will throw an NPE anyway; no need for the extra check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
