TEZ-4719: Move to Junit6#502
Open
Aggarwal-Raghav wants to merge 2 commits into
Open
Conversation
|
💔 -1 overall
This message was automatically generated. |
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
|
💔 -1 overall
This message was automatically generated. |
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
|
💔 -1 overall
This message was automatically generated. |
Aggarwal-Raghav
force-pushed
the
TEZ-4719
branch
from
5f401be to
e60e50a
Compare
|
💔 -1 overall
This message was automatically generated. |
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.
| import org.apache.hadoop.net.DNSToSwitchMapping; | ||
| import org.apache.hadoop.yarn.util.RackResolver; | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
this doesn't seem to be needed, there is already a line break between imports and class javadoc
Contributor
Author
There was a problem hiding this comment.
It seems the line breaks were added during rebase. Might have made some mistake in IDE. Will fix them
| <groupId>junit</groupId> | ||
| <artifactId>junit</artifactId> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter</artifactId> |
Contributor
There was a problem hiding this comment.
on current tez master, I can see this:
[INFO] +- org.apache.hadoop:hadoop-mapreduce-client-core:jar:3.5.0:test (scope not updated to test)
[INFO] | +- (org.apache.hadoop:hadoop-yarn-client:jar:3.5.0:test - version managed from 3.5.0; omitted for duplicate)
[INFO] | +- (org.apache.hadoop:hadoop-yarn-common:jar:3.5.0:test - version managed from 3.5.0; omitted for duplicate)
[INFO] | +- (org.apache.hadoop:hadoop-hdfs-client:jar:3.5.0:test - version managed from 3.5.0; omitted for duplicate)
[INFO] | +- (com.fasterxml.jackson.core:jackson-databind:jar:2.18.6:test - version managed from 2.18.6; omitted for duplicate)
[INFO] | +- (org.slf4j:slf4j-api:jar:1.7.36:test - version managed from 1.7.36; omitted for duplicate)
[INFO] | +- (org.slf4j:slf4j-reload4j:jar:1.7.36:test - version managed from 1.7.36; omitted for duplicate)
[INFO] | \- (io.netty:netty-all:jar:4.1.130.Final:compile - version managed from 4.1.130.Final; scope managed from compile; omitted for duplicate)
for me, this means that module is not about to pull in junit-jupiter at all, is this exclusion needed?
Contributor
Author
There was a problem hiding this comment.
Removed junit exclusion from here
| </dependency> | ||
| <dependency> | ||
| <groupId>org.junit.vintage</groupId> | ||
| <artifactId>junit-vintage-engine</artifactId> |
Contributor
There was a problem hiding this comment.
after removing the vintage engine, the old tests won't run (silently): even though there is no Junit4 after this PR, can you please add an enforcer rule to ban junit4 completely?
abstractdog
reviewed
Jun 30, 2026
| @@ -20,6 +20,7 @@ | |||
|
|
|||
| import org.apache.hadoop.util.ProgramDriver; | |||
|
|
|||
|
|
|||
Contributor
There was a problem hiding this comment.
this line break is not needed
abstractdog
reviewed
Jun 30, 2026
|
|
||
| /** | ||
| * Test to verify secure-shuffle (SSL mode) in Tez | ||
| */ | ||
| @RunWith(Parameterized.class) | ||
|
|
Contributor
There was a problem hiding this comment.
this extra line is not needed after removing the annotation
abstractdog
reviewed
Jun 30, 2026
Comment on lines
+65
to
+66
|
|
||
|
|
Contributor
There was a problem hiding this comment.
these line breaks are not needed
abstractdog
reviewed
Jun 30, 2026
Comment on lines
+181
to
+182
|
|
||
|
|
Contributor
There was a problem hiding this comment.
these line breaks are not needed
Aggarwal-Raghav
force-pushed
the
TEZ-4719
branch
from
19fd7ae to
dd8e0a0
Compare
Aggarwal-Raghav
force-pushed
the
TEZ-4719
branch
from
dd8e0a0 to
6374968
Compare
|
💔 -1 overall
This message was automatically generated. |
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");