#152 support value classes #849
Conversation
|
I'm fine with upgrading Kotlin to 1.7.0 by default, just make sure we maintain backwards compatibility at least to a couple versions back, i.e. we can drop support to 1.4.* if needed but I'd keep at least support to 1.5.*. |
|
Okay, I think I've cracked the code. A quick summary: To fix slots, /**
* Checks if argument is of specific type
*/
interface TypedMatcher {
val argumentType: KClass<*>
fun checkType(arg: Any?): Boolean {
return when {
argumentType.simpleName === null -> true
argumentType.isValue -> {
val unboxedClass = InternalPlatformDsl.unboxClass(argumentType)
return unboxedClass.isInstance(arg)
}
else -> argumentType.isInstance(arg)
}
}
}And to handle matching stubs to invocations, class JvmSignatureValueGenerator(val rnd: Random) : SignatureValueGenerator {
override fun <T : Any> signatureValue(
cls: KClass<T>,
anyValueGeneratorProvider: () -> AnyValueGenerator,
instantiator: AbstractInstantiator,
): T {
if (cls.isValue) {
val valueCls = InternalPlatformDsl.unboxClass(cls)
val valueSig = signatureValue(valueCls, anyValueGeneratorProvider, instantiator)
val constructor = cls.primaryConstructor!!.apply { isAccessible = true }
return constructor.call(valueSig)
}
return cls.cast(
when (cls) {
java.lang.Boolean::class -> rnd.nextBoolean()
java.lang.Byte::class -> rnd.nextInt().toByte()
java.lang.Short::class -> rnd.nextInt().toShort()
java.lang.Character::class -> rnd.nextInt().toChar()
java.lang.Integer::class -> rnd.nextInt()
java.lang.Long::class -> rnd.nextLong()
java.lang.Float::class -> rnd.nextFloat()
java.lang.Double::class -> rnd.nextDouble()
java.lang.String::class -> rnd.nextLong().toString(16)
else ->
@Suppress("UNCHECKED_CAST")
anyValueGeneratorProvider().anyValue(cls, isNullable = false) {
instantiator.instantiate(cls)
} as T
}
)
}
}In order to do this I created a common function on actual object InternalPlatformDsl {
// ...
actual fun unboxClass(cls: KClass<*>): KClass<*> {
if (!cls.isValue) return cls
// get backing field
val backingField = cls.valueField()
// get boxed value
return backingField.returnType.classifier as KClass<*>
}
}I'll commit what I have. I think there are some edgecases if a value class' value is also a value class. The test coverage for value classes is very low - basically every MockK API operation should verify it is compatible with value classes. |
| // TODO this is copy-pasted from ValueClassSupport | ||
| // I will try to move that class so it's available here | ||
|
|
||
| private val valueClassFieldCache = mutableMapOf<KClass<out Any>, KProperty1<out Any, *>>() | ||
|
|
||
| private fun <T : Any> KClass<T>.valueField(): KProperty1<out T, *> { | ||
| @Suppress("UNCHECKED_CAST") | ||
| return valueClassFieldCache.getOrPut(this) { | ||
| require(isValue) { "$this is not a value class" } | ||
|
|
||
| // value classes always have a primary constructor... | ||
| val constructor = primaryConstructor!!.apply { isAccessible = true } | ||
| // ...and exactly one constructor parameter | ||
| val constructorParameter = constructor.parameters.first() | ||
| // ...with a backing field | ||
| val backingField = declaredMemberProperties | ||
| .first { it.name == constructorParameter.name } | ||
| .apply { isAccessible = true } | ||
|
|
||
| backingField | ||
| } as KProperty1<out T, *> | ||
| } |
There was a problem hiding this comment.
I'm not sure how to resolve this
TODO this is from ValueClassSupport.kt - try and refactor to avoid copy-pasting
ValueClassSupport.ktis in projectmockk-agent-jvmInternalPlatformDslis in projectmockk-dsl-jvm
I'm looking for suggestions. Maybe it's okay just to have it copy and pasted?
There was a problem hiding this comment.
yep i think if they are in those two separate modules there's no better way and we can live with it being copypasted.
There was a problem hiding this comment.
I think a comment where the same functionality is implemented would help, i case that code ever needs to be updated.
There was a problem hiding this comment.
Good idea, and I've made an issue to try and de-duplicated it later #857
|
Thanks a lot for putting this together! I'll take a look early next week. |
|
There's a bug. I tried adding a test case for #729, but it causes MockK to hang indefinitely! /** https://github.com/mockk/mockk/issues/729 */
@Test
fun `verify extension function with UInt return can be stubbed`() {
val fn = mockk<String.() -> UInt>()
every { "string".fn() } returns 777u
val result = "string".fn()
assertEquals(777u, result)
}update: it gets stuck in a loop somewhere I guess this issue is related: #656 |
|
I think we can reduce the code of package me.qoomon.enhancements.kotlin
import kotlin.reflect.KClass
import kotlin.reflect.KProperty1
import kotlin.reflect.full.declaredMemberProperties
import kotlin.reflect.jvm.isAccessible
/**
* Underlying property value of a **`value class`** or self
*/
val <T : Any> T.boxedValue: Any?
@Suppress("UNCHECKED_CAST")
get() = if (!this::class.isValue) this
else (this::class as KClass<T>).boxedProperty.get(this)
/**
* Underlying property class of a **`value class`** or self
*/
val KClass<*>.boxedClass: KClass<*>
get() = if (!this.isValue) this
else this.boxedProperty.returnType.classifier as KClass<*>
/**
* Underlying property of a **`value class`**
*/
private val <T : Any> KClass<T>.boxedProperty: KProperty1<T, *>
get() = if (!this.isValue) throw UnsupportedOperationException("$this is not a value class")
// value classes always have exactly one property
else this.declaredMemberProperties.first().apply { isAccessible = true } |
Agreed - but |
|
Is it the native isValue property that's throwing an exception or my polyfill implemention? |
|
@aSemy I wasn't able to reproduce an exception by calling |
|
@aSemy there was a bug in my reduced |
|
Thanks @qoomon! Merged in, and I made some minor tweaks. I also added some more tests for nested value classes - I'll disable them for now because they can be handled in a later PR. However the local-extension function tests need to be resolved - they cause MockK to go into an infinite loop. Even if there's a way to make MockK throw an exception and say "this isn't supported yet" - is that possible? |
|
The issue with tests like this failing is that value classes are handled inconsistently in MockK. For example, if I comment out this line (or another one like it, I can't remember exactly) then the extension function tests work It's hard to keep track of all of the entry and exit points of MockK's stubbing and mocking and such. I think a proper fix to this is to create a new internal Whenever MockK has to handle a Creating a new type for this would ensure consistent behaviour across MockK. sealed interface KClassData<T> {
val cls: KClass<T>
/** A regular [KClass] - treat as normal */
@JvmInline
value class Cls<T>(
override val cls: KClass<T>
) : KClassData<T>
/** A value class - must be wrapped/unwrapped as required */
data class VCls<T, E>(
override val cls: KClass<T>,
): KClassData<T> {
val unwrappedType: KClass<E> = ...
fun wrap(any: Any?): T = // recursively wrap
fun unwrap(instance: T): E = // recursively unwrap
}
}I'm not going to do it in this ticket - just throwing out ideas. It would be a big refactor. |
build.gradle
Outdated
| tasks.withType(Test).configureEach { | ||
| timeout.set(Duration.ofMinutes(2)) | ||
| } |
There was a problem hiding this comment.
I added this timeout for all test tasks - I think 2 minutes should be fine.
build.gradle
Outdated
| tasks.withType(Test).configureEach { | ||
| timeout.set(Duration.ofMinutes(2)) | ||
| } |
There was a problem hiding this comment.
I added this timeout to prevent the tests from hanging. I think 2 5 minutes should be fine.
build.gradle
Outdated
| tasks.withType(Test).configureEach { | ||
| timeout.set(Duration.ofMinutes(2)) | ||
| } |
|
Thanks a lot to both of you for looking so deep into this! I'll review the code over the next days. |
|
Thank you for this great project. |
|
Ok, this is really cool, thanks a lot! Merging this as soon as the build finishes. |
|
Thank you! |
|
Thank you @Raibaz |
|
@Raibaz I would have liked to be mentioned in the release changelog as well 😅. |
|
Damn you're right, sorry! I edited the changelog. |
|
hey, I still notice issues with I described two problems I found here: #847 (comment) Not sure if this is related to this issue, but I encountered it because I first had the issue described here, then upgraded the version of MockK, error was gone but another two came up to life 😅 (you can see it in the issue I linked) |
|
@aSemy @Raibaz I found the bug causing Fixed code. WDYT? private val <T : Any> KClass<T>.boxedProperty: KProperty1<T, *>
get() = if (!this.isValue_safe) {
throw UnsupportedOperationException("$this is not a value class")
} else {
// value classes always have exactly one property with a backing field
this.declaredMemberProperties.first { it.javaField != null }.apply { isAccessible = true }
} |
|
I'll create a PR this evening, including some tests. |
|
Have a look at #872 |
|
Just ran into this issue today. Method definition: These are both very straightforward and look like they should be matched with the current implementation. produces Changing the parameter types to non-value-classes makes things work as expected. mockk version: 1.13.8 |
Update:
any()andslot()now support value classes!Summary
RecordingStatematcher to account for value classesSignatureValueGeneratorjvm impl to recursively generate values for value classesresolves #152
resolves #791
resolves #683
resolves #847
Fix (maybe?) #778?
Notes:
This PR will hopefully provide some support for value classes #152
WIP - just some failing tests at the moment (credit to @qoomon)
This PR is blocked at the moment because the Kotlin language level is set to 1.4 - see #850MockK is now on language level 1.5 🎉