mockito-kotlin icon indicating copy to clipboard operation
mockito-kotlin copied to clipboard

<<null as T>> issues

Open bohsen opened this issue 5 years ago • 14 comments

The "quirk" that mockito-kotlin is relying on to create mock instances is targeted to be fixed in the upcoming 1.3.20 release of kotlin.

What to do after that?


Edit by @nhaarman: For the foreseeable future, the target version for fixing KT-8135 has been removed. We still need to find a proper way to mitigate this for when they do fix this issue.

bohsen avatar Nov 20 '18 23:11 bohsen

Ugh. Guess we'll need to check if mockk is stable enough?

nhaarman avatar Nov 21 '18 07:11 nhaarman

Mockito-kotlin 1 used to do a lot of reflection-based instantiation to be able to deal with this, which made it rely on the reflect library. Using different versions of the stdlib and the reflection library leads to conflict problems.
KT-18398 talks about solving this issue which coincidentally is fixed for 1.3.20 as well.

Other problems with this reflection-based instantiation is that it tried to use the 'easiest' constructor available, passing in dummy data when possible. When this instantiation would fail for example due to range checks, users could statically register 'instance creators'. Statically registering things may lead to flaky tests.

Since then however, Mockito's mock-maker-inline came around, allowing final classes to be mocked. What was initially impossible now may be possible:

inline fun <reified T : Any> createInstance(): T {
    return mock()
}

Although.. Can I thenReturn() an inlined mock()?. This would also mean that users need to have mock-maker-inline enabled; I'm not sure what the current stability status on that feature is.

nhaarman avatar Nov 21 '18 07:11 nhaarman

I'm using inline-mock-maker on two projects without any issues other than taking an impact on how long it takes to build before each test run. For larger projects that impact is quite annoying though.

bohsen avatar Nov 21 '18 08:11 bohsen

I've dealt with problems caused by reflection in the past (in java) and IMHO I think that reflection, if possible, should be avoided. That said, I know the performance hit that inline-mock-maker introduces could scare people off. And I know really competent developers are actually looking at how reflection can be utilized (Jake Wharton at DroidCon-London-2018).

I've actually considered switching to DexOpener, but this is for Android projects only AFAIK.

bohsen avatar Nov 21 '18 09:11 bohsen

What kind of performance hit is this?

nhaarman avatar Nov 21 '18 11:11 nhaarman

The performance hit would be on the time it takes from tests compilation and until they're finished running. There's an excelent article here.

I've actually never measured the performance difference. I just always thought it was caused by the plugin after reading the above article a while ago. I'll have a closer look at it tomorrow, just to be sure that it's inline-mock-maker that causes the slow performance.

bohsen avatar Nov 21 '18 14:11 bohsen

@nhaarman The slow performance wasn't from inline-mock-maker. It was four tests that were hanging do to MockWebserver. After a refactor I can now execute 600+ tests in ~7 seconds. So a solution using inline-mock-maker might not be a bad idea.

bohsen avatar Nov 27 '18 21:11 bohsen

Oh, they've dropped the fix for now. Closing this then.

bohsen avatar Nov 27 '18 21:11 bohsen

Let's reopen this to keep tracking the issue.

nhaarman avatar Nov 28 '18 09:11 nhaarman

@nhaarman The slow performance wasn't from inline-mock-maker. It was four tests that were hanging do to MockWebserver. After a refactor I can now execute 600+ tests in ~7 seconds. So a solution using inline-mock-maker might not be a bad idea.

Good to hear!

To prepare for future changes in KT-8135, we could create a mechanism to configure the createInstance method or something like that.

I guess using mocks is preferred, at least when mock-maker-inline is enabled.
Mockito-kotlin 1.x contained the following snippet to create mocks without altering internal state:

https://github.com/nhaarman/mockito-kotlin/blob/1.x/mockito-kotlin/src/main/kotlin/com/nhaarman/mockito_kotlin/createinstance/InstanceCreator.kt#L176

/**
 * Creates a mock instance of given class, without modifying or checking any internal Mockito state.
 */
@Suppress("UNCHECKED_CAST")
fun <T> Class<T>.uncheckedMock(): T {
    val impl = MockSettingsImpl<T>().defaultAnswer(Answers.RETURNS_DEFAULTS) as MockSettingsImpl<T>
    val creationSettings = impl.setTypeToMock(this)
    return MockUtil.createMock(creationSettings).apply {
        (this as? MockAccess)?.mockitoInterceptor = null
    }
}

However, some internal Mockito APIs were accessed:

import org.mockito.internal.creation.MockSettingsImpl
import org.mockito.internal.creation.bytebuddy.MockAccess
import org.mockito.internal.util.MockUtil

nhaarman avatar Nov 28 '18 09:11 nhaarman

Since then however, Mockito's mock-maker-inline came around, allowing final classes to be mocked. What was initially impossible now may be possible:

inline fun <reified T : Any> createInstance(): T {
    return mock()
}

The mock-maker-inline solution to me seems safer (if it works?). Depending on internal APIs would be a no-go for me unless there were no other options. Also mock-maker-inline can now be provided as a dependency - testImplementation "org.mockito:mockito-inline:$mockito_version" (I use this without issues). So it's really easy to setup, as supposed to before. I've created a branch with working solution here where all tests pass. Will require a thorough review.

Although.. Can I thenReturn() an inlined mock()?.

If that's the only trade off then... The documentation should reflect it of course. Not sure if mockito-core provides a way to detect inlined mocking? Then it could throw exception.

bohsen avatar Nov 30 '18 09:11 bohsen

Looks like JetBrains made plans for Mockito-kotlin in version 1.3.40 🥳

bohsen avatar Apr 06 '19 20:04 bohsen

Hi guys, im afraid to ask, but this fix isnt implemented if i understand the ticket correctly right?

Ditscheridou avatar Jan 18 '21 14:01 Ditscheridou

@Ditscheridou It's not. Still open. They had it on the list for kotlin 1.4.20, but they took it off again.

bohsen avatar Jan 18 '21 15:01 bohsen