jqwik-team / jqwik

Property-Based Testing on the JUnit Platform
http://jqwik.net
Eclipse Public License 2.0
576 stars 64 forks source link

Add anyForType() dedicated to sealed classes and interfaces in Kotlin module: anyForSubtypeOf() #555

Closed jibidus closed 8 months ago

jibidus commented 9 months ago

Overview

anyForType() does not support sealed classes or interfaces. The idea here is to support these cases with a dedicated function anyForSubtypeOf() for Kotlin only, since it is easier to implement than in Java.

Details

anyForSubtypeOf() use TypeArray under the hood for each sealed subtypes, recursively:

sealed interface Character
sealed interface Hero : Character
class Knight(val name: String) : Hero
class Wizard(val name: String) : Character

val arbitrary =  anyForSubtypeOf<Character>(enableArbitraryRecursion = true) {
    provide<Wizard> { Arbitraries.of(Wizard("Merlin"),Wizard("Élias de Kelliwic’h")) }
}

It is also possible to enable recursion with created TypeArray with enableArbitraryRecursion, or to provide custom arbitraries for specific subtypes.

Notable decisions


I hereby agree to the terms of the jqwik Contributor Agreement.

jibidus commented 9 months ago

This will fix #523 for Kotlin only.

jlink commented 9 months ago

I haven't had time to check the code yet, but as for

It would probably be more intuitive for jqwik users, to add this feature in anyForType(), but I'm not familiar enough with DefaultTypeArbitrary and TraverseArbitrary classes to be able to do that. Maybe with a little help…

maybe a simple if-clause could do, along the lines:

if (targetType.isSealed()) {
    return anyForSubtype(targetType)
}
... // do whatever was there in the first place
}
jibidus commented 9 months ago

I see many difficulties with this simple if :

… unless you were thinking of putting this if in anyForType() function? It would be better for jqwik users, but anyForSubtype() have to return an instance of TypeArbitrary, and this is one of the difficulties I have had to face.

jlink commented 9 months ago

unless you were thinking of putting this if in anyForType() function? It would be better for jqwik users, but anyForSubtype() have to return an instance of TypeArbitrary, and this is one of the difficulties I have had to face.

I think we can deal with this later. Let's get anyForSubtypeOf() ready for release first.

jlink commented 9 months ago
  • this if should be in jqwik engine, where isSealed() is not available (not in kotlin module.

Could https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.reflect/-k-class/is-sealed.html do the trick?

jibidus commented 9 months ago

I think the entry point for anyForSubtypeOf(..) should be in file JqwikGlobals.kt, next to anyForType.

Done (all moved in JqwikGlobals.kt).

this if should be in jqwik engine, where isSealed() is not available (not in kotlin module).

Could https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.reflect/-k-class/is-sealed.html do the trick?

This is what I meant by isSealed(). We cannot use it from jqwik engine (where DefaultTypeArbitrary or TypeTraverser are located), right?

jibidus commented 8 months ago

Since the implementation is sufficiently complex I'd rather have it in a separate file and the function in JqwikGlobals delegating to it. What do you think?

I have no opinion.

With only anyForSubtypeOf() in JqwikGlobals, we follow a technical cohesion (with only entry points gathered in JqwikGlobals). On the other side, with all components of anyForSubtypeOf feature in the same place, we follow a functional cohesion. It seems that the first one permits to follow convention of current codebase (which is great), whereas the second one permits to follow a better cohesion (based on functionalities, not on technical characteristics).

So I have no opinion. I have done what you suggest 🙂.