-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-19431. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-distcp. #7368
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@cnauroth Can you help review this PR? Thank you very much! |
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, with one minor change requested.
Additionally, is there a JUnit 4 dependency that should be removed from hadoop-distcp/pom.xml?
@@ -119,6 +119,16 @@ protected SimpleCopyListing(Configuration configuration, | |||
this.randomizeFileListing = randomizeFileListing; | |||
} | |||
|
|||
protected void initSimpleCopyListing(Configuration pConfiguration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please mark this as VisibleForTesting
?
Maybe TestCopyListing
shouldn't be extending SimpleCopyListing
, but no need to take on further refactoring right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please mark this as
VisibleForTesting
?
Thank you for your explanation! I have added the VisibleForTesting
annotation above the initSimpleCopyListing
function.
Maybe TestCopyListing shouldn't be extending SimpleCopyListing, but no need to take on further refactoring right now.
This is a great suggestion! I will record it as a to-do. Once we complete the upgrade from JUnit4 to JUnit5, we will implement this optimization.
@cnauroth We cannot remove the JUnit4 dependency from the |
🎊 +1 overall
This message was automatically generated. |
Oh, that makes perfect sense. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@cnauroth Thank you very much for reviewing the code! I have merged this PR into the trunk branch to proceed with the work of upgrading JUnit4 to JUnit5 in other modules. |
Description of PR
JIRA: HADOOP-19431. Upgrade JUnit from 4 to 5 in hadoop-distcp.
How was this patch tested?
Junit Test & mvn clean test.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?