YARN-11258. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-yarn-server-common. #7437
Conversation
This file contains hidden or 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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@cnauroth Could you please help review this PR? Thank you very much! |
|
💔 -1 overall
This message was automatically generated. |
cnauroth
requested changes
Feb 27, 2025
| private FederationStateStoreFacade facade; | ||
|
|
||
| public TestFederationCache(Class cacheClassName) { | ||
| public void initTestFederationCache(Class cacheClassName) |
There was a problem hiding this comment.
Can this be made private?
There was a problem hiding this comment.
Thank you for reviewing this PR! I will work on improving it.
| private Boolean isCachingEnabled; | ||
|
|
||
| public TestFederationStateStoreFacade(Boolean isCachingEnabled) { | ||
| public void initTestFederationStateStoreFacade(Boolean pIsCachingEnabled) |
There was a problem hiding this comment.
Can this be made private?
There was a problem hiding this comment.
I will work on improving it.
| } | ||
|
|
||
| @VisibleForTesting | ||
| protected ZKFederationStateStoreOpDurations resetOpDurations() { |
There was a problem hiding this comment.
I think I understand why the resetOpDurations changes were made. (There's a singleton instance holding state across test cases.) However, did something about JUnit 5 trigger the need for this? Is it just an unrelated bug?
There was a problem hiding this comment.
Thank you very much for your question! it's a very good one. Apologies for the delayed response, as I've been caught up with some work in the past 1-2 days.
I have noticed a difference in behavior between Junit4 and Junit5 when running unit tests with inheritance. In Junit4, the test methods of the parent and child classes are executed separately, while in Junit5, the test methods of the parent and child classes are coupled together and executed in the child class.
-
Junit4: The test methods of the parent and child classes are generally executed separately. If we define a test method in the parent class, the child class can inherit these methods and also define its own test methods. During execution, Junit4 will first execute the parent class's test methods, then the child class's test methods.
-
Junit5: In Junit5, the test methods in the parent class and the child class are executed together. The inheritance mechanism in Junit5 causes the parent class's test methods to be inherited into the child class, and by default, both the parent and child test methods are executed, with a different execution order compared to Junit4.
This difference leads to an issue: in the TestZookeeperFederationStateStore#testMetricsInited test, my goal is to check whether the metric accumulation behaves as expected.
In Junit4, ZKFederationStateStoreOpDurations is recreated when executing the child class's tests, so the data it contains is the initial value, which matches the expected result.
However, in Junit5, due to the singleton nature of ZKFederationStateStoreOpDurations, it accumulates metric data from all methods in the unit test, leading to a result that does not meet expectations.
There was a problem hiding this comment.
To ensure that the unit test passes in Junit5, I added a reset method here to make the execution result match the expected outcome.
There was a problem hiding this comment.
I understand it now. Thanks for the detailed explanation!
|
🎊 +1 overall
This message was automatically generated. |
cnauroth
approved these changes
Mar 1, 2025
75 tasks
YanivKunda
pushed a commit
to YanivKunda/hadoop
that referenced
this pull request
Mar 23, 2025
…ommon. (apache#7437) [JDK17] Upgrade JUnit from 4 to 5 in hadoop-yarn-server-common. Co-authored-by: Chris Nauroth <[email protected]> Co-authored-by: Hualong Zhang <[email protected]> Reviewed-by: Chris Nauroth <[email protected]> Reviewed-by: Hualong Zhang <[email protected]> Signed-off-by: Shilun Fan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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: YARN-11258. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-yarn-server-common.
How was this patch tested?
Junit Test & mvn clean test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?