kotest icon indicating copy to clipboard operation
kotest copied to clipboard

Provide RandomSource in PropertyContext

Open tbvh opened this issue 2 years ago • 13 comments

For certain tests it might be useful to have access to a deterministic random source. For example when working on (large) lists with expensive computations, where it is too expensive to verify all combinations exhaustively. Or simply when one element or a subset must be selected.

It would be helpful to be able to simply call: val element = theCompleteList.random(random)

Omitting the random argument results in a test that is not repeatable.

Since the properties execute in the context of PropertyContext, it would be possible to proved the randomSource as a public member.

tbvh avatar Jun 10 '22 13:06 tbvh

If there is support for this feature I'm willing to create a merge request to add RandomSource to PropertyContext.

For the time being, as workaround one can append the checkAll with a Generator for the kind of values that is desired. E.g. Arb.int() for list indexes, but this is cumbersome b/c list size needs to be respected.

Alternative workaround: a faux Arbitrary can be created that exposes the actual Random.

fun random(): Arb<Random> {
    return arbitrary { it.random }
}

tbvh avatar Jun 10 '22 13:06 tbvh

We have briefly discussed about it in the past https://github.com/kotest/kotest/issues/2574#issue-1028289236

The caveat here is that shrinking won't work.. so that was why we couldn't pursue it further. However, if we don't care about shrinking then yes it's possible to propagate random source and whether it's an edgecase etc inside a property context so that you can sample/edgecase any arb as in:

checkAll(arbA, arbB) { a, b ->
  val cValue: C = arbC.bind()
  ...
}

But otherwise you're right currently we also can achieve that using arbitrary { rs -> rs }.

Summoning @sksamuel

myuwono avatar Jun 10 '22 22:06 myuwono

If we're talking about just having a random source in general, then we can add seed and random to the test structure in Kotest. So that any test, whether a property test or not, has access to a deterministic random instance.

If we are talking about property testing in particular, then I don't see why PropertyContext can't have access to the random instance used by the property test. I don't understand the link between that and the bind discussion @myuwono .

sksamuel avatar Jun 12 '22 02:06 sksamuel

Ah right, nice! well in that case i think it makes sense to introduce the seed and therefore propagated as random source to within the prop/normal test. After which that actually means we can bind any arbs within any test context.

myuwono avatar Jun 12 '22 08:06 myuwono

Leave this to me, I think I know what to do

myuwono avatar Jun 12 '22 09:06 myuwono

Hi, I'm reporting back after tinkering with the Arb<Random> workaround. At first I didn't understand the concern with shrinking, but I've now encountered it myself and can illustrate it. I believe there is still a good case to provide deterministic random but it should come with some guidance to the user (I), and perhaps more (complex) examples of preparing the generator (II).

I) I've added an example below. In the example an Arb.list is used to generate the input, and then a sample is taken to run the tests with. The issue is that when the input is shrunk the sample is different, even if the failing case is still present in the shrunk input. Note that this is the case both for random sampling and for non-random sampling, not because of random sampling. The issue arises because the shrinker is not exhaustive, it is in some way an artifact of the shrinking function. Imo should developers be informed about this caveat and get some direction how to circumvent it. Something like:

In order to support the shrinker, a developer should make sure that entire input is consumed. I.e. the complete input arguments are send to the code-under-test, or collections are traversed without skips when a testcase is prepared (eg filtering/pruning). Sampling should be reserved for verification only (the then phase). When a failing test case is found and shrinking fails this can be resolved by making the verification exhaustive, without breaking determinism of the test.

This guidance is required since the shrinker is not exhaustive, but having knowledge about the (list) shrinker allows us to sample cautiously element by element. In the example below, taking just the first two element from the list to use in when doesn't interfere with the shrinker.

II) To overcome the issue above, the best way is to prevent tinkering with the input in the given part of the test. In fact, property based testing should read like: "Given [this arbitrary input] when [code-under-test] then [verification]", so the given part is already covered by creation of the input arguments.

For it to work like this, however, it is required to build more complex generators (with appropriate shrinkers). Imo the docs could do a better job to illustrate how, with examples, and introduce this as a best practice. (That is: to move the given complexity to a generator instead of applying it in the check block.)

Second, it perhaps requires shrinkers to work hierarchically for wrapped Arbs. The code below does not apply the String shrinker to further reduce the input:

        checkAll(Arb.list(Arb.string(), 1..50)) { 
            it.first().length shouldBeLessThan  7
        }

Note: in my real world example I'm performing graph operations and sample nodes from a generated list of nodes. Instead I should generate a list of nodes in my generator and transform that in a list of tuples to operate on. Then the shrinker can shrink the tuples, i.e. shrink the actual input for the code-under-test.

class ShrinkIssuesTest : StringSpec({
    fun distinct(a: Int, b: Int) = a != b || a == 99 // A bug!!

    "Distinct of random values should return correct value" {
        checkAll(
            PropTestConfig(seed = 4676911949770428600L),
            Arb.list(Arb.int(1..100), 2..100),
            arbitrary { it.random }
        ) { numbers, rnd ->

            // Given
            /*
             * By taking a random value to run the test with, it is possible that the shrunk arguments still contain the failing case, but it won't be found.
             */
            val a = numbers.random(rnd)
            val b = numbers.random(rnd)

            //When
            val actual = distinct(a, b)

            // Then
            actual shouldBe (a != b)
        }
    }

    "Distinct of non-random value should return correct value" {
        checkAll(
            PropTestConfig(seed = -2191802612892208069L),
            Arb.list(Arb.int(1..100), 2..100),
            arbitrary { it.random }
        ) { numbers, rnd ->

            // Given
            /*
             * Even if randomness is dropped, constructing the test case from the arb arguments can cause shrinking to fail.
             */
            val a = numbers[numbers.size/3]
            val b = numbers[numbers.size/4]

            //When
            val actual = distinct(a, b)

            // Then
            actual shouldBe (a != b)
        }
    }

})

tbvh avatar Jun 21 '22 10:06 tbvh

Sorry to complicate this further. I've just created https://github.com/kotest/kotest/issues/3076, suggesting the possibility to run the shrunk case, by skipping over the large iterations.

This new feature requires that the random source is completely controlled by the kotest framework, and no interactions with it are outside the framework. This should be taken into account when solving this issue (https://github.com/kotest/kotest/issues/3046).

Instead of providing the actual random source from the framework, my suggestion is to create a new Random, seeding it it from the frameworks random source, and injecting the new Random into the test context. This way the RandomSource of the framework remains isolated from the Random provided to the test.

tbvh avatar Jun 27 '22 09:06 tbvh

all good inputs! thank you @tbvh. Those all make sense. I think the last point would require quite an overhaul of how checkAll is currently run within kotest. Since I'm currently making some pretty large modification to the property test runner it makes sense to ensure that the last point that you raised is addressed.

myuwono avatar Jun 27 '22 11:06 myuwono

Instead of providing the actual random source from the framework, my suggestion is to create a new Random, seeding it it from the frameworks random source, and injecting the new Random into the test context. This way the RandomSource of the framework remains isolated from the Random provided to the test.

So in other words, using the provided random source wouldn't move the framework's random source on? So we'd create a "seed" random from the seed value, and then use that to generate a random per test, which would be fed into the test for users to use?

Something like

val seedRandom = Random(seed) for (iteration in iterations) { val testRandom = Random(seedRandom.nextLong()) }

sksamuel avatar Jul 03 '22 17:07 sksamuel

Yeah, I've done exactly that in my local and hasn't pushed it. I'll raise a quick pr for that.

myuwono avatar Jul 03 '22 21:07 myuwono

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Aug 12 '22 00:08 stale[bot]

Sorry for the significant delays @sksamuel I've been occupied with various things in life.. I'll look into this in the coming week.

myuwono avatar Aug 30 '22 11:08 myuwono

Great!

On Tue, 30 Aug 2022 at 06:47, Mitchell Yuwono @.***> wrote:

Sorry for the significant delays @sksamuel https://github.com/sksamuel I've been occupied with various things in life.. I'll look into this in the coming week.

— Reply to this email directly, view it on GitHub https://github.com/kotest/kotest/issues/3046#issuecomment-1231555572, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFVSGQYD6D7ZB7XHH5SEGLV3XYGXANCNFSM5YNT2JEA . You are receiving this because you were mentioned.Message ID: @.***>

sksamuel avatar Sep 01 '22 00:09 sksamuel