mockito / mockito-kotlin

Using Mockito with Kotlin
MIT License
3.11k stars 202 forks source link

<<null as T>> issues #310

Open bohsen opened 5 years ago

bohsen commented 5 years ago

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.

nhaarman commented 5 years ago

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

nhaarman commented 5 years ago

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.

bohsen commented 5 years ago

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 commented 5 years ago

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.

nhaarman commented 5 years ago

What kind of performance hit is this?

bohsen commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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

nhaarman commented 5 years ago

Let's reopen this to keep tracking the issue.

nhaarman commented 5 years ago

@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
bohsen commented 5 years ago

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 commented 5 years ago

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

Ditscheridou commented 3 years ago

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

bohsen commented 3 years ago

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