Add property testing extensions for custom generators

[breandan]

I wanted to write:

"test plus" {
  assertAll(CustomDoubleGenerator) { x: MyDouble, y: MyDouble ->
    (x + y).value == x.value + y.value
  }
}

But instead, I had to write:

"test plus" {
  assertAll(CustomDoubleGenerator, CustomDoubleGenerator) { x: MyDouble, y: MyDouble ->
    (x + y).value == x.value + y.value
  }
}

This seemed a bit repetitive, so I added some extra extension methods to support the first notation.

Been using KotlinTest for a while and enjoying it so far! Thanks for this awesome project.

[breandan]

The it -> solution is somewhat annoying, but this ought to be fixable on the language side. If a lambda argument on a function with multiple overloads and varying lambda parameter length is used, the language should infer it refers to the function with one lambda parameter, but alas, Kotlin needs an explicit lambda parameter at the call site to disambiguate.

[LeoColman]

Sorry for the long time to review this.

I don't think that adding that it to everything is better than repeating the Gen. I believe that changing this may break a lot of codes that work today.

Maybe there's another way to do this?

[sksamuel]

I agree with @Kerooker . This is a breaking change that trades one set of time saving sugar for another. You save some code in your examples, but in other places have to add it, and I think the latter is probably a bit of a gotcha.
I like the goal though, perhaps we can do this another way? Maybe just give assertAll another name or something.

[sksamuel]

Could use assertAll2, assertAll3, or assertAllTupled or something.

[LeoColman]

Maybe we can provide extensions for Gen themselves?
If the goal is to reuse a single generator a lot of times, we could do something like this:

Gen.int().assertAll {
    it shouldBe 0
}

Gen.int().assertAll { a, b ->   // Necessarily an Int, as it's Gen's type
a + b shouldBe 0
}

And so on for all the extensions. We would kill two birds with one stone, because this would also be a good addition to current Gens, not only custom ones.

What do you guys think?

[sksamuel]

Very nice. I think that could work well, but do you still have the same issue with the overloads? Need to test that.

[LeoColman]

Let us fiddle with this. I'll try something

[breandan]

This should really be addressed at the language level. Kotlin should automatically choose lambda expressions with a single argument over multiple arguments for the implicit case. You should not need to specify an explicit lambda signature in order to use it when competing overloads all have higher arity.

[sksamuel]

Yes it should be, but we can't change Kotlin :)
So a workaround is the next best thing.

[breandan]

Even if you could fix Kotlin, you might still want to support KotlinTest users on previous language versions. Maybe it would be less disruptive if you were to put a hold on this PR until a major version update, or some other breaking API change and ship them together? I'm not sure if there is a good way to support universal quantification extensions for property testing using the simplified CustomGenerator notation without breaking existing call sites with implicit unary lambdas of the form claim(CustomGenerator){ ...it... } unless we force users to adopt explicit lambdas: claim(CustomGenerator){ it -> ...it... }.

edit: You could change the name for higher arities, although maybe that's not quite as satisfying a solution.

[sksamuel]

I don't want to force users to have to declare it as the parameter because that's not intuitive. Despite the fact that the language forces this, I think users would be surprised as to why their code was not compiling.

I think @Kerooker's proposal is the best so far, overload the Generator itself, and don't include the unary versions there. Backwards compatible, and just as easy as the alternative.

[breandan]

As suggested by @Kerooker, I added the higher arity versions of assertAll, assertNone, forAll, forNone, verifyAll, verifyNone as extension functions to Gen<A> directly, and reverted the test modifications to fix the previous implementation with overloading. This syntax is more succinct and backwards-compatible, requiring no alterations in existing KotlinTest code bases. Please let me know if this is what you had in mind.

[LeoColman]

I believe that this is on point on what I had in mind! Good one!

However some work should still be done.

• The usual assertAll(gen) { } and variations have some customizations, such as amount of invocations via theiterations parameter. Please add a iterations parameter that defaults to 1000 instead of using 1000 always to all extensions.
• Add a test for each of the extensions. If you don't want to implement a test for every arity, at least create one for the lowest arity and we can assume that the greater ones will work.

[LeoColman]

A test with iterations != 1000 would be good to cover the iterations parameter, then I think we're good

[breandan]

Thank you for your feedback. I have added an iterations: Int = 1000 parameter for each of the extensions on Gen<A> and unit tests for extensions with dyadic lambdas in each of the existing property testing tests in kotlintest-tests/kotlintest-tests-core/src/test/kotlin/com/sksamuel/kotlintest/properties, both for the default number of iterations and with a custom number of iterations. Please let me know if you need anything else on my end in order to merge this PR.

[breandan]

IntelliJ IDEA seems to run very slowly on this codebase. Is it just me or you have any suggestions to make navigation and compilation more responsive?

[LeoColman]

There's a lot of code. You have to let it build at least once, then it will get a little bit smoother.

[LeoColman]

Ok. One more thing

As we're adding these extensions, why not add one with arity = 1?

Gen.int().forAll {
    it + 5 shouldBe 5 + it
}

[breandan]

why not add one with arity = 1

Not sure, @sksamuel?

overload the Generator itself, and don't include the unary versions there

[LeoColman]

I suppose it's because the JVM would clash as the signature would be the same, but this isn't supposed to be called from Java anyway, so we can change the name.

But I mean, if the user wants to use Gen.int().assertAll { } instead of assertAll(Gen) {}, it should be ok too

[breandan]

I would skip the unary version, but it’s your call. Having two methods which do the same thing may lead to some confusion, and you may want to avoid the scenario where the user assumes (incorrectly) that it can be used multiple times to generate distinct values in the same lambda. By only defining multiple arguments, the semantics are more explicit.

[sksamuel]

Having two ways to do something is not ideal, as @breandan says.
However, if you have a custom generator for a built in type, for example Gen.numericDoubles(), then you can't do.

assertAll { a : Double -> ... }

Because it will pick up the default double generator.
So you need to do

assertAll(Gen.numericDoubles()) { a -> ... }

Maybe more elegant to do

Gen.numericDoubles().assertAll { a - > ... }

?

I don't have a strong opinion either way.

[LeoColman]

I think that allowing unary gens to be used with the extension is also good.

[LeoColman]

@breandan
If you want to add it, be my guest.

If you don't, I can do it :)

[sksamuel]

I think this is good to merge now.

[sksamuel]

Great contribution @breandan