Codecov Report

❌ Patch coverage is 71.28713% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.50%. Comparing base (756a3cf) to head (97d27c3).
⚠️ Report is 15 commits behind head on main.

@@             Coverage Diff              @@
##               main    #3762      +/-   ##
============================================
+ Coverage     86.46%   86.50%   +0.04%     
- Complexity     2988     3004      +16     
============================================
  Files           341      343       +2     
  Lines          9041     9099      +58     
  Branches       1113     1121       +8     
============================================
+ Hits           7817     7871      +54     
- Misses          942      945       +3     
- Partials        282      283       +1     

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

Error: Java setup failed due to network issue or timeout: error code: 500

CI actions look to be failing due to network issues. I will try to repush later

Just a friendly bump on this PR - would love to get feedback on this

My initial reaction would be to not introduce language specific code into mockito-core. I understand that there is no inherent dependency on Kotlin code, but I also don't see a particular usage in a non-Kotlin context.

It seems like this API goes against the idea of static mocks in mockito-core. Namely that they are stubbed in a particular scope, thus shouldn't leak beyond their initialization. Now, with these settings, it would continue to overwrite these methods by left-over state. That seems like a safety hazard that I don't think is desirable.

Looking at the linked issue, the solution as mentioned makes sense to me:

We found a way to make stubbing companion object methods work by using mockConstruction:

Is there a particular reason why you would want a different solution than the one proposed?

@TimvdLippe thanks for your reply.

My initial reaction would be to not introduce language specific code into mockito-core. I understand that there is no inherent dependency on Kotlin code, but I also don't see a particular usage in a non-Kotlin context.

This makes sense, and if I could implement this in mockito-kotlin alone I would, but I don't see how that's possible without a new API like this in core.

I will say there are existing Kotlin specific things in internals like:
https://github.com/mockito/mockito/blob/df3e0ccdd42533ac933f87e3fa00c0681d362c5b/mockito-core/src/main/java/org/mockito/internal/util/KotlinInlineClassUtil.java
https://github.com/mockito/mockito/blob/df3e0ccdd42533ac933f87e3fa00c0681d362c5b/mockito-core/src/main/java/org/mockito/internal/creation/SuspendMethod.java

Maybe we can find a way to hide this feature from the public "settings" API so it's not really exposed to end users.

It seems like this API goes against the idea of static mocks in mockito-core. Namely that they are stubbed in a particular scope, thus shouldn't leak beyond their initialization. Now, with these settings, it would continue to overwrite these methods by left-over state. That seems like a safety hazard that I don't think is desirable.

Unless I misunderstand you, I don't see how stubbing leaks outside of the MockedStatic scope. When that closeable handle is closed, all static and instance stubbing on the class is removed.

Looking at the linked issue, the solution as mentioned makes sense to me:

We found a way to make stubbing companion object methods work by using mockConstruction:

Is there a particular reason why you would want a different solution than the one proposed?

Yes, it turns out that using mockConstruction as a solution to mock Kotlin singletons is actually quite brittle for several reasons:

• It requires that the Kotlin singleton has not yet been classloaded. So a test author must be careful to avoid initializing the singleton prior to intercepting the companion construction (which can happen easily if, say, you mock Foo before trying to mockConstruction(Foo.Companion.class))
• Mocking state leaks across test cases. Because the singleton instance is a static field, using mockConstruction() to intercept construction means that every other test case in the test binary uses that mock (with all its stubbing), which might not be desirable.

That's why I'm looking for a Mockito-native, properly-scoped solution to this problem

To follow up, is the response here a "definitely no" or "maybe with changes we could allow it"

For what it's worth, I think a "mockObject" for Kotlin is a big feature gap comparing Mockito to MockK, and this could help improve Mockito's reputation in the Kotlin community.

Sorry, it's December time and I am swamped with (open source) work atm. My response was a question. Based on your answer, I agree that this is a problem worth solving. I haven't read up more about the context and therefore am unsure if your proposed solution is the right one.

Unfortunately I don't know when I have sufficient time and headspace to look into these details. The main question I have is: why mirror the creation settings structure and limit it to static mocks? Why can't we unify these, or can we design a different structure that is agnostic of whether a mock is static or not and would work for all mocks?

So all in all, I am happy to see your contribution and eagerness to solve this problem. I think it is a valid problem and prefer to have a design review to tackle the above questions.

Thanks, and I understand the end-of-year push for everything.

The main question I have is: why mirror the creation settings structure and limit it to static mocks? Why can't we unify these, or can we design a different structure that is agnostic of whether a mock is static or not and would work for all mocks?

My intention was to build off the mockStatic implementation because we are doing a similar thing of intercepting methods an a class that was not instantiated as a mock(), and keeping an associated closeable scope. But I'm not attached to this approach. My first prototype was to maintain a separate map (similar to staticInterceptors) with a separate API entry point (say, mockSingleton(T.class) - and we could hide this in internal package). I ended up switching to this PR's approach because it felt like too much duplicated code, but I could also do some refactoring to mitigate that too.

@TimvdLippe I just saw your post about stepping down as a maintainer. I just wanted to say thank you for your leadership, for cultivating such an important part of the JVM ecosystem, and for working with me on the handful of PRs recently. Best of luck on your current and future projects!

Thank you! And do keep on contributing, I will stay on until March. Unfortunately this PR is too big of a design that isn't well-timed given the transition, so I am hopeful a future maintainer can help you further. Apologies for the delay.

@TimvdLippe Since, as far as I can tell, there is no discussion about who will be future maintainers, it seems like it may be some time off. What's my best course of action for this PR?

I could attempt to retrofit this onto the mockito-kotlin sources to mitigate concerns of Kotlin stuff leaking into core, though I think the implementation would be hacky and not great in the long term.

To be honest, I don't have a clue how best to proceed here. Apologies for the inconvenience. @mockito/developers can you help Joshua figure out a path forward?

My general feedback regarding this approach of hardcoding Kotlin-specific code in mockito-core still stands. However, I don't have an alternative and I don't feel like I am in a position right now to help figure out an alternative. Therefore, I will leave it up to you folks to make a decision on how to move forward.

I will continue to follow up, but I have no capacity to actively track the issues. I will however make sure that Mockito works on future JVMs. I can also help review this ticket.

I looked at this now, and to me it also does not feel quite right and I would like to explore other options. Basically, the problem is that you want to instrument methods of an instance that is the singleton, but the object is already created, is that correct?

Does Kotlin not expose the singleton through a field, not a method? Maybe this could be a good access point?

Basically, the problem is that you want to instrument methods of an instance that is the singleton, but the object is already created, is that correct?

Yes, exactly.

Does Kotlin not expose the singleton through a field, not a method?

The underlying field that stores the singleton is not normally accessible, even via Kotlin reflection. The Kotlin compiler stores the instance as a private static final field.

The way we currently hack around this in our tests to support a "mockObject" is to use the sun.misc.Unsafe API to reassign the instance field to a mock. But, these APIs have been deprecated in JDK 18 and marked for removal in a future release. Based on https://openjdk.org/jeps/471, the VarHandle API is intended to replace these but it seems this will not work for final fields:

If a VarHandle references a read-only variable (for example a final field) then write, atomic update, numeric atomic update, and bitwise atomic update access modes are not supported and corresponding methods throw UnsupportedOperationException

For what it's worth, this implementation is inspired by MockK (which we have turned away from due to worse performance) - the implementation for their mockkObject is very similar to this - using ByteBuddy to intercept instance methods on an existing instance.

I see, the field is actually public and the methods are accessed from it, so instance method interception seems like the only way.

I wonder if we can add something to the mock maker without exposing it to the Mockito public API. I think that could be a better way of handling this, as this otherwise might lead to abuse and unfortunate patterns.

Unless we want to duplicate the ByteBuddy scaffolding logic at the Mockito-Kotlin layer, I think we still should build off the static mock implementation. I will try abstracting this to the MockMaker interface like you suggested.

Super, if we can keep it on that API level, I agree to this change.

We might even make use of this in Mockito directly, too, to allow for mocking og enums. That would go down the same path, also with the mocking being thread local, affecting instance methods, and only being applied for specific, well-known instances.

Thinking about it more and how to make it useful for enums, seems like the API should accept the singleton instance we want to mock, not the class. This is fine on the Kotlin end. We also may want to decouple from the implementation being shared with static mocks - a user may want to mock instance methods on the enum and not necessarily static methods I suppose.

Let me investigate and follow up here.

I think the mock maker would just instrument the methods of the enum, then one could discriminate by the instance in the method interceptor. In this sense, the responsibilities would be clearly separated and other languages, like Kotlin, could make use of that.

Still need to add documentation to the top-level Mockito javadoc. What do you think of this approach?

I like this solution, also with regards to correlating the implementation with what we already have. In a sense, this can be used for spying, but without creating a wrapper object, and it is rather useful for that as languages increasingly hide identity in sort-of-singletons.

I also like that we release this with a POC of using it on enums.

The javadoc should however clearly state that this is limited to the current thread, and the mock will not be active on other threads unless it is activated on those explicitly.

Also, as you point out, this needs a feature description in the Mockito javadoc.

Nitpick: it's safe to use the object from another thread, it will just behave as if it was not mocked.

@mockito/developers LGTM, any comments?

Looks good now. Let's hear what the others say, I am happy to merge this!

I don't think my opinion should be considered here, given I am in the process of stepping down. In general, I appreciate the iteration to make this not specific to Kotlin and generally usable. Putting it on the MockMaker API also makes sense to me. I trust Rafael's knowledge on JVM that this is a valid approach.

Thanks @jselbo for sticking with us and the iterations to come up with an alternative approach.

Just checking, is this OK to merge?

I guess it is my call in the end anyways, and I am quite happy with this change. Thanks for the contribution and the positive reception of our many feedbacks!