HDDS-15109. Extract rename test cases to OzoneFileSystemTests#10120
HDDS-15109. Extract rename test cases to OzoneFileSystemTests#10120len548 wants to merge 2 commits intoapache:masterfrom
Conversation
adoroszlai
added
test
code-cleanup
| Path filePath = pathUnderFsRoot(root + "/file1"); | ||
| ContractTestUtils.touch(getFs(), filePath); | ||
| Path newDestinPath = new Path(filePath, "c"); | ||
| // rename shouldthrow exception |
There was a problem hiding this comment.
Nit: there is a small typo here. "shouldthrow" should be "should throw".
| // rename shouldthrow exception | |
| // rename should throw exception |
|
|
||
| abstract void verifyRenameFile(Path workDir, Path expectedDest) throws IOException; | ||
|
|
||
| abstract Path pathUnderFsRoot(String pathUnderFsRoot) throws IOException; |
There was a problem hiding this comment.
Nit: could we remove throws IOException from pathUnderFsRoot? The current implementations do not seem to throw IOException, so the simpler signature may be enough.
| void verifyRenameFile(Path workDir, Path expectedDest) throws IOException { | ||
| FileStatus[] fStatus = getFs().listStatus(workDir); | ||
| assertEquals(1, fStatus.length, "Renamed failed"); | ||
| } |
There was a problem hiding this comment.
Could we also verify expectedDest here, or add a short comment explaining why RootedOzoneFileSystem only checks the number of entries? Since expectedDest is passed from the base helper, skipping the path assertion looks a bit unclear to me.
| assertTrue(fs.exists(expectedFilePathAfterRename), "Rename failed"); | ||
| } | ||
|
|
||
| public void renameDirToItsOwnSubDir() throws Exception { |
There was a problem hiding this comment.
Nit: would it be better to keep this helper package-private, similar to the other rename helpers?
| public void renameDirToItsOwnSubDir() throws Exception { | |
| void renameDirToItsOwnSubDir() throws Exception { |
sreejasahithi
left a comment
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @len548 , left few minor comments
| @Override | ||
| Path pathUnderFsRoot(String p) { | ||
| return new Path(getBucketPath() + p); | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: parameter name does not convey any meaning , could you please change this to a descriptive name like relativePath.
| protected Path pathUnderFsRoot(String p) { | ||
| return new Path(getFs().getUri().toString() + p); |
| void renameDestinationParentDoesNotExist() throws Exception { | ||
| final String root = "/root_dir"; | ||
| final String dir1 = root + "/dir1"; | ||
| final String dir2 = dir1 + "/dir2"; | ||
| final Path dir2SourcePath = pathUnderFsRoot(dir2); | ||
| getFs().mkdirs(dir2SourcePath); | ||
| // (a) parent of dst does not exist. /root_dir/b/c | ||
| final Path destinPath = pathUnderFsRoot(root + "/b/c"); | ||
|
|
||
| // rename should throw exception | ||
| try { | ||
| getFs().rename(dir2SourcePath, destinPath); | ||
| fail("Should fail as parent of dst does not exist!"); | ||
| } catch (FileNotFoundException fnfe) { | ||
| //expected | ||
| } | ||
| // (b) parent of dst is a file. /root_dir/file1/c |
There was a problem hiding this comment.
capture FileSystem fs = getFs() once and reuse it similar to how it is done in other methods.
4 participants
What changes were proposed in this pull request?
There are duplicates of rename tests between
AbstractOzoneFileSystemTestandAbstractRootedOzoneFileSystemTest.testRenameFiletestRenameFileToDirtestRenameToParentDirtestRenameDirToItsOwnSubDirtestRenameDestinationParentDoesntExist(O3FS) /testRenameDestinationParentDoesNotExist(OFS) – same test, different spelling.This PR moved shared logic of these tests to OzoneFileSystemTestBase and also introduced a helper method
pathUnderFsRoot(String p)which centralizes the difference “fs.getUri().toString() + path” (O3FS) vs "getBucketPath() + path" (OFS) by template method pattern.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15109
How was this patch tested?
CI pass: https://github.com/len548/ozone/actions/runs/24833798824