MAPREDUCE-7418. [JDK17] Upgrade Junit 4 to 5 in hadoop-mapreduce-client-app. #7350
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
slfan1989
changed the title
|
💔 -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. |
|
The PR has the following points that need to be explained: TestAMWebApp.javaIn this unit test, EnvironmentVariables was introduced to simulate an SSL environment by modifying the System.Env environment variables during runtime. However, starting from JDK 9, direct modification of environment variables is prohibited. Therefore, we replaced it with the recommended approach using System.setProperty to set temporary variables. Additionally, we added logic in the WebApps to retrieve environment variables to ensure the test runs smoothly. TestStagingCleanup.javaThe Checkstyle issues in this class cannot be automatically fixed due to an extra space at the beginning of each line, which causes Checkstyle errors. New Dependency: mockito-junit-jupiterWe introduced the mockito-junit-jupiter dependency to support the Mockito mocking functionality required in the TestTaskAttemptListenerImpl class. |
cnauroth
reviewed
Feb 5, 2025
| @@ -369,9 +369,15 @@ public void setup() { | |||
|
|
|||
| if (httpScheme.equals(WebAppUtils.HTTPS_PREFIX)) { | |||
| String amKeystoreLoc = System.getenv("KEYSTORE_FILE_LOCATION"); | |||
| if (StringUtils.isBlank(amKeystoreLoc)) { | |||
There was a problem hiding this comment.
Why were changes needed in this file?
There was a problem hiding this comment.
Thank you for your question!
We made this adjustment because the unit tests TestAMWebApp#testMRWebAppSSLEnabled and TestAMWebApp#testMRWebAppSSLEnabledWithClientAuth are failing in JUnit 5.
In the original JUnit 4 environment, these tests relied on the EnvironmentVariables.set method to modify System.Env data. However, in JUnit 5, we can no longer modify the environment variables directly. One important reason for this is that starting from JDK 9, System.Env became an immutable structure. It is recommended to use System.setProperty and System.getProperty as replacements.
Therefore, we modified the unit tests to temporarily set System.setProperty, and also check if the variables we set exist in System.getProperty when used.
| import org.junit.jupiter.api.extension.BeforeEachCallback; | ||
| import org.junit.jupiter.api.extension.ExtensionContext; | ||
|
|
||
| public class EnvironmentVariablesExtension implements BeforeEachCallback { |
There was a problem hiding this comment.
Is this needed? I couldn't find it used anywhere else in the patch.
There was a problem hiding this comment.
I previously tried using a JUnit 5 extension to set the environment variables, but this approach didn't work as expected. In the latest code, I have removed this class.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
cnauroth
reviewed
Feb 6, 2025
| @@ -257,12 +260,6 @@ public void testGetMapCompletionEvents() throws IOException { | |||
| createTce(3, false, TaskAttemptCompletionEventStatus.FAILED) }; | |||
| TaskAttemptCompletionEvent[] mapEvents = { taskEvents[0], taskEvents[2] }; | |||
| Job mockJob = mock(Job.class); | |||
| when(mockJob.getTaskAttemptCompletionEvents(0, 100)) | |||
There was a problem hiding this comment.
Is this code removed intentionally?
There was a problem hiding this comment.
Thank you for your comment!
We removed this part of the code based on the suggestion from Mockito, which indicated that this code was not called in the test and recommended its removal.
The error messages are as follows:
org.mockito.exceptions.misusing.UnnecessaryStubbingException:
Unnecessary stubbings detected.
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of code):
1. -> at org.apache.hadoop.mapred.TestTaskAttemptListenerImpl.testGetMapCompletionEvents(TestTaskAttemptListenerImpl.java:264)
2. -> at org.apache.hadoop.mapred.TestTaskAttemptListenerImpl.testGetMapCompletionEvents(TestTaskAttemptListenerImpl.java:266)
3. -> at org.apache.hadoop.mapred.TestTaskAttemptListenerImpl.testGetMapCompletionEvents(TestTaskAttemptListenerImpl.java:268)
Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.
| @@ -369,9 +369,15 @@ public void setup() { | |||
|
|
|||
| if (httpScheme.equals(WebAppUtils.HTTPS_PREFIX)) { | |||
| String amKeystoreLoc = System.getenv("KEYSTORE_FILE_LOCATION"); | |||
| if (StringUtils.isBlank(amKeystoreLoc)) { | |||
cnauroth
approved these changes
Feb 6, 2025
|
@cnauroth Thank you very much for reviewing the code! I will merge this PR into the trunk branch. The Checkstyle issues will not be fixed for now, as they are caused by the TestStagingCleanup.java class. Some methods in this class have inaccurate Checkstyle (an extra space on each line). It will be fixed in the future when possible. |
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
MAPREDUCE-7418. [JDK17] Upgrade Junit 4 to 5 in hadoop-mapreduce-client-app.
How was this patch tested?
Junit Test & mvn clean test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?