Follow-up on primitive value classes

[m-koops]

This PR is a direct follow-up to #557 , in which @jselbo introduced some test cases that proved Mockito-Kotlin to fail on handling (nullable) primitive value types in eq matcher and argument captor.

Some background: Kotlin compiler will typically pass on value classes unboxed to Java calls. While unboxing, it will apply the nullability of the value class to the unboxed inner type. But with one exception: when the inner type is a Java primitive then nullability cannot be applied to that primitive. In that case Kotlin compiler passes on the unboxed value type into Java calls.
This exceptional behavior is what is breaking MOckito-Kotlin.

To fully cover handling of (nullable) primitive value class types, tests have been added/activated for those types in eq and any matchers, in argument captors and as return values in synchronous and suspendable functions.

Additionally, the boxing and unboxing logic has been extracted to a new class ValueClassSupport, to facilitate for reuse of the logic and to test the logic with dedicated unit tests.

[jselbo]

Thanks! This is a great improvement. I ran our tests against this and it fixed several issues.

I found two remaining issues.

1. Use of suspend functions in argumentCaptor breaks

example:

val callbackCaptor = argumentCaptor<suspend () -> Unit>()

fails to build with:

error: suspend functional types are not supported in typeOf

I tried using typealias to define the function but this fails with the same error

2. eq() on nullable primitive value class only works if casted

In this test case you added as PrimitiveValueClass? and the test fails without the cast, which isn't intuitive

        fun eqNullablePrimitiveValueClass() {
            val primitiveValueClass = PrimitiveValueClass(123) as PrimitiveValueClass?

I can work around in test code by adding the cast but is there a way to fix this to avoid this requirement?

[m-koops]

Many thanks for the feedback, I will dive into both issues and hope to come with improvements.

[m-koops]

I was able to find a solution for the 2nd issue you mentioned.

Within the definition of the matchers there is no context of the parameter signatures of the stubbed function call, it should match with. Therefore there is no way to detect the mismatch between a nullable function parameter and a non-nullable argument for the eq matcher function.

I've fixed the mismatch for (nullable) arguments in eqValueClass() by relaxing that matcher: it now matches the actual mock function call argument against either the boxed or unboxed value of eq-matcher's argument.

I will have a look into the captor issue later this weekend.

[m-koops]

The issue with the captors is a nasty one: the typeOf<T>() function, which is used in KArgumentCaptor to detect the runtime nullability of the type parameter(s) in the various overloads of factory methods argumentCaptor(), is not compatible with suspend functions as type parameter. It doesn't even matter if that typeOf() call is actually on a reachable code branch in the function, the compiler just fails over the incompatibility of the reified type parameter in case of a suspend function.

I have introduced a new factory method suspendFunctionArgumentCaptor() that can deal well with the suspend function type argument (which in this case is declared non-nullable). This new method can be applied as second-best alternative for the broken use of argumentCaptor(). Sorry, I did my very best to find a smart backwards compatible solution but there isn't (yet). The incompatibility in typeOf<T>() is descriped in Kotlin issue KT-47562. In future, when this ticket is solved, suspendFunctionArgumentCaptor() can be dropped in favor of the generic factory method argumentCaptor().

At least, the use of ArgumentCator with (suspend) funtion arguments is now covered with tests.

For me this completes this PR, I hope you agree with me.

[TimvdLippe]

General approach LGTM. Will let @jselbo do the in-depth review.

[m-koops]

I would like to suggest to first focus on getting this PR merged, before merging #556. After merging this one, and rebasing #556, I can adjust documentation of new API to that of the other API. Also the added test cases should be updated to align with the other tests. I can do so in an additional commit after rebasing.

[jselbo]

Code changes LGTM 👍

I checked with the latest changes and all our tests are passing, after making use of the new suspendFunctionArgumentCaptor. Thanks for digging into these tricky corner cases!

[jselbo]

Can you comment on KT-47562 linking to this PR and saying this complicates the public API for a very popular mocking framework? Maybe a bump will motivate Jetbrains to fix the issue with typeOf()

[m-koops]

Can you comment on KT-47562 linking to this PR and saying this complicates the public API for a very popular mocking framework? Maybe a bump will motivate Jetbrains to fix the issue with typeOf()

I've just posted a comment explaining the troubles encountered here due to typeOf() being incompatable with suspend functions.