Skip to content

HDDS-15109. Extract rename test cases to OzoneFileSystemTests#10120

Merged
adoroszlai merged 2 commits intoapache:masterfrom
len548:HDDS-12355-5
May 7, 2026
Merged

HDDS-15109. Extract rename test cases to OzoneFileSystemTests#10120
adoroszlai merged 2 commits intoapache:masterfrom
len548:HDDS-12355-5

Conversation

@len548
Copy link
Copy Markdown
Contributor

@len548 len548 commented Apr 24, 2026

What changes were proposed in this pull request?

There are duplicates of rename tests between AbstractOzoneFileSystemTest and AbstractRootedOzoneFileSystemTest.

  • testRenameFile
  • testRenameFileToDir
  • testRenameToParentDir
  • testRenameDirToItsOwnSubDir
  • testRenameDestinationParentDoesntExist (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

@adoroszlai adoroszlai added test code-cleanup Changes that aim to make code better, without changing functionality. labels Apr 24, 2026
@adoroszlai adoroszlai requested a review from sadanand48 April 25, 2026 04:45
Copy link
Copy Markdown
Contributor

@Russole Russole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @len548 for working on this. Overall, LGTM. I just left a few minor comments.

Comment on lines +1744 to +1747
void verifyRenameFile(Path workDir, Path expectedDest) throws IOException {
FileStatus[] fStatus = getFs().listStatus(workDir);
assertEquals(1, fStatus.length, "Renamed failed");
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@sreejasahithi sreejasahithi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @len548 , left few minor comments

Copy link
Copy Markdown
Contributor

@sreejasahithi sreejasahithi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @len548, overall LGTM

@len548
Copy link
Copy Markdown
Contributor Author

len548 commented May 7, 2026

Hi @Russole, can you review this again when you have time? Thanks!

Copy link
Copy Markdown
Contributor

@Russole Russole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @len548 for working on this. LGTM!

@adoroszlai adoroszlai merged commit cf76290 into apache:master May 7, 2026
31 of 32 checks passed
@adoroszlai
Copy link
Copy Markdown
Contributor

Thanks @len548 for the patch, @Russole, @sreejasahithi for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-cleanup Changes that aim to make code better, without changing functionality. test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants