Rework of injection strategy in the context of modules #3608

raphw · 2025-03-12T12:21:26Z

Makes sure that a module can be injected into, and opens and exports the correct packages when Mockito is used as a named module together with other named modules.

Fixes #3607
Fixes #3612

codecov-commenter · 2025-03-12T12:28:33Z

Codecov Report

Attention: Patch coverage is 63.68715% with 65 lines in your changes missing coverage. Please review.

Project coverage is 86.46%. Comparing base (c81be5d) to head (010102c).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...ito/internal/creation/bytebuddy/ModuleHandler.java	56.94%	58 Missing and 4 partials ⚠️
.../creation/bytebuddy/SubclassBytecodeGenerator.java	88.88%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3608      +/-   ##
============================================
+ Coverage     85.56%   86.46%   +0.90%     
+ Complexity     2957     2952       -5     
============================================
  Files           341      340       -1     
  Lines          9028     8971      -57     
  Branches       1119     1103      -16     
============================================
+ Hits           7725     7757      +32     
+ Misses         1013      934      -79     
+ Partials        290      280      -10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…cess package.

TimvdLippe

There's quite a bit going on here, but I did my best to understand your approach. If I understand correctly, we now use a strategy pattern to determine whether we are dealing with module boundaries or not. Based on that, we choose the appropriate strategy and make the necessary adjustments in the module graph to allow for creating of classes in the module?

That all sounds good to me. I don't have deep knowledge of these systems, so I can't comment on the implementation in detail. That said, I reckon you are one of the folks on the planet who even understands these 😛

Looking at the coverage (for me to figure out how things flow), it seems that parts of https://app.codecov.io/gh/mockito/mockito/pull/3608/blob/mockito-core/src/main/java/org/mockito/internal/creation/bytebuddy/ModuleHandler.java are not used. Most notably exportFromTo and when we can't change the packages. Is it possible to add more regression tests in module-test for these, or are these (near) impossible to recreate? All the uncovered IllegalStateException I don't really mind, given that we are using reflection there.

Nit: Should we use MockitoException wrapping an IllegalStateException?

TimvdLippe · 2025-03-12T19:39:53Z

@scordio Could you also check if the issue you were hitting with AssertJ would be fixed with this PR?

raphw · 2025-03-12T21:46:03Z

I understand why we never reach this branch, and it is because the InlineMockMaker that delegates to the SubclassMockMaker already adds the export. The SubclassMockMaker by itself does however not use instrumentation, so in a way its redundant. It does however yield a proper error when the subclass maker is used with named modules.

I still suggest we keep it that way. Its guarded with a check, so the cost is minimal. Also, the code is easier to read this way, as another reader would wonder, and it makes it more robust against changes in the inline maker.

I'd say this is good with the few additional tests.

TimvdLippe · 2025-03-12T21:57:08Z

Sounds good to me, thanks for the additional explanation and tests. Let's wait till tomorrow to hopefully have input from the other folks if it works in their project and then we can merge. Essentially I expect assertj/assertj#3791 to have a working build again with these fixes.

scordio · 2025-03-13T01:57:11Z

I built this branch locally, but it didn't solve the issues I experienced at assertj/assertj#3791.

The following JVM options helped to fix them:

--add-exports org.mockito/org.mockito.internal.creation.bytebuddy=org.assertj.core
--add-opens org.mockito/org.mockito.internal=java.instrument
--add-opens org.mockito/org.mockito.internal.creation.bytebuddy=hamcrest.core,org.assertj.tests.core

I'm not entirely sure if the first and the last can be prevented, but at least the middle one seems unrelated to the AssertJ codebase (see also assertj/assertj#3791 (comment)).

https://github.com/assertj/assertj/pull/3791/commits

mockito-core/src/main/java/module-info.java

TimvdLippe · 2025-03-14T11:11:20Z

@scordio Thanks for testing! Please retry your PR with the latest commit from Rafael which should resolve the second line (and #3612)

raphw · 2025-03-14T11:22:43Z

I built this branch locally, but it didn't solve the issues I experienced at assertj/assertj#3791.

The following JVM options helped to fix them:
--add-exports org.mockito/org.mockito.internal.creation.bytebuddy=org.assertj.core
--add-opens org.mockito/org.mockito.internal=java.instrument
--add-opens org.mockito/org.mockito.internal.creation.bytebuddy=hamcrest.core,org.assertj.tests.core
I'm not entirely sure if the first and the last can be prevented, but at least the middle one seems unrelated to the AssertJ codebase (see also assertj/assertj#3791 (comment)).

https://github.com/assertj/assertj/pull/3791/commits

Are you using the internal packages in assertj? If so, we'd need to look into creating public APIs around the functionality you are using. We want to keep the internal packages internal, of course.

scordio · 2025-03-14T12:24:53Z

Are you using the internal packages in assertj? If so, we'd need to look into creating public APIs around the functionality you are using. We want to keep the internal packages internal, of course.

I don't think so and, even if we do, it's in test code only and I would argue why.

I'll check and get back!

scordio · 2025-03-14T15:51:30Z

Are you using the internal packages in assertj?

There were some usages, and I removed them at assertj/assertj@fae7ffd8. However, I still experience issues.

Here's the current summary of the test execution of assertj-core-tests.

Mockito 5.16.1-SNAPSHOT (this branch)

I see one remaining error:

java.lang.IllegalAccessError: superinterface check failed: class org.assertj.core.error.AssertionErrorFactory$MockitoMock$EwYQbOOX (in module org.assertj.core) cannot access class org.mockito.internal.creation.bytebuddy.access.MockAccess (in module org.mockito) because module org.assertj.core does not read module org.mockito

Details

org.mockito.exceptions.base.MockitoException: 
Mockito cannot mock this class: interface org.assertj.core.error.AssertionErrorFactory.

If you're not sure why you're getting this error, please open an issue on GitHub.


Java               : 21
JVM vendor name    : Oracle Corporation
JVM vendor version : 21.0.2+13-58
JVM name           : OpenJDK 64-Bit Server VM
JVM version        : 21.0.2+13-58
JVM info           : mixed mode, sharing
OS name            : Mac OS X
OS version         : 14.7.3


You are seeing this disclaimer because Mockito is configured to create inlined mocks.
You can learn about inline mocks and their limitations under item #39 of the Mockito class javadoc.

Underlying exception : java.lang.IllegalArgumentException: Could not create type

	at org.assertj.tests.core/org.assertj.tests.core.internal.failures.Failures_failure_with_AssertionErrorFactory_Test.setUp(Failures_failure_with_AssertionErrorFactory_Test.java:44)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.IllegalArgumentException: Could not create type
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:170)
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:399)
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:190)
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:410)
	at org.mockito/org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator.mockClass(TypeCachingBytecodeGenerator.java:75)
	at org.mockito/org.mockito.internal.creation.bytebuddy.InlineBytecodeGenerator.mockClass(InlineBytecodeGenerator.java:223)
	at org.mockito/org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator.lambda$mockClass$0(TypeCachingBytecodeGenerator.java:78)
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:168)
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:399)
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:190)
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache$WithInlineExpunction.findOrInsert(TypeCache.java:410)
	at org.mockito/org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator.mockClass(TypeCachingBytecodeGenerator.java:75)
	at org.mockito/org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.createMockType(InlineDelegateByteBuddyMockMaker.java:429)
	at org.mockito/org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.doCreateMock(InlineDelegateByteBuddyMockMaker.java:388)
	at org.mockito/org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.createMock(InlineDelegateByteBuddyMockMaker.java:367)
	at org.mockito/org.mockito.internal.creation.bytebuddy.InlineByteBuddyMockMaker.createMock(InlineByteBuddyMockMaker.java:56)
	at org.mockito/org.mockito.internal.util.MockUtil.createMock(MockUtil.java:99)
	at org.mockito/org.mockito.internal.MockitoCore.mock(MockitoCore.java:84)
	at org.mockito/org.mockito.Mockito.mock(Mockito.java:2197)
	at org.mockito/org.mockito.Mockito.mock(Mockito.java:2112)
	... 4 more
Caused by: java.lang.IllegalAccessError: superinterface check failed: class org.assertj.core.error.AssertionErrorFactory$MockitoMock$EwYQbOOX (in module org.assertj.core) cannot access class org.mockito.internal.creation.bytebuddy.access.MockAccess (in module org.mockito) because module org.assertj.core does not read module org.mockito
	at java.base/java.lang.ClassLoader.defineClass0(Native Method)
	at java.base/java.lang.System$2.defineClass(System.java:2394)
	at java.base/java.lang.invoke.MethodHandles$Lookup$ClassDefiner.defineClass(MethodHandles.java:2505)
	at java.base/java.lang.invoke.MethodHandles$Lookup$ClassDefiner.defineClass(MethodHandles.java:2480)
	at java.base/java.lang.invoke.MethodHandles$Lookup.defineClass(MethodHandles.java:1865)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at net.bytebuddy.utility.Invoker$Dispatcher.invoke(Unknown Source)
	at net.bytebuddy@1.17.1/net.bytebuddy.utility.dispatcher.JavaDispatcher$Dispatcher$ForNonStaticMethod.invoke(JavaDispatcher.java:1033)
	at net.bytebuddy@1.17.1/net.bytebuddy.utility.dispatcher.JavaDispatcher$ProxiedInvocationHandler.invoke(JavaDispatcher.java:1163)
	at jdk.proxy2/jdk.proxy2.$Proxy96.defineClass(Unknown Source)
	at net.bytebuddy@1.17.1/net.bytebuddy.dynamic.loading.ClassInjector$UsingLookup.injectRaw(ClassInjector.java:1686)
	at net.bytebuddy@1.17.1/net.bytebuddy.dynamic.loading.ClassInjector$AbstractBase.injectRaw(ClassInjector.java:167)
	at net.bytebuddy@1.17.1/net.bytebuddy.dynamic.loading.ClassInjector$AbstractBase.inject(ClassInjector.java:155)
	at net.bytebuddy@1.17.1/net.bytebuddy.dynamic.loading.ClassLoadingStrategy$UsingLookup.load(ClassLoadingStrategy.java:519)
	at net.bytebuddy@1.17.1/net.bytebuddy.dynamic.TypeResolutionStrategy$Passive.initialize(TypeResolutionStrategy.java:101)
	at net.bytebuddy@1.17.1/net.bytebuddy.dynamic.DynamicType$Default$Unloaded.load(DynamicType.java:6432)
	at org.mockito/org.mockito.internal.creation.bytebuddy.SubclassBytecodeGenerator.mockClass(SubclassBytecodeGenerator.java:280)
	at org.mockito/org.mockito.internal.creation.bytebuddy.TypeCachingBytecodeGenerator.lambda$mockClass$0(TypeCachingBytecodeGenerator.java:78)
	at net.bytebuddy@1.17.1/net.bytebuddy.TypeCache.findOrInsert(TypeCache.java:168)
	... 23 more

I'll dig more to figure out the workaround.

raphw · 2025-03-14T15:53:14Z

That must be a different or outdated branch as I moved the mentioned class to a different package.

scordio · 2025-03-14T16:08:01Z

@raphw sorry, I wrote the previous comment misleadingly 🙂 please recheck it.

I also deleted all the details about 5.16.0, as you solved them anyway. Sorry for the noise.

scordio · 2025-03-14T16:16:48Z

I see one remaining error:

java.lang.IllegalAccessError: superinterface check failed: class org.assertj.core.error.AssertionErrorFactory$MockitoMock$EwYQbOOX (in module org.assertj.core) cannot access class org.mockito.internal.creation.bytebuddy.access.MockAccess (in module org.mockito) because module org.assertj.core does not read module org.mockito

...

I'll dig more to figure out the workaround.

Adding:

--add-reads org.assertj.core=org.mockito

solves the issue.

I wonder if this can also be solved on Mockito side as I wouldn't expect the org.assertj.core module descriptor to declare dependencies for testing (that's instead done in the org.assertj.tests.core module descriptor).

TimvdLippe · 2025-03-15T15:18:06Z

@raphw Do I understand correctly from Scordio's comments that this PR is ready to be merged and we can publish a new patch version? Or are we missing a fix for the issues mentioned?

scordio · 2025-03-15T15:22:53Z

Having to add:

--add-reads org.assertj.core=org.mockito

seems like an unresolved issue to me, but happy to hear your opinion 🙂

raphw · 2025-03-15T15:52:26Z

There's one more fix coming for that.

TimvdLippe · 2025-03-15T16:24:52Z

All right, cool. No rush!

scordio · 2025-03-15T19:03:15Z

@raphw all AssertJ tests are now green, no --add-reads required 👍

raphw · 2025-03-15T19:18:01Z

I made a last adjustment. Could you give it a last run?

scordio · 2025-03-15T19:22:08Z

All good 👍

raphw · 2025-03-15T19:23:11Z

then this is ready to be merged @TimvdLippe

| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [org.mockito:mockito-core](https://github.com/mockito/mockito) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `5.16.0` -> `5.16.1` | | [org.junit.jupiter:junit-jupiter-params](https://junit.org/junit5/) ([source](https://github.com/junit-team/junit5)) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `5.12.0` -> `5.12.1` | | [org.junit.jupiter:junit-jupiter-engine](https://junit.org/junit5/) ([source](https://github.com/junit-team/junit5)) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `5.12.0` -> `5.12.1` | | [org.junit.jupiter:junit-jupiter-api](https://junit.org/junit5/) ([source](https://github.com/junit-team/junit5)) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `5.12.0` -> `5.12.1` | | [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.0` -> `2.31.1` | | [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.0` -> `2.31.1` | | [software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.0` -> `2.31.1` | | [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.0` -> `2.31.1` | | [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.0` -> `2.31.1` | | [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.0` -> `2.31.1` | | [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.0` -> `2.31.1` | --- ### Release Notes <details> <summary>mockito/mockito (org.mockito:mockito-core)</summary> ### [`v5.16.1`](https://github.com/mockito/mockito/releases/tag/v5.16.1) <sup><sup>*Changelog generated by [Shipkit Changelog Gradle Plugin](https://github.com/shipkit/shipkit-changelog)*</sup></sup> ##### 5.16.1 - 2025-03-15 - [3 commit(s)](mockito/mockito@v5.16.0...v5.16.1) by Adrian Roos, Jérôme Prinet, Rafael Winterhalter - Remove Arrays.asList from critical stubbing path in GenericMetadataSu… [(#3610)](mockito/mockito#3610) - Rework of injection strategy in the context of modules [(#3608)](mockito/mockito#3608) - Adjust inline mocking snippet to allow task relocatability [(#3606)](mockito/mockito#3606) - Inline mocking configuration snippet for Gradle should allow task relocatability [(#3605)](mockito/mockito#3605) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Never, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 1939516a6a445b5c4812a6d4f27f483cdb66b66e

raphw added 20 commits March 11, 2025 13:07

Move Mock Access to dedicated package to thin out export.

e74ef72

Add desired state to test to demonstrate solution.

bdd3637

Add concept of module opener.

6f00e9e

Clean up comment.

96295df

Remove module opener.

488b230

Disable module handler for now.

aed7cde

Adjust module handler.

9d0b5f3

Remove injection base.

6470e3a

Complete handler.

ab8917c

Remove internal references.

8a2dc23

Remove impossisble branch.

4343bdf

Reintroduce injection base.

343e620

Choose right strategy.

1879864

Improve error message and remove redundant test.

2b5e567

Clean up.

2bd5101

Clean up.

9f853f2

Move package to internal.

96380c8

Avoid internal package.

c949e73

Add mock loader.

Loading
Loading status checks…

03efb42

Remove import.

19ca50d

raphw mentioned this pull request Mar 12, 2025

java.lang.IllegalStateException: java.lang.IllegalAccessError: superinterface check failed #3607

Closed

Add weak reference cache for class loaders.

Loading
Loading status checks…

ba999d0

raphw added 5 commits March 12, 2025 13:29

Correct visibility for interceptor type.

Loading
Loading status checks…

06e1e94

Adjust package structure to expose mock-included API fully through ac…

Loading
Loading status checks…

7688eda

…cess package.

Fix codestyle.

Loading
Loading status checks…

1862f84

Fix test.

Loading
Loading status checks…

e1c36c6

Fix other test.

Loading
Loading status checks…

5dc3838

TimvdLippe approved these changes Mar 12, 2025

View reviewed changes

TimvdLippe mentioned this pull request Mar 14, 2025

Build fails when testing under module path (Regression in 5.16.0) #3612

Closed

Open premain method to JDK instrumentation engine.

Loading
Loading status checks…

059b115

TimvdLippe reviewed Mar 14, 2025

View reviewed changes

mockito-core/src/main/java/module-info.java Show resolved Hide resolved

raphw added 3 commits March 15, 2025 19:35

Reorder exports.

c3b6f33

Add reads for target module.

Loading
Loading status checks…

260bb46

Remove unused methods.

Loading
Loading status checks…

644d6c6

Fix module reading.

Loading
Loading status checks…

010102c

TimvdLippe merged commit d000e63 into main Mar 15, 2025
18 checks passed

TimvdLippe deleted the named-module-subclass branch March 15, 2025 19:27

This was referenced Mar 17, 2025

Bump org.mockito:mockito-junit-jupiter from 5.16.0 to 5.16.1 kousen/java_8_recipes#128

Merged

Bump mockito from 5.16.0 to 5.16.1 kousen/junit5_workshop#253

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework of injection strategy in the context of modules #3608

Rework of injection strategy in the context of modules #3608

raphw commented Mar 12, 2025 •

edited by TimvdLippe

Loading

codecov-commenter commented Mar 12, 2025 •

edited

Loading

TimvdLippe left a comment

TimvdLippe commented Mar 12, 2025

raphw commented Mar 12, 2025

TimvdLippe commented Mar 12, 2025

scordio commented Mar 13, 2025 •

edited

Loading

TimvdLippe commented Mar 14, 2025

raphw commented Mar 14, 2025

scordio commented Mar 14, 2025

scordio commented Mar 14, 2025 •

edited

Loading

raphw commented Mar 14, 2025

scordio commented Mar 14, 2025 •

edited

Loading

scordio commented Mar 14, 2025 •

edited

Loading

TimvdLippe commented Mar 15, 2025

scordio commented Mar 15, 2025

raphw commented Mar 15, 2025

TimvdLippe commented Mar 15, 2025

scordio commented Mar 15, 2025

raphw commented Mar 15, 2025

scordio commented Mar 15, 2025

raphw commented Mar 15, 2025

Rework of injection strategy in the context of modules #3608

Rework of injection strategy in the context of modules #3608

Conversation

raphw commented Mar 12, 2025 • edited by TimvdLippe Loading

codecov-commenter commented Mar 12, 2025 • edited Loading

Codecov Report

TimvdLippe left a comment

Choose a reason for hiding this comment

TimvdLippe commented Mar 12, 2025

raphw commented Mar 12, 2025

TimvdLippe commented Mar 12, 2025

scordio commented Mar 13, 2025 • edited Loading

TimvdLippe commented Mar 14, 2025

raphw commented Mar 14, 2025

scordio commented Mar 14, 2025

scordio commented Mar 14, 2025 • edited Loading

Mockito 5.16.1-SNAPSHOT (this branch)

raphw commented Mar 14, 2025

scordio commented Mar 14, 2025 • edited Loading

scordio commented Mar 14, 2025 • edited Loading

TimvdLippe commented Mar 15, 2025

scordio commented Mar 15, 2025

raphw commented Mar 15, 2025

TimvdLippe commented Mar 15, 2025

scordio commented Mar 15, 2025

raphw commented Mar 15, 2025

scordio commented Mar 15, 2025

raphw commented Mar 15, 2025

raphw commented Mar 12, 2025 •

edited by TimvdLippe

Loading

codecov-commenter commented Mar 12, 2025 •

edited

Loading

scordio commented Mar 13, 2025 •

edited

Loading

scordio commented Mar 14, 2025 •

edited

Loading

scordio commented Mar 14, 2025 •

edited

Loading

scordio commented Mar 14, 2025 •

edited

Loading