HADOOP-19425. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-azure Part3. #7674
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. |
slfan1989
force-pushed
the
HADOOP-19425-PART3
branch
from
d45cef6 to
0cc111f
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
force-pushed
the
HADOOP-19425-PART3
branch
from
9098424 to
f518fa5
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
force-pushed
the
HADOOP-19425-PART3
branch
from
f518fa5 to
f88fb9f
Compare
|
This PR is ready for review. I understand there might be questions about why the change set is relatively large, so here's some context: the main focus of this PR is on test classes that extend @anujmodi2021 @cnauroth Could you please help review this PR? Thank you very much! |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@slfan1989 Thanks for the patch. Update 1: Found the issue. Most of these classes are inheriting Update 2: Similar issue in |
anujmodi2021
requested changes
Jul 14, 2025
| import static org.apache.hadoop.fs.azure.integration.AzureTestUtils.assumeScaleTestsEnabled; | ||
|
|
||
| /** | ||
| * Integration tests at bigger scale; configurable as to | ||
| * size, off by default. | ||
| */ | ||
| @Timeout(value = SCALE_TEST_TIMEOUT_MILLIS, unit = TimeUnit.MILLISECONDS) |
There was a problem hiding this comment.
Is it newly getting introduced?
| @@ -178,14 +170,35 @@ public static Iterable<Object[]> params() { | |||
| }); | |||
| } | |||
|
|
|||
| public ITestAbfsCustomEncryption() throws Exception { | |||
| public void initITestAbfsCustomEncryption(EncryptionType pFileEncryptionType, | |||
There was a problem hiding this comment.
This seems like too much work around in Junit5.
I saw a discussion that we are planning to simplify this with an updated version of Junit5. Should we try to add some TODOs so that we know where all we need to simplify Parameterised tests so that we don't skip any?
| import static org.mockito.ArgumentMatchers.nullable; | ||
| import static org.mockito.Mockito.anyBoolean; |
There was a problem hiding this comment.
Is this change also part of this upgrade?
| import static org.apache.hadoop.fs.azure.integration.AzureTestUtils.assumeScaleTestsEnabled; | ||
|
|
||
| /** | ||
| * Integration tests at bigger scale; configurable as to | ||
| * size, off by default. | ||
| */ | ||
| @Timeout(value = SCALE_TEST_TIMEOUT_MILLIS, unit = TimeUnit.MILLISECONDS) | ||
| public class AbstractAbfsScaleTest extends AbstractAbfsIntegrationTest { |
There was a problem hiding this comment.
setup method of this class should also be annotated with @beforeeach.
This is causing some test failures.
| @@ -74,7 +74,7 @@ public class ITestAbfsDelegationTokens extends AbstractAbfsIntegrationTest { | |||
| /*** | |||
| * Set up the clusters. | |||
| */ | |||
| @BeforeClass | |||
| @BeforeAll | |||
There was a problem hiding this comment.
setup() and teardown() in this class also need to be annotated
|
Some asserts in |
|
There are a few tests in folowing classes failing on assert because the parameter order was missed changing.
Some more test failures are there. Will try to debug as soon as I get some more time. [ERROR] org.apache.hadoop.fs.azurebfs.ITestAzureBlobFileSystemInitAndCreate.testFileSystemInitializationFailsForInvalidIngress Time elapsed: 0.001 s <<< ERROR! |
|
@anujmodi2021 Thank you very much for reviewing my code! Your suggestions and feedback are extremely helpful. I'm going through them carefully and will work on addressing these issues in the next version. Thanks again for your support. |
|
🎊 +1 overall
This message was automatically generated. |
|
@slfan1989 - Should we consider upgrading to Junit 5.13.1 where class level parameterization is available. We can solve a lot of problem due to junit 5 migration in hadoop-aws module |
Xref https://issues.apache.org/jira/browse/HADOOP-19529 / https://issues.apache.org/jira/browse/HADOOP-19608 / #7785 |
|
@anujmodi2021 @shameersss1 @h-vetinari Sorry for the delayed response — I missed some key information earlier. I really appreciate your attention to this PR and the valuable suggestions you provided. I completely agree with your points. Once #7785, #7814 is merged, I’ll revise and clean up this PR accordingly. Thanks again for your support! |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
slfan1989
force-pushed
the
HADOOP-19425-PART3
branch
from
7cad605 to
40e553c
Compare
|
After rebasing the code on the latest trunk, the unit tests have been comprehensively upgraded to use JUnit 5. The key improvements and adjustments are as follows:
All test methods using @timeout now explicitly specify the unit parameter to preserve the original time unit (milliseconds via TimeUnit.MILLISECONDS), ensuring consistent behavior without altering existing logic.
Issues caused by incorrect parameter ordering in assertEquals assertions during migration have been manually reviewed and corrected to ensure accurate test logic and prevent false negatives.
Legacy org.junit.Assume statements have been replaced with JUnit 5's org.junit.jupiter.api.Assumptions.assumeTrue, bringing assumption-based tests into alignment with the JUnit 5 framework.
Introduced the new JUnit 5 annotation @ParameterizedClass to refactor parameterized tests. This preserves the original structure as much as possible while significantly improving readability and maintainability.
The hadoop-azure module has fully removed its dependency on JUnit 4. All upgraded tests now compile and run successfully under JUnit 5 alone. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@anujmodi2021 I've fixed all the issues you pointed out. Could you please help review this PR again? Much appreciated! After rebasing the code on the latest trunk, the unit tests have been comprehensively upgraded to use JUnit 5. The key improvements and adjustments are as follows:
All test methods using @timeout now explicitly specify the unit parameter to preserve the original time unit (milliseconds via TimeUnit.MILLISECONDS), ensuring consistent behavior without altering existing logic.
Issues caused by incorrect parameter ordering in assertEquals assertions during migration have been manually reviewed and corrected to ensure accurate test logic and prevent false negatives.
Legacy org.junit.Assume statements have been replaced with JUnit 5's org.junit.jupiter.api.Assumptions.assumeTrue, bringing assumption-based tests into alignment with the JUnit 5 framework.
Introduced the new JUnit 5 annotation @ParameterizedClass to refactor parameterized tests. This preserves the original structure as much as possible while significantly improving readability and maintainability.
The hadoop-azure module has fully removed its dependency on JUnit 4. All upgraded tests now compile and run successfully under JUnit 5 alone. |
|
In PR #7858, Steve suggested using AssertJ instead of JUnit 5, which I believe is a very reasonable approach. |
Thanks a lot for taking in my suggestions and fixes. |
|
🎊 +1 overall
This message was automatically generated. |
Thank you very much for reviewing the code and running the tests! I hope the test results are successful. |
anujmodi2021
approved these changes
Aug 9, 2025
|
@anujmodi2021 Thank you very much for helping review the code! Merged into trunk. |
| Assume.assumeFalse( | ||
| "SAS Based Authentication Not Allowed For Integration Tests", | ||
| currentAuthType == AuthType.SAS); | ||
| assumeThat(currentAuthType). |
There was a problem hiding this comment.
This is working as assumeTrue but should have been working as assumeFalse.
This is leading to all Integration tests getting skipped.
Will fix this with https://issues.apache.org/jira/browse/HADOOP-19649
There was a problem hiding this comment.
I have received your message and I’m sorry for any inconvenience caused. I will do my best to assist you, and you can reach out to me anytime if you need any help.
| getAuthType() == AuthType.OAuth); | ||
| assumeThat(getAuthType()) | ||
| .as("ITestOauthOverAbfsScheme is skipped because auth type is not OAuth") | ||
| .isNotEqualTo(AuthType.OAuth); |
There was a problem hiding this comment.
This also need fix. Will take up with https://issues.apache.org/jira/browse/HADOOP-19649
| @@ -67,8 +67,9 @@ public void testExpect100ContinueHandling() throws Exception { | |||
| AzureBlobFileSystem fs = getFileSystem(); | |||
| Path path = new Path("/testExpect100ContinueHandling"); | |||
| if (isAppendBlobEnabled()) { | |||
| Assume.assumeFalse("Not valid for AppendBlob with blob endpoint", | |||
| getIngressServiceType() == AbfsServiceType.BLOB); | |||
| assumeThat(getIngressServiceType()) | |||
There was a problem hiding this comment.
This also need fixing.
|
Hi @slfan1989 I will be fixing those as part of JIRA: https://issues.apache.org/jira/browse/HADOOP-19649 |
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. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-azure Part3.
How was this patch tested?
mvn clean test & junit test.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?