HADOOP-19415. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-common Part1. #7339
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@steveloughran @ayushtkn @cnauroth Can you help review this PR? Thank you very much! |
|
💔 -1 overall
This message was automatically generated. |
cnauroth
reviewed
Jan 30, 2025
There was a problem hiding this comment.
Hi @slfan1989 !
I haven't reviewed the whole thing, but I have some general questions about the process, similar to discussion in #7337 . It seems there are several changes that aren't directly related to JUnit 5 migration. In #7337, I was in favor of an approach of a straight port to JUnit 5, with any other code quality improvements to be done later. I flagged just a few examples of what I'm talking about in line-level comments.
Are these unrelated changes something that is also being applied by the automated tool, or are these manual coding chnages done after running the tool?
Pre-submit also reports new Checkstyle warnings. I'd be willing to let that slide in order to get this conversion done more quickly, but I'm curious if other reviewers disagree.
| @@ -113,8 +119,7 @@ public void setUp() throws Exception { | |||
| readTestConfigFile(); | |||
|
|
|||
| conf = new Configuration(); | |||
| conf.setBoolean(CommonConfigurationKeys.HADOOP_SECURITY_AUTHORIZATION, | |||
| true); | |||
| conf.setBoolean(CommonConfigurationKeys.HADOOP_SECURITY_AUTHORIZATION, true); | |||
There was a problem hiding this comment.
Unrelated formatting change?
There was a problem hiding this comment.
Thank you for helping review the code! I have rolled back the unnecessary changes.
| @@ -155,31 +160,27 @@ private void displayResults() { | |||
| // Display the details only if there is a failure | |||
| if (!testResult) { | |||
| LOG.info("-------------------------------------------"); | |||
| LOG.info(" Test ID: [" + (i + 1) + "]"); | |||
| LOG.info(" Test Description: [" + td.getTestDesc() + "]"); | |||
| LOG.info(" Test ID: [ {} ]", (i + 1)); | |||
| OpensslCipher cipher = OpensslCipher.getInstance("AES/CTR/NoPadding"); | ||
| Assert.assertTrue(cipher != null); | ||
| assertNotNull(cipher); |
There was a problem hiding this comment.
Unrelated assert style change?
|
💔 -1 overall
This message was automatically generated. |
cnauroth
requested changes
Feb 1, 2025
There was a problem hiding this comment.
@slfan1989 , I think I'm close to giving the LGTM on this one! I entered just one more question about some commented-out code.
The patch has 1 line(s) that end in blanks.
The patch 2 line(s) with tabs.
Can you please clean those up? No need to worry about any other Checkstyle stuff though.
I think for this first JUnit 5 patch, it would be good to get a review from one other committer too. Let's make sure we have consensus on how these patches are being done.
| @@ -2557,11 +2546,11 @@ public void testGettingPropertiesWithPrefix() throws Exception { | |||
| assertTrue(prefixedProps.isEmpty()); | |||
| } | |||
|
|
|||
| public static void main(String[] argv) throws Exception { | |||
| /*public static void main(String[] argv) throws Exception { | |||
There was a problem hiding this comment.
Could this not be translated to JUnit 5?
I'm actually not sure if it's valuable anyway. If it's not needed, let's delete it instead of commenting it out.
There was a problem hiding this comment.
This change was made during my local testing, and I have already rolled it back. Thank you for pointing out this issue.
|
@cnauroth Thank you very much for reviewing the code! I will work on improving this PR as soon as possible and provide responses to the issues you raised. |
@cnauroth Thank you very much for your comment! Regarding the migration standard from JUnit4 to JUnit5, we have reached a consensus to make changes only when necessary. Currently, our conversion tool cannot automatically fix Checkstyle issues, so these need to be addressed manually. |
@steveloughran @ayushtkn Could you please help review this PR? Thank you very much! |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@steveloughran Do you have any suggestions for this PR? |
|
@cnauroth @ayushtkn Thank you very much for reviewing this PR! I will merge it into the trunk branch first to facilitate the JUnit upgrade for other packages in hadoop-common. After merging this PR, I will monitor the JDK8/JDK11 build emails for one day to ensure there are no other issues. cc: @steveloughran |
74 tasks
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
JIRA: HADOOP-19415. Upgrade JUnit from 4 to 5 in hadoop-common Part1.
This PR is the first PR for hadoop-common, modifying the unit tests under the
cliconfconstantscryptopackage.I will list some basic information about the upgrade in the form of a table.
How was this patch tested?
This PR modifies the unit tests, and we don't need new unit tests.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?