Hadoop 18860: Upgrade mockito version to 4.11.0 #6275
+69
−55
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
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
anmolanmol1234
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @steveloughran, I have made changes for shaded client checks that were failing on mockito upgrade. Requesting your review for the same. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
approved these changes
Nov 28, 2023
There was a problem hiding this comment.
lets try again. looking at this PR and how you've had to change all uses of verifyZeroInteractions() reminds me how easy it is in the name of "making things clearer" you end "making upgrades and cherrypicking worse". We should aim to avoid doing the same to others
There was a problem hiding this comment.
actually, I'm trying to understand this.
we changed the scope of the mockito-core dependency to compile.
why do we need this? and why can't it just be marked as "provided" so it doesn't get picked up by everything downstream on what is meant to be a low-dependency component
Have changed the explicit mention of scope as a part of new iteration. Removed the test scope in hadoop-project, so mockito-core artifact now gets recognised in the hadoop-minicluster module |
|
💔 -1 overall
This message was automatically generated. |
saxenapranav
suggested changes
Nov 30, 2023
| @@ -400,6 +400,7 @@ | |||
| <dependency> | |||
| <groupId>org.mockito</groupId> | |||
| <artifactId>mockito-core</artifactId> | |||
| <version>${mockito.version}</version> | |||
There was a problem hiding this comment.
I think we can remove it. Since, hadoop-project is parent of this pom, and in that, there is dependency-management which has this dependency with a version. This would pick that version, if we remove version here.
There was a problem hiding this comment.
yes, it must be removed to stop inconsistencies ever surfacing
| @@ -58,6 +58,10 @@ allowed_expr+="|^org.apache.hadoop.application-classloader.properties$" | |||
| allowed_expr+="|^java.policy$" | |||
| # * Used by javax.annotation | |||
| allowed_expr+="|^jndi.properties$" | |||
| allowed_expr+="|^win32-x86/$" | |||
There was a problem hiding this comment.
mockito-core is an optional dependency in minicluster -> it should not be transitive and also would not be coming into classpath of this project (ref: https://maven.apache.org/guides/introduction/introduction-to-optional-and-excludes-dependencies.html#how-do-optional-dependencies-work). Was curious, what is the reason for these dll coming here.
Would be awesome, if you can add comment for the reasoning.
There was a problem hiding this comment.
same for the other shell file.
hadoop-project/pom.xml
Outdated
| <groupId>org.mockito</groupId> | ||
| <artifactId>mockito-bom</artifactId> | ||
| <version>${mockito.version}</version> | ||
| <type>pom</type> |
There was a problem hiding this comment.
This would import the dependency-management of mockito-bom. From, https://central.sonatype.com/artifact/org.mockito/mockito-bom, looks like it also has mockito-android. Was curious that if the dll files are coming because of this. Also, do we need the bom, since we are adding dependencies for both mockito-core and mockito-inline (because we would be importing many other dependencies which might not be of use).
There was a problem hiding this comment.
be safer to just pull in the ones we know we need
There was a problem hiding this comment.
I agree with the decision to remove the 'mockito-bom' dependency. However, I've encountered an issue where the DLL files are not being included due to the removal of this dependency. The DLL files are originally included as part of the 'byte-buddy-agent' POM, and 'mockito-core' has a dependency on this module. Despite setting the value of 'mockito-core' as optional in 'hadoop-minicluster,' the transitive dependency is still affecting 'hadoop-client-check-test-invariants,' resulting in an error during the check for jar contents due to disallowed DLL values. Consequently, I've addressed this by adding the necessary expressions to allow these DLL files
steveloughran
requested changes
Dec 5, 2023
There was a problem hiding this comment.
I'm with @saxenapranav: we don't want those android dependencies on the classpath
| @@ -69,6 +69,10 @@ allowed_expr+="|^krb5_udp-template.conf$" | |||
| allowed_expr+="|^jetty-dir.css$" | |||
| # Snappy java is native library. We cannot relocate it to under org/apache/hadoop. | |||
| allowed_expr+="|^org/xerial/" | |||
| allowed_expr+="|^win32-x86/$" | |||
| allowed_expr+="|^win32-x86/attach_hotspot_windows.dll$" | |||
There was a problem hiding this comment.
yes, these seem android related. if we keep the android dependencies out, we don't need these
There was a problem hiding this comment.
These are coming from byte-buddy-agent module and not the mockito-bom one.
There was a problem hiding this comment.
Looks like its dlls will come in the minicluster jar. Reason being, mockito-core has a compile-dependency on byte-buddy-agent (so creating jar of minicluster bring in mockito-core, but also byte-buddy-agent by transitivity). The use of mockito-core as compile in minicluster looks like its there to help hadoop-hdfs test-jar run in integeration-test.
Now, mockito-core may need these dlls in the flow defined by -> https://github.com/mockito/mockito/blob/d86bca62a4e9500567ffe97826865460a9c30f18/src/main/java/org/mockito/internal/creation/bytebuddy/InlineDelegateByteBuddyMockMaker.java#L133 -> which creates an object of VirtualMachine. These dlls would be used here: https://github.com/raphw/byte-buddy/blob/master/byte-buddy-agent/src/main/java/net/bytebuddy/agent/VirtualMachine.java#L1269.
These dlls come on the root of the minicluster jar, and the VirtualMachine will be in org.apache.hadoop.shaded.net.bytebuddy.agent.VirtualMachine. The only concern is if the VirtualMachine triggering NativeLibrary API from shaded path can take up the dll on the root or if it has to be relocated to shaded path.
There was a problem hiding this comment.
I have no idea what happens with DLLs and shaded libs. probably messy.
Does byte buddy ever get used? if not, can we exclude it from the base import and so no problems downstream.
There was a problem hiding this comment.
...given what bytebuddy does, I suspect its used a lot...
| @@ -58,6 +58,10 @@ allowed_expr+="|^org.apache.hadoop.application-classloader.properties$" | |||
| allowed_expr+="|^java.policy$" | |||
| # * Used by javax.annotation | |||
| allowed_expr+="|^jndi.properties$" | |||
| allowed_expr+="|^win32-x86/$" | |||
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
Dec 7, 2023
There was a problem hiding this comment.
Right, I'm going to propose something radical here to try and get our mockito upgrades under control.
Add a new method to org.apache.hadoop.test.MockitoUtil, verifyZeroInteractions() which forwards to Mockito.verifyNoInteractions(); test cases which get broken by the upgrade change their import and the code in the test cases doesn't need to change.
This should reduce the cost not just of migrating, but of backporting, because a backported version of the new method could call the older mockito method.
We can do the same in future the next time mockito does another incompatible change
| @@ -400,6 +400,7 @@ | |||
| <dependency> | |||
| <groupId>org.mockito</groupId> | |||
| <artifactId>mockito-core</artifactId> | |||
| <version>${mockito.version}</version> | |||
There was a problem hiding this comment.
yes, it must be removed to stop inconsistencies ever surfacing
|
🎊 +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.
HADOOP-18860: Upgrade mockito version to 4.11.0
Changes made :