MAPREDUCE-7419. Upgrade Junit 4 to 5 in hadoop-mapreduce-client-common #5028
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. |
|
@aajisaka - Please help in reviewing in your free time. Thanks. |
|
@aajisaka @slfan1989 Please help in reviewing this PR. Thanks. |
ayushtkn
reviewed
May 12, 2023
There was a problem hiding this comment.
Dropped some comments, in general looks good,
Some generic stuff, don't use * in imports, rather expand. I flagged at some places, but do it where ever you added the import. (don't touch the existing ones).
Second, regarding the test. MR is a dependency to other modules as well and Jenkins by default doesn't run all the tests.
So, once you address all the comments, to make sure nothing break, can you make an additional commit touching the hadoop-project/pom so that entire test runs. We can revert that once we get the green build and merge this PR.
If, everything stays green post this. changes lgtm
| @@ -22,77 +22,81 @@ | |||
| import java.util.Collection; | |||
|
|
|||
| import org.apache.hadoop.conf.Configuration; | |||
|
|
|||
| import static org.junit.jupiter.api.Assertions.*; | |||
There was a problem hiding this comment.
expand, we don't use .* in general
| } | ||
|
|
||
| @Test(timeout = 10000) | ||
| public void testIsJobDirValid() throws IOException { |
There was a problem hiding this comment.
is removing public mandatory? if things can work with this and others staying public, Would prefer it stay as public only
Comment on lines
24
to
25
|
|
||
| import static org.junit.jupiter.api.Assertions.assertNotNull; |
There was a problem hiding this comment.
Hi @ayushtkn . I used Intellij to optimize imports. Can you share what should be ideal order here ? Thanks
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.Timeout; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; |
| @@ -20,6 +20,7 @@ | |||
| import org.apache.hadoop.util.StringUtils; | |||
|
|
|||
| import static org.assertj.core.api.Assertions.assertThat; | |||
| import static org.junit.jupiter.api.Assertions.*; | |||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.jupiter.api.Assertions.*; |
| import static org.junit.Assert.assertNull; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; | ||
| import static org.junit.jupiter.api.Assertions.*; |
|
Thanks a lot @ayushtkn for your review, I will address your comments in my next commit. |
|
🎊 +1 overall
This message was automatically generated. |
@ayushtkn - As suggested the above by you. triggering next run with change in |
|
🎊 +1 overall
This message was automatically generated. |
|
I think it didn’t trigger the tests for the entire project :-( may be not hadoop-project/pom but the pom in main project would do |
Thanks for your review. I have updated |
|
💔 -1 overall
This message was automatically generated. |
|
Have triggered the build again, test were failing due to unable to create native thread. |
|
💔 -1 overall
This message was automatically generated. |
Sorry @ayushtkn for missing it. Can you please help with import order thing ? |
|
Hi @ashutoshcipher I just meant don't use *, there rather expand and the second was if we don't need to remove public and things can work like that also, we should let it stay, it will reduce our code changes, and less chances of people blaming us if something breaks around that to us. there is a code formatter for hadoop in case interested |
|
Will make changes in my next commit. Thanks |
|
💔 -1 overall
This message was automatically generated. |
|
Good to go from my side. I don't think the test failures are related, double check once. If all safe can revert the changes done in the pom and licence to trigger the tests. |
added 2 commits
May 21, 2023 18:29
Thanks @ayushtkn for reviewing it. I checked as well and test failures doesnt look related. Reverted the pom and licence changes as well that were for to trigger the tests |
|
💔 -1 overall
This message was automatically generated. |
74 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
Upgrade Junit 4 to 5 in hadoop-mapreduce-client-common
JIRA - MAPREDUCE-7419
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?