Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Mar 5, 2024

Conversation

jibidus
Copy link
Contributor

@jibidus jibidus commented Feb 26, 2024

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

  • enableRecursion is not handled through a fluent api (but with a parameter), which seems to be the common way to do such things in jqwik. I didn't succeed to implement this feature with a dedicated type (ex SubtypeArbitrary), which would permit to use a fluent api.
  • since many concepts are created here, I used a dedicated file AnyForSubtypeOfDsl.kt (instead of JqwikGlobals.kt where anyForType() is declared).
  • I would have liked to group tests with their test cases, in order to reuse same interface en class names for readability purpose. But I didn't succeed to use inner classes annotated with @Group annotations, since sealed interface cannot be declared in inner classes.
  • 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…

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

@jibidus
Copy link
Contributor Author

jibidus commented Feb 27, 2024

This will fix #523 for Kotlin only.

@jlink
Copy link
Collaborator

jlink commented Feb 27, 2024

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
Copy link
Contributor Author

jibidus commented Feb 27, 2024

I see many difficulties with this simple if :

  • this if should be in jqwik engine, where isSealed() is not available (not in kotlin module.
  • it's not clear for me where to put this if. Maybe in DefaultTraverseArbitrary?

… 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.

Copy link
Collaborator

@jlink jlink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the entry point for anyForSubtypeOf(..) should be in file JqwikGlobals.kt, next to anyForType. Of course, you can delegate from there to the real implementation in a different file.

@jlink
Copy link
Collaborator

jlink commented Feb 29, 2024

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
Copy link
Collaborator

jlink commented Feb 29, 2024

  • 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
Copy link
Contributor Author

jibidus commented Feb 29, 2024

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?

Copy link
Collaborator

@jlink jlink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* }
* ```
* @param enableArbitraryRecursion is applied to all created [TypeArbitrary].
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add api annotation:
@API(status = API.Status.EXPERIMENTAL, since = "1.8.4")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

inline fun <reified T> anyForSubtypeOf(
enableArbitraryRecursion: Boolean = false,
crossinline subtypeScope: SubtypeScope<T>.() -> Unit = {}
): Arbitrary<T> where T : Any {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to have TypeArbitrary<T> as return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would imply that custom arbitrary (registered with SubtypeScope<T>.provide()) can only be TypeArbitrary<T>. This would make custom arbitraries useless, wouldn't it?

@jibidus
Copy link
Contributor Author

jibidus commented Mar 3, 2024

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 🙂.

@jlink jlink merged commit dd4fded into jqwik-team:main Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants