Hi all. just bouncing some ideas around property t...

# kotest-contributors

[mitch]

Hi all. just bouncing some ideas around property testing. I just got some inspiration based on some recent work in property and suspend funs, as well as @sam’s branch on prop spec. This is 100% possible with some tweaks in the proptest and PropertyContext.

so before

test(...) {
  checkAll(arbFirst, arbSecond) { first, second ->
    sut.execute(first, second)
  }
}

and after

test(...) { sut.execute(arbFirst.value(), arbSecond.value()) }

It’s really mindblowing! I can imagine how many hundreds lines of code I can simplify in our current tests if kotest has this.

[mitch]

the challenge that i can think of is classification. i don’t know how to make that work in here

[mitch]

@sam would be awesome to get your expert opinion around this.

[sam]

What is it that you're trying to simplify here (because I don't have the before to compare)

[mitch]

ah yes. hang on

[mitch]

we have a lot of tests like following

// if we'd need additional setups - which alot of time is possible 
test("should test using arbs with setup") {
  checkAll(arbFirst, arbSecond) { first, second
    mongo.createDocument(first.copy(id = null))
    val testFirst = mongo.findByName(first.name)
    val testSecond = second.copy(id = testFirst.id)

    systemUnderTest.execute(testFirst, testSecond)
  }
}

I think this is possible

test("should test using arbs with setup") {
  val first = mongo.createDocument(arbFirst.value().copy(id = null))
  systemUnderTest.execute(
    mongo.findByName(first.name),
    arbSecond.value().copy(id = first.id)
  )
}

[mitch]

as a user it’s pretty convenient to just call an arb inside a test and one just got a generated value out of it

[mitch]

and plus because there’s a rs, it’s repeatable when things fail

[mitch]

@sam added the examples 🙌

[mitch]

moved more nitty gritty implementation detail here:

test("arbitraries should be used and registered") {
  propertyContext(proptestConfig, ...etc) { // produces a random source, this is PropertyContext
    val first = arbFirst.value()   // calls arbFirst.generate(rs) under the hood and record the sample & shrinks.
    val second = arbSecond.value() // calls arbSecond.generate(rs) under the hood and record the sample & shrinks.
    
    systemUnderTest.execute(first, second)
  }
}

// or simply the following, with Sam's property syntax
context("given some context") {
  configureProperty(proptestConfig)

  proptest("arbitraries should be used and registered") {
    systemUnderTest.execute(arbFirst.value(), arbSecond.value())
  }
}

[sam]

Getting a single arb is easy, you just call .next() on the arb

[sam]

I guess having a repeatable rs requires as bit more boilerplate

[sam]

I guess it would be easy to expose the rs so you can call next and have the value be repeatable.

checkAll { 
  val first = <http://Arb.int|Arb.int>().value()
  val second = Arb.string().value()
  sut.execute(first, second)
}

But isn't that just

checkAll<Int, String> {  first, second -> 
  sut.execute(first, second)
}

Which is actually less code ?

[mitch]

Yeah. The first one is what im thinking. Here we got the benefit of convenience and repeatability from rs as well.

We actually never really use the second one (simple type inference), because they aren't explicit which arb were used. Its utility is also quite limited to simple types.. maybe that's something to expand on as well

[sam]

If we want to support this I think it would be really easy.

checkAll(iterations, [seed]) {
  val first = <http://Arb.int|Arb.int>().value()
  val second = Arb.string().value()
  sut.execute(first, second)
}

And for anything other than that:

checkAll(config) {
  val first = <http://Arb.int|Arb.int>().value()
  val second = Arb.string().value()
  sut.execute(first, second)
}

[sam]

We could make checkAll accept a name too, if we want to even avoid having to wrap inside a test("") block, but I don't know if that's conflating two things

[sam]

Eg

test("all strings should be commutative under addition") {
   checkAll(1000) {
      val a = Arb.string().value
      val b = Arb.string().value
      a + b shouldBe b + a
   }
}

vs

checkAll("all strings should be commutative under addition", 1000) {
   val a = Arb.string().value
   val b = Arb.string().value
   a + b shouldBe b + a
}

[mitch]

yeah you read my mind, I’m thinking if we can perhaps piggyback on the current checkAll like in your first example. Because all infrastructure are already set in there.

[sam]

which one do you like best

[mitch]

i think this is definitely tempting (probably as a follow up)

test("...") {
   val a = Arb.string().value()
   val b = Arb.string().value()
   a + b shouldBe b + a
}

which kinda suggest we need to be propagate some base PropertyConfig in the CoroutineContext

[mitch]

the thing i can’t still figure out is classification, it can’t easily work, can it?

[sam]

why not

[sam]

the existing classification or the new auto classification

[mitch]

existing classification. you see, right now, it creates some sort of map storing the classification instances of the arbs that got passed in. but with the new mechanism, you can conjure a new arb inside the lambda.

[mitch]

like this

test("...") {
   val a = Arb.string().value() // that's new, so everytime it loops, you get a new arb
   val b = Arb.string().value()
   a + b shouldBe b + a
}

[sam]

the classification values are held inside the PropertyContext

[sam]

which is setup by checkAll

[sam]

it doesn't matter what the arb does, so it can be a fresh arb each time

[mitch]

checking..

[mitch]

where do we put value inside the classifications mutable map?

[sam]

when is it added ?

[mitch]

yeah

[sam]

classify methods on PropertyContext

[mitch]

ah right! yes you’re right, that’ll continue to work

[sam]

yep

[mitch]

why did i think it won’t work

[mitch]

i must have found something else…?

[sam]

maybe you think the arbs held onto state or something

[mitch]

would autoclassification works?

fun autoclassifications(): Map<String, Map<String, Int>> = autoclassifications.toMap()

[sam]

yep

[sam]

all the state is held inside property context

[mitch]

man that’s super awesome!

[sam]

yep 🙂

[mitch]

awesome! i think we can’t use this function for that then? because the arbs are now dynamically registered. I can start looking into it if you haven’t already.

internal suspend fun test(
   context: PropertyContext,
   config: PropTestConfig,
   shrinkfn: suspend () -> List<ShrinkResult<Any?>>,
   inputs: List<Any?>,
   classifiers: List<Classifier<out Any?>?>, // this
   seed: Long,
   fn: suspend () -> Any
) {
   require(inputs.size == classifiers.size)
   try {
      inputs.indices.forEach { k ->
         val value = inputs[k]
         val classifier = classifiers[k]
         if (classifier != null) {
            val label = (classifier as Classifier<Any?>).classify(value) // this
            if (label != null) context.classify(k, label)
         }
      }
}

[sam]

Hmm that might be the sticking point - that test method is used for shrinking

[sam]

I think we can work around that

[sam]

we would need to hold onto a copy of every value returned from .value in that loop - and then use those values if an exception is throw to shrink them

[mitch]

yeah that list of values (which can be stored in an atomic) can be cleared afresh when the test lambda completes, to conserve memory.

[sam]

right

[sam]

so .value on an arb would need to get access to property context and could register the values there

[sam]

but now you need to define two receivers

[sam]

which we can't do

[sam]

unless .value was actually a member method...

[sam]

inside Arb: fun PropertyContext.value() : A {
val a = next(rs)
store(a)
return a
}

[mitch]

yeah, or

sealed class Gen<A> {
  suspend fun TestContext.value(): A = getOrCreatePropertyContext { ctx ->
    val a = next(rs)
    ctx.store(a)
    a
  }
}

[sam]

yeah you could pull it out of the coroutine context

[sam]

the extension method stops you trying to do it elsewhere though

[mitch]

but that’ll require property context to be propagated through and created when users opens up any sort of

test("stuff") { // new context here
}

[mitch]

ah shoot you’re right

[sam]

the check all's currently expose PropertyContexxt as the receiver

[sam]

so we could do the same in the new checkAll

[mitch]

yeah we don’t want to end up with this

interface TestContext : PropertyContext, CoroutineScope { ... 
}

[mitch]

conflation alert

[sam]

well that would be tricky as a) the kotest framework doesn't bring in the property framework, b) it means you couldn't use it in junit c) breakaing changes

[mitch]

yeah exactly

[mitch]

or maybe we can enrich property context, since we have a new scope

interface DynamicPropertyContext : PropertyContext {
  fun <A> Arb<A>.value(): A { ... }
}

[sam]

yes that's another option

[sam]

or just add that method to the existing property context

[sam]

PropertyContext is not an interface so you can add to it safely

[mitch]

that’s currently calling that internal test method.. so it can be tricky

[sam]

I think PC is a better place for it then the Arb class

[mitch]

yeah i definitely +1 on that

[sam]

I started something here:
https://github.com/kotest/kotest/pull/2529

[sam]

All we need to do is pass RandomSource to the property context when it's created, and create a new checkAll method that sets up a property context and calls reset

[mitch]

mind blown

[mitch]

sam you’re so fast

[sam]

I will add the checkall now

[mitch]

🙇

[sam]

Ok this now works (pushed on branch)

test("checkAll should setup a property context") {
   var count = 0
   checkAll(100) {
      val a = Arb.string().value()
      val b = Arb.string().value()
      (a + b).length shouldBe b.length + a.length
      count++
   }
   count shouldBe 100
}

test("checkAll should use a repeatable seed") {
   checkAll(1, 52104482139021L) {
      val a = Arb.string().value()
      a shouldBe "cUj%x=.f6ktw\"icenM(AEw&5CP3q+8FxU*xi<p!Jbd"
   }
}

[sam]

the count in the first test is just to confirm the iterations parameter is being respected

[mitch]

nice!!

[sam]

its nice this

[sam]

test("auto labelling") {
   val context = checkAll(100, 98173L) {
      Arb.string().value()
   }
   context.autoclassifications()["1"] shouldBe mapOf(
      "MAX LENGTH" to 3,
      "ANY LENGTH LETTER OR DIGITS" to 3
   )
}

[sam]

auto labelling wired in

[sam]

bug there, should not be 3

[sam]

ok all good

[sam]

we need shrinking now

[sam]

Ok I added some hooks for shrinking. I can leave that bit to someone else 🙂

[mitch]

i’ll pick that up @sam thanks a lot! 😉