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.
|
TimvdLippe
left a comment
There was a problem hiding this comment.
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. |
TimvdLippe
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
| pureMockitoAPIClasses.remove( | ||
| "org.mockito.internal.creation.instance.ObjenesisInstantiator"); | ||
| pureMockitoAPIClasses.remove( | ||
| "org.mockito.internal.creation.instance.UnsafeInstantiatorStrategy"); |
There was a problem hiding this comment.
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.
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.