[GitHub] [hadoop] steveloughran commented on a diff in pull request #5909: Hadoop 18826: [ABFS] Fix for Empty Relative Path Issue Leading to GetFileStatus("/") failure.

[via GitHub]

steveloughran commented on code in PR #5909:
URL: https://github.com/apache/hadoop/pull/5909#discussion_r1279811438


##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemFileStatus.java:
##########
@@ -83,9 +85,11 @@ private FileStatus validateStatus(final AzureBlobFileSystem 
fs, final Path name,
       if (isDir) {
         assertEquals(errorInStatus + ": permission",
                 new FsPermission(DEFAULT_DIR_PERMISSION_VALUE), 
fileStatus.getPermission());
+        assertTrue(fileStatus.isDirectory());

Review Comment:
   we always need error messages on simple asserttrue/false. Think to yourself 
"if this test failed and I had was the test report, what would I want to know". 
   
   here: the filestatus. Something like
   ```
   assertTrue("not a directory " + fileStatus, fileStatus.isDirectory())
   ```
   



##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemFileStatus.java:
##########
@@ -83,9 +85,11 @@ private FileStatus validateStatus(final AzureBlobFileSystem 
fs, final Path name,
       if (isDir) {
         assertEquals(errorInStatus + ": permission",
                 new FsPermission(DEFAULT_DIR_PERMISSION_VALUE), 
fileStatus.getPermission());
+        assertTrue(fileStatus.isDirectory());
       } else {
         assertEquals(errorInStatus + ": permission",
                 new FsPermission(DEFAULT_FILE_PERMISSION_VALUE), 
fileStatus.getPermission());
+        assertTrue(fileStatus.isFile());

Review Comment:
   same



##########
hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemFileStatus.java:
##########
@@ -144,4 +148,22 @@ public void testLastModifiedTime() throws IOException {
     assertTrue("lastModifiedTime should be before createEndTime",
         createEndTime > lastModifiedTime);
   }
+
+  @Test
+  public void testFileStatusOnRoot() throws IOException {
+    AzureBlobFileSystem fs = this.getFileSystem();

Review Comment:
   nit: no need for `this.`



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


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