TEZ-4719: Move to Junit6#502
Merged
Merged
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Aggarwal-Raghav
changed the title
Aggarwal-Raghav
commented
Jun 1, 2026
|
|
||
| import org.junit.Assert; | ||
| import org.junit.Test; | ||
| import org.junit.jupiter.api.*; |
Contributor
Author
There was a problem hiding this comment.
Which is the preferred way?
- Using single line separte import
*imports
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
UT has passed, which is good thing, few indendations and java formatting is left that will be handled once final rebase with #505 is done. |
Aggarwal-Raghav
force-pushed
the
TEZ-4719
branch
3 times, most recently
from
5fb400c to
5f401be
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Aggarwal-Raghav
force-pushed
the
TEZ-4719
branch
from
5f401be to
e60e50a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Aggarwal-Raghav
force-pushed
the
TEZ-4719
branch
from
e60e50a to
19fd7ae
Compare
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
@abstractdog , this is ready for review. |
abstractdog
requested changes
Jun 30, 2026
abstractdog
left a comment
abstractdog
left a comment
Contributor
There was a problem hiding this comment.
this is huge so far, thanks @Aggarwal-Raghav ! left some comments
Comment on lines
+128
to
+132
| Environment.PWD.$() | ||
| + File.pathSeparator | ||
| + "USER_PATH" | ||
| + File.pathSeparator | ||
| + "DEFAULT_PATH", |
Contributor
There was a problem hiding this comment.
it's a matter of taste, but I don't think indenting a string concatenation this way makes the code better, a single line is more compact and still pretty much readable
Contributor
Author
There was a problem hiding this comment.
i was using google java formatter in intelliJ, now I have moved to tez project checkstyle present at tez-build-tools/src/main/resources/checkstyle/checkstyle.xml and updated the whole PR manually file by file. Fixed line length >120 in this PR. I hope it should help in checkstyle/spotbugs as well.
Contributor
There was a problem hiding this comment.
thanks for clarifying!
I believe we can start soon a community discussion about which formatter to accept, and then we can enforce it by spotless: that's going to be another huge patch, not a problem :)
abstractdog
reviewed
Jun 30, 2026
abstractdog
reviewed
Jun 30, 2026
abstractdog
reviewed
Jun 30, 2026
abstractdog
reviewed
Jun 30, 2026
Aggarwal-Raghav
force-pushed
the
TEZ-4719
branch
2 times, most recently
from
dd8e0a0 to
6374968
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Aggarwal-Raghav
force-pushed
the
TEZ-4719
branch
from
6374968 to
654d8b1
Compare
Contributor
Author
|
CI has failed because of some artifactory issue, have re-triggered it |
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
Author
|
Giving 1 more try, there is some infra issue i guess, puppycrawler is going OOM, and different issue in checkstyle , sporbugs. then will play aroung Jenkinsfile. I know there are few checkstyle issues still because of this PR, will handle once CI start passing. |
Contributor
Author
|
Failing with OOM. |
Contributor
Author
|
@abstractdog , is it because of some jenkins node problem? |
Contributor
Author
|
rebasing as there are merge conflict. The older run is stuck, neither failing nor proceeding. |
Contributor
yeah, please be aware of a recent unit test, which might need to be migrated: 8fde353 |
Contributor
Author
yes, doing that only :-) |
Contributor
Author
|
For |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
CI has passed @abstractdog , I think the checkstyle especially in javac can be handled separately? |
Contributor
agree! those problems were not introduced by this patch, and this is already huge, it's worth moving on before further conflicts :) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Update Assertions:
org.junit.Assert.* → org.junit.jupiter.api.Assertions.*Removed the transitive dependency on
Hamcrestmatchers.Migrated all
@Test(timeout = ...)parameters to the@Timeoutannotation while maintianing the Millisecond timeunitReplaced
@Rule TemporaryFolderwith JUnit Jupiter's@TempDirNote: In JUnit 4, the custom assertion message was the first parameter. In JUnit 6, it is the last parameter.
In JUnit 4:
assertEquals("Values should match", expected, actual);In JUnit 6:
assertEquals(expected, actual, "Values should match");