-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support subclass mocks on Graal VM. #2613
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2613 +/- ##
============================================
- Coverage 86.62% 86.27% -0.35%
+ Complexity 2762 2761 -1
============================================
Files 314 314
Lines 8237 8268 +31
Branches 1017 1026 +9
============================================
- Hits 7135 7133 -2
- Misses 842 869 +27
- Partials 260 266 +6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't had the chance to look at the implementation yet, but would it be possible to add a mockito-graal
test project to our regression suite? That would help me with reviewing the implementation as well. Thanks!
I looked at it, but this would require running on Graal and installing the native extension. I am not sure how realistic this is and I would want to avoid downloading a GB for each build. It also requires two execution circles. For those reasons, I have decided against it in Byte Buddy and rather rely on manual tests so long as the general setup is still autotested. I was hoping that the Graal ecosystem spawns some more mature testing utilities in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Only a couple of nits and clarifying questions. Other than that LGTM!
GraalImageCode.getCurrent().isDefined() | ||
&& !features.interfaces.contains(Serializable.class) | ||
? CompoundList.of( | ||
new ArrayList<Type>(features.interfaces), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can we maybe use a Set
here, so that we can skip the contains check for Serializable
? E.g. on Graal we always add Serializable
to the Set
and then we create the CompoundList
of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to replace it with a sorted set, actually as a different order would cause a different class file and hash and fail the Graal run.
"%s$%s$%s", | ||
typeName, | ||
"MockitoMock", | ||
GraalImageCode.getCurrent().isDefined() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific reason we only want this on Graal? I can imagine we would want this for others as well, to keep everything consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The very theoretical situation would be a OSGi setup where multiple modules of Mockito wanted to create a mock for the same class loader. It's unlikely but might trigger a regression. The question is if the stable name justifies this theoretical issue.
@@ -35,6 +33,8 @@ public void pure_mockito_should_not_depend_bytecode_libraries() throws Exception | |||
"org.mockito.internal.creation.instance.DefaultInstantiatorProvider"); | |||
pureMockitoAPIClasses.remove( | |||
"org.mockito.internal.creation.instance.ObjenesisInstantiator"); | |||
pureMockitoAPIClasses.remove( | |||
"org.mockito.internal.creation.instance.UnsafeInstantiatorStrategy"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think we no longer need this, as this particular class isn't included in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this stems from an intermediate version to work around something that is now fixed in Graal.
d95060f
to
1a9cbfe
Compare
Adds support for running Mockito on Graal native image using the (still experimental) experimental-class-define-support. Byte Buddy now picks up the Graal image code and creates more predicatable class files such that hashes during the JIT runtime and the image runtime match.
This leads to some minor restrictions, but generally this should not be a big issue. With future improvements in Graal, these restrictions might fade in the future.
As an example, with the mockto.jar that is produced in this PR:
The setup is still a bit cluncky but Oracle is working on improving the process. There will likely be plugins for the tools in question to improve this.