Switch the default mockmaker to the inline mockmaker on JDK 17+

[TimvdLippe]

All right, we have been talking about this for a while so let's consolidate discussions. TLDR: more and more use cases are broken (by default) with Mockito and JDK 17. That's because the subclass mockmaker runs into fundamental limitations on JDK 17, but the inline mockmaker works as expected.

There are a lot of issues related to this:

• Spying on interfaces with default methods does not work anymore with JDK 17 (Temurin) #2587
• mock(SecureRandom.class) and mock(Random.class) fails #2560
• Spies don't behave properly when running in Java 17 #2573
• Mockito silently produces incorrect mocks of jdk classes with java 17 #2561 (suspected)
• Cannot execute any tests in JDK17 envir. Error: "NoSuchMethodException: sun.misc.Unsafe.defineClass(...) " #2521 (suspected)
• InjectMocks and Spy annotations are no more working in Java 17 #2449 (suspected)

At this point, the rate of issues created is increasing, as more and more of our users are adopting JDK 17+

We have historically talked about this as well: #1728

When chatting with @raphw about this issue, there were a couple of options that we have available:

1. Keep on using the subclass mockmaker by default on all JDK's and suggest users to use mockito-inline whenever appropriate
2. Switch the default to the inline mockmaker for all JDK's and put the subclass mockmaker in an artifact such as mockito-subclass (legacy)
3. Detect the current JDK version and make the inline mockmaker the default on JDK 17+, but keep the subclass mockmaker the default for older JDK versions

In my opinion, we should opt for least amount of surprise. E.g. Mockito should "just" work out-of-the-box for the majority of use cases. Therefore, if Mockito (by default) does not work nicely with JDK 17 in more and more cases, I consider that a bug.

That said, I don't think it is a good choice to force all existing users that are on JDK 16 and below to update their codebase wholesale to the new mockmaker. Inheritently, the new mockmaker works slightly different and forcing the update onto users doesn't feel like a nice solution. In the end, it works just fine on JDK 16 on below, so why change it?

Therefore, my proposed way forward is:

1. We publish Mockito 5 which switches the default mockmaker to the inline mockmaker on JDK 17+
2. We update the JavaDoc of the subclass mockmaker to make users aware it is only intended to be used for specific use cases

In terms of timeline, I think we should collective user feedback from the community first. This issue seems like the best place to do so. Once we are confident that we would be able to switch the default, we could publish Mockito 5. For now, what if we take June 2022 as a temporary date to make such a change, unless we receive strong/valid user feedback that should block such a change?

CC @mockito/developers

[raphw]

Fully agree with it but with (2). I think we should keep it as it can function well if the JVM is setup correctly. I am for example currently working with some adjustments that will make Mockito work on Graal AOT. This will only work with the subclass mock maker.

[TimvdLippe]

Do I understand correctly you would prefer to switch the default for all JDK's, but ship both mockmakers as they target separate use cases? E.g. we don't deprecate the subclass mockmaker, but we maintain the two synonymous and we make the default experience inline, but we allow the subclass mockmaker for JVMs such as Graal.

[bric3]

I am personally in favor of option (1).
In my opinion as long as there are use cases for the subclassing strategy (eg android), it shouldn't be abandoned. Even ignoring GraalVM native or Android, I don't think dropping the subclass strategy would be appropriate as long as Mockito base line is not JDK 17.

[TimvdLippe]

Thanks for your input! Dropping the subclass mockmaker is off-the-table given both Graal and Android will need, so I removed that from the plan.

Still, while we don't have JDK 17 as a baseline, I don't think we should ship a broken-by-default experience on these newer JDK's. Therefore, I would prefer tackling this issue even before we officially adopt JDK 17 as our minimum version (which is a long time off).

@bric3 would plan 2 be appropriate for you, with the given addition that we do NOT drop the subclass mockmaker? E.g. we switch the default on JDK 17+, but we keep vendoring both mockmakers as standalone packages so that users who need can choose the most appropriate one.

[bric3]

@bric3 would plan 2 be appropriate for you, with the given addition that we do NOT drop the subclass mockmaker? E.g. we switch the default on JDK 17+, but we keep vendoring both mockmakers as standalone packages so that users who need can choose the most appropriate one.

Yes, I think it's the sensible choice.

@raphw Is this one of the relevant PR : openjdk/jdk#3546

[raphw]

Yes, that is what I wanted to suggest. Let's keep the mock makers in one artifact, but let us also have a smart default choice in case that no explicit mock maker is chosen.

I think this is the right thing: default to inline mock maker in Java 17+, subclass mock maker in Java 16-. And also the subclass mock maker if a Graal AOT image code is found. I currently am working with Oracle to adjust Byte Buddy and Mockito to work with Graal AOT. I hope to merge and publish this in a week or two.

[TimvdLippe]

Cool! I think we are all in agreement then on the proposed way forward. I will write up a wiki page for users that we can point to, so that we can collect feedback and ask users to test out mockito-inline.

[JojOatXGME]

I think it would also be nice if you can disable the inline mock maker for specific mocks. I think it is good if users can switch to the inline mock maker as early as possible. However, sometimes existing code does not work due to some strange cases. For example, I encountered one case where the code did only work because final methods (including equals and hashCode) were not mocked. Allowing users to enable the inline mock maker by default but keeping the old one in specific cases for some time could simplify the migration.

Maybe you can introduce something like MockSettings.createSubclass().

[TimvdLippe]

Hm, that's a fair point, but I think it is going to be difficult for us, given that mockmakers are globally applied:

mockito/src/main/java/org/mockito/internal/util/MockUtil.java

Line 28 in 737f0a3

private static final MockMaker mockMaker = Plugins.getMockMaker();

We could indeed update the MockCreationSettings to add a new field that allows users to supply a MockMaker and we use that instead of the globally defined one (here

mockito/src/main/java/org/mockito/internal/util/MockUtil.java

Line 44 in 737f0a3

mockMaker

). @JojOatXGME can you file that as a separate feature request and, if possible, send us a WIP PR that would show whether it is feasible to do so?

[JojOatXGME]

I created #2626 about that topic. I am not sure if I find time to create a PR within the next few weeks, but lets see.

[TimvdLippe]

@raphw Shall we pick this up soon so that we can prepare for a new major version around September? Given that more and more companies are switching to JDK 17, I think this is the right moment to make the change.

[raphw]

Yeah, I think it's hard to estimate the actual impact before it's out so the longer we wait, the more Java 17 is adopted and the more big bang this will get.

[antch]

Switching to mockito-inline caused our test suite to take ~2.5x the previous runtime (~20m -> ~50m). In reality the non-inline mock-maker is fine for the vast majority of cases -- is there a potential for a per-use, opt-in mechanism? I'm having a similar issue to #2628 in Java 17, except with FileOutputStream... unfortunately it's far more harmful to upgrade to the inline mock maker than to completely rework (or even just delete) the failing test. I'm very worried about eventually having to eat that huge performance hit in order to upgrade Mockito.

Interesting note, even tests which do not use Mockito at all ballooned in runtime.

[raphw]

You won't have to eat the performance hit because of Mockito, you can certainly pin the subclass mock maker, but the unsafe APIs are going to be deleted and with it, the subclass Mock maker will have to call constructors or break at some point. The JDK will force you to eat the hit, therefore.

Try setting up Surefire to allow self attach, or even add the Byte Buddy agent on the VM startup.

Other than that, the inline mock maker is more expensive due to its design. Nothing we can do about it, we have to go via instrumentation for it.

[antch]

Thank you for the response -- when you reference "Surefire self attach" and "Byte Buddy agent", do you mean that those would be used to try to troubleshoot the runtime performance?

[raphw]

Yes, it's mentioned in the Mockito javadoc in greater detail.