HADOOP-19425. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-azure Part1. #7369
+1,120
−1,089
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. |
slfan1989
force-pushed
the
HADOOP-19425
branch
from
7f6734c to
b52214a
Compare
|
I am trying to modify the code of hadoop-azure, this is part 1. I am syncing the improvements so that it can be better reviewed: 1. TestNameIn JUnit 5, the Here is an example:
2. TimeoutIn JUnit 5, Here is an example:
3. Assume.assumeNotNull → Assumptions.assumeTrueIn JUnit 5, there is no Here is an example: 4. ExpectedException → assertThrowsIn JUnit 5, the 5. @ignore → @disabledIn JUnit 5, 6. Method Order: @FixMethodOrder → @TestMethodOrderIn JUnit 5, |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@cnauroth @steveloughran @anujmodi2021 Can you help review this PR? Thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
cnauroth
reviewed
Feb 9, 2025
There was a problem hiding this comment.
Hi @slfan1989 . This looks good to me overall. I entered one question.
Are you able to run these tests against Azure to make sure they're all working? Unfortunately, I can't help with that kind of testing. I can test GCS and S3A (using GCS's S3-compatible XML API), but not the other cloud providers. If you need help with this testing, then you might need to work with someone else.
| @@ -434,10 +431,6 @@ class MatchesPattern extends TypeSafeMatcher<String> { | |||
| } | |||
| } | |||
|
|
|||
| expectedEx.expectMessage(new MatchesPattern( | |||
There was a problem hiding this comment.
Did we lose an expectation here?
There was a problem hiding this comment.
In JUnit 5, org.junit.rules.ExpectedException has been deprecated, and it is recommended to use assertThrows to validate exception throwing, as shown below:
import static org.junit.jupiter.api.Assertions.assertThrows;
@Test
public void shouldThrowException() {
assertThrows(IllegalArgumentException.class, () -> {
someMethodThatThrows();
});
}
The change is primarily in syntax, where assertThrows captures exceptions via lambda expressions, replacing the ExpectedException rule from JUnit 4. However, it does not have any fundamental impact on the logic of unit tests.
I can further improve this unit test to also validate the exception message using a regular expression.
There was a problem hiding this comment.
I have seen the conversions to assertThrows. Those all look good.
Perhaps what I find confusing is that the setupExpectations() method now seems to be a no-op. It seems to define a custom matcher class, but then never use it. Should setupExpectations() be removed, or am I missing something?
There was a problem hiding this comment.
Thank you for your suggestion! I also believe that setupExpectations should be removed. I will continue to improve the code.
@cnauroth Thank you very much for your explanation! I think this change needs to be verified by @steveloughran and @anujmodi2021 together, and I hope it can pass the integration tests. If there are any issues, I will make improvements as needed. |
74 tasks
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks a lot @slfan1989 for the patch. |
@anujmodi2021 Thank you very much for your help! I sincerely hope that our work will be completed successfully. |
cnauroth
approved these changes
Feb 10, 2025
There was a problem hiding this comment.
+1, pending successful run of the integration tests. Thank you, @slfan1989 ! @anujmodi2021 , thank you for the help with testing.
There was a problem hiding this comment.
Thanks @slfan1989 and pardon me for the delay in review.
Changes look good to me. No functionality change as such.
Have added some minor doubts and suggestions.
Waiting for test results to come in before +1.
Also, are you planning to do same work for ABFS Driver as well?? As part 2 of this Jira??
| @@ -157,7 +157,7 @@ public void testTransientErrorOnDelete() throws Exception { | |||
| // Need to do this test against a live storage account | |||
| AzureBlobStorageTestAccount testAccount = | |||
| AzureBlobStorageTestAccount.create(); | |||
| assumeNotNull(testAccount); | |||
| assumeTrue(testAccount != null); | |||
There was a problem hiding this comment.
Nit: would it be better to define a accessible method assumeNotNull(variable) that internally calls assumeTrue(variable!= null);
This will reduce the overall diffs I feel.
There was a problem hiding this comment.
Thank you for your suggestions! I will try to improve this part of the code.
| @@ -40,7 +41,7 @@ public class ITestNativeAzureFileSystemConcurrencyLive | |||
| extends AbstractWasbTestBase { | |||
|
|
|||
| private static final int THREAD_COUNT = 102; | |||
| private static final int TEST_EXECUTION_TIMEOUT = 30000; | |||
| private static final int TEST_EXECUTION_TIMEOUT = 30; | |||
There was a problem hiding this comment.
Why we are reducing this value?? Any specific reason or observation around this?
There was a problem hiding this comment.
I think this is more that from JUnit 4 to 5, the default time unit for timeout settings has transitioned from milliseconds to seconds. Hence, the constant changes from 30000 (milliseconds) to 30 (seconds). The value is not being reduced.
There was a problem hiding this comment.
@anujmodi2021 The understanding of @cnauroth is correct. In JUnit 5, the timeout is in seconds, while in JUnit 4, the timeout is in milliseconds. So, I changed this value to be in seconds.
We plan to upgrade all unit tests for hadoop-azure to JUnit 5. I will continue with Part 2, but to make the review process smoother, we will keep the code changes smaller for each commit. Additionally, any parts that depend on hadoop-common will be addressed in a separate PR(#7375). However, we will need your assistance throughout the entire upgrade process. |
Thank you for leading efforts on this @slfan1989 |
|
I got a few test failures on this patch. Since We majorly work on ABFS and don't have much context on Wasb we are not sure currently if they are due to some code change or mis configuration on our part. I will continue to look into them today, please pitch in if you might know why they are failing. For the |
...ools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/ITestWasbUriAndConfiguration.java
Outdated
Show resolved
Hide resolved
...s/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/integration/ITestAzureHugeFiles.java
Show resolved
Hide resolved
It sounds like you have a solid lead on For the rest, are you able to share more details on the assertions that are failing? There is a very small chance that I could dig back into my brain's WASB archives from 10 years ago and provide a hint. No promises, but I'll try! |
|
@anujmodi2021 I have submitted a commit with adjustments to the code we discussed earlier. If you have any new questions, feel free to leave a comment. If we can provide more specific error messages, it will be easier to pinpoint the issue. Thank you again! cc: @cnauroth |
|
🎊 +1 overall
This message was automatically generated. |
Yes, let me share the traces here.
|
|
@cnauroth For ITestAzureFileSystemInstrumentation.testMetricsOnFileCreateRead. This is also failing for me on trunk as well. |
|
I will submit another version of the code later to try to fix this issue. |
Sounds good @slfan1989 |
|
ITestFileSystemOperationExceptionMessage.testAnonymouseCredentialExceptionMessage: This is failing because it expects an exception from file system initialization: It seems to specify a random bucket and expect it to fail. I wonder if something in the initialization path has changed so that it's not making the remote call that would have triggered this error? If it's a pre-existing failure, then I suggest we file a separate bug for follow-up and take it out of scope of this issue. |
|
ITestWasbUriAndConfiguration.testConnectUsingSecureSAS: Sorry, I'm not sure about this one. I don't recall seeing this code while I was working on WASB, so maybe it came later. |
|
ITestAzureFileSystemInstrumentation.testMetricsOnFileCreateRead: I'm guessing this is some kind of race on full metrics propagation before the assert runs. Once again, if it fails on trunk, then I'd suggest filing a separate follow-bug issue and de-scope it from the current patch. |
|
@cnauroth @anujmodi2021 I will continue to follow up on the CheckStyle issues for this PR. If the CheckStyle issues are related to method names or magic numbers, we will not be fixing them for now. Are there any other concerns with this PR? If not, we plan to merge it tomorrow. Thank you for your help with the verification and review! Examples of CheckStyle issues that will not be fixed for now: |
|
🎊 +1 overall
This message was automatically generated. |
cnauroth
approved these changes
Feb 12, 2025
There was a problem hiding this comment.
@slfan1989 and @anujmodi2021 , I think this patch is good. I recommended follow-up bugs on known test failures, but I think this patch can proceed without resolving those.
I remain +1.
Thank you to @slfan1989 for the patch and thank you to @anujmodi2021 for the review!
+1 from my side as well. |
| * @return a method name unique to (fork, method). | ||
| * @throws IOException IO problems | ||
| */ | ||
| protected Path methodPath() throws IOException { | ||
| return path(methodName.getMethodName()); | ||
| protected Path methodPath(TestInfo testInfo) throws IOException { |
There was a problem hiding this comment.
why is Junit 5 trying to ruin our lives. Everything worked.
setup should be caching that testinfo for the current suite, and all these helper methods just using that to derive values. we shouldn't be passing it down
There was a problem hiding this comment.
Thank you for raising this issue! In JUnit 5, org.junit.rules.TestName has been removed, and it is recommended to use TestInfo to get the test method name. TestInfo can be used as a parameter, applied in setup or test methods, but it cannot be declared as a class-level variable.
Your suggestion made me think of a new solution: we can implement a method to retrieve the test method name and register it using the @RegisterExtension annotation. This method will retrieve the current test method name during the execution of @BeforeEach.
There was a problem hiding this comment.
@cnauroth @anujmodi2021 @steveloughran I have improved the code again. Could you please help review this PR?
Thank you very much!
|
🎊 +1 overall
This message was automatically generated. |
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
JIRA: HADOOP-19425. Upgrade JUnit from 4 to 5 in hadoop-azure Part1.
How was this patch tested?
Junit Test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?