ZIO Test: Add Gen Combinators #1356

adamgfraser · 2019-08-06T00:02:16Z

Adds Gen combinators. There are a lot of directions to go here and we're still fleshing out the API so wanted to share and get feedback.

A couple of observations from my end:

Generating random values works well. Very compositional. Definitely a lot more methods we could add here but feels like we have the right building blocks.
I'm a bit worried we don't have quite the right structures to support efficient shrinking. Right now we are representing possible shrinks as just a stream of potential shrinks. The issue here is that often there are multiple "paths" we can go down to do a shrink (e.g. shrinking a list by shrinking an element versus reducing the size of the list) and with just a flat list it is hard to make sure we go down the right path and know when to terminate.
I wonder if we need to do more to distinguish finite from random generators. The machinery definitely supports both of them, but unintentionally combining the two can often lead to poor results. For example, we have a union operation that switches pulling values from two generators. It works as you would expect for two finite generators or two random generators, but the result can be unintuitive when combining a finite and random generator because a random generator is conceptually just a stream of one effectual value. Maybe a different namespace for some of the different constructors?

With regard to the second point maybe we could do something like:

final case class Gen[R, A](sample: ZStream[R, Nothing, Sample[R, A]])

final case class Sample[R, A](value: A, shrinks: ZStream[R, Nothing, Sample[R, A])

That way the shrinks are a recursive tree instead of a flat list.

@jdegoes Definitely open to feedback on this. I may just not be getting the right way to implement some of these combinators within our structure.

jdegoes · 2019-08-07T09:35:46Z

test/shared/src/main/scala/zio/test/Gen.scala

+  final val boolean: Gen[Random, Boolean] =
+    choose(false, true)
+
+  final def char(range: Range): Gen[Random, Char] =


Would be useful to add identifierChar, printableChar, unicodeChar, etc.

jdegoes · 2019-08-07T09:37:22Z

test/shared/src/main/scala/zio/test/Gen.scala

+  final def constSample[R, A](sample: => Sample[R, A]): Gen[R, A] =
+    fromEffectSample(ZIO.succeedLazy(sample))
+
+  final def double(min: Double, max: Double): Gen[Random, Double] =


Useful to add float, short, long, etc. Maybe the bounded variants, but also: anyDouble, anyChar, anyShort, anyLong, anyFloat, etc.

Yes. One potential issue with the bounded variants for types like Long and Double is that they are "bigger" than the Int that underlies range. I wonder if we should roll our own simple range for this so we can be consistent across numeric types or just use something like min, max, step.

jdegoes · 2019-08-07T09:38:07Z

test/shared/src/main/scala/zio/test/Gen.scala

+
+  final def oneOf[R, A](a: Gen[R, A], as: Gen[R, A]*): Gen[R with Random, A] =
+    int(0 to as.length).flatMap(n => if (n == 0) a else as(n - 1))
+
  /**
   * A sized generator, whose size falls within the specified bounds.
   */
  final def sized[R <: Random, A](min: Int, max: Int)(f: Int => Gen[R, A]): Gen[R, A] =


Probably you can refactor the min: Int, max: Int to be a Range like the other combinators you introduced.

jdegoes · 2019-08-07T09:39:07Z

test/shared/src/main/scala/zio/test/Gen.scala

+    gen1.zipAll(gen2).flatMap {
+      case (a, a2) =>
+        Gen {
+          a.fold[Gen[R, A]](empty)(const(_)).sample ++


Maybe interleave would be more appropriate than ++?

Yes as soon as we get that merged we can simplify this a lot and implement some other combinators like weighted. 😃

jdegoes · 2019-08-07T09:46:27Z

test/shared/src/main/scala/zio/test/Gen.scala

+  final def flatMap[R1 <: R, B](f: A => Gen[R1, B]): Gen[R1, B] = Gen {
+    sample.flatMap { a =>
+      f(a.value).sample.map { b =>
+        val rest = a.shrink.flatMap(a => f(a).sample).flatMap(sample => ZStream(sample.value) ++ sample.shrink)


I wonder what the most useful way to define this is...

I think this is one of the key issues. I think, though I could be wrong, that this is the only lawful way to define it for potentially finite streams. The challenge with the stream based flatMap (at least when the stream is not just a single element that generates a random effect) is that we have to essentially traverse all the children of the first possibility before we can explore other possibilities. So like:

for { x <- Stream(1, 2, 3) y <- Stream(4, 5, 6) } yield (x, y) // Stream((1, 4), (1, 5), (1, 6), (2, 4), ...)

We get all the 1 possibilities before we see other possibilities whereas a lot of the time we want a more balanced distribution which is where combinators like union come in to get a more balanced distribution.

The other thing we could explore is the infinite stream instance where unit is repeating the same value and flatMap is taking the diagonal but I'm not sure that is where we want to go.

jdegoes · 2019-08-07T09:49:47Z

@adamgfraser

That way the shrinks are a recursive tree instead of a flat list.

I am open to this. The common libraries just use a flat list. In a PureScript library (strongcheck), I once experimented with something more sophisticated — a tree in which you could explore by distance from your current location.

The tree you suggest basically gives you 2 dimensions: the horizontal one can be used for "alternates", the vertical one for "sizes".

Note that we can actually reconstruct such a tree from the flat shrink list and a perturb function, perturb: (A, Double) => ZIO[R, Nothing, A]. I'm not sure it's worth it to formulate it that way, but it's worth keeping in mind there are different choices of bases.

adamgfraser · 2019-08-09T19:14:43Z

@jdegoes Hedgehog actually uses a shrink tree. So does Hypothesis in Python and test.check in Clojure. I think your concept about perturbing the shrink list is really interesting. Even in Hedgehog with the shrink tree special logic is needed when building structures like lists to get them to shrink efficiency versus being able to just do something like replicateM. So my takeaway would be the listOf method needs to be responsible for returning an efficient versus naive shrink list and we see how far we can get with that. I agree we should explore the full functionality we can get from our existing structure before changing it. Let me work on this.

jdegoes · 2019-08-10T07:00:14Z

Hedgehog actually uses a shrink tree.

Ah, I didn't know that. I'm fine either way on this. I do agree the extra dimension (the tree) gives you ability to find better minimized examples. Won't have a major impact on end-user API.

Let me know when you're ready for a review!

adamgfraser · 2019-08-10T13:00:45Z

Will do.

The other thing to keep in mind is that a lot of shrinking approaches that seem to use shrink lists actually use trees. Like ScalaCheck and QuickCheck look like they use a shrink list with a type that is roughly f: A => Stream[A]. But since f can take any A they can always apply it to any element of the original shrink list to construct the part of the shrink tree they want to explore.

Thinking about it more, the key idea is that we need to be able to express an idea of conditionality ("if this shrink still fails the test keep shrinking from there, otherwise backtrack and try shrinking some other way"). With just a list we can't do that. The shrink list has to be monotonically decreasing in some concept of "size" because when you do the shrink search in a method like checkStream you are just going to take the last failing shrink out of how many you explore. But if it is monotonically decreasing we have to go through all the larger possibilities, since they ultimately might be the best shrink, before the smaller ones, which means we are very inefficient.

jdegoes · 2019-08-11T06:52:44Z

Thinking about it more, the key idea is that we need to be able to express an idea of conditionality ("if this shrink still fails the test keep shrinking from there, otherwise backtrack and try shrinking some other way"). With just a list we can't do that.

Excellent point. This agues for a tree IMO. Then we can keep compositional shrinking with "backtracking".

adamgfraser · 2019-08-11T10:23:38Z

I agree. I am working on it.

adamgfraser · 2019-08-13T22:34:31Z

@jdegoes This is ready for another review.

jdegoes · 2019-08-14T07:03:41Z

test/shared/src/main/scala/zio/test/Gen.scala

+        .map(Sample.shrinkIntegral(min))
+    }
+
+  final def listOf[R, A](min: Int, max: Int)(g: Gen[R, A]): Gen[R with Random, List[A]] =


I was thinking it could be more compositional to make the "sized" variants like this:

final def listOf(g: Gen[R, A]): Int => Gen[R with Random, List[A]]`

Then you use them like this:

import Gen.{ listOf, sized } sized(0, 10)(listOf(boolean))

What do you think?

By the way, a new pattern that infers better is using R <: Random on the type declaration, rather than R with Random on the output.

I like that.

It potentially takes us in the direction of being able to have test modes where the size parameter is controlled by the runner versus the test, which a lot of other frameworks have but we don't yet. You could imagine having an alias for Int => Gen[R, A], maybe SGen[R, A] for a Gen that takes a size parameter. Then we could have a checkSized method that took an SGen, feed it a series of sizes, and checked each one.

The one issue I see here is that if you go down this road the next step is that you want to be able to compose sized generators, which works but would require writing a ton more code to make methods available on sized generators in addition to normal ones. One other way we could approach it is to use the environment type:

final case class Size(n: Int) final def listOf[R <: Random with Size, A](g: Gen[R, A]): Gen[R, List[A]] = Gen.fromEffect(ZIO.access[Size](_.n)).flatMap(listOfN(_)(g))

Then sizes naturally compose together unless you want to provide a different size parameter locally.

Depending on where we go with this the listOfN, StringN and vectorOfN methods may be redundant with the new "sized" variants (they would be literally identical if the function versions if we just curried the parameters in the other order) so I think we take them out unless we go in a different direction here.

I agree this makes sense. Although for namespacing reasons, it would probably be:

trait Size { def size: Int }

Or maybe even better:

trait TestConfig { def testConfig: TestConfig.Service }

...since we could stick other settings in there, as well.

I think we need to do this at some point, question is: 1.0 or after 1.0?

Global size parameters were kinda abused in QuickCheck (et al). What size do you need for trees? What size do you need for 2D arrays? Is one global default really suitable for all cases?

Basically people use sized for convenience even when it didn't make sense, yielding some suboptimal tests. At least being explicit now, burden is on the user for choosing correct settings.

What I would really like to see in v1.0 and which should be easy is "small", "medium", and "large" combinators, e.g. small(listOf(...)), where the bigger ones spend more time on smaller sizes, but bump up to bigger ones once in a while (exponential or quadratic distribution).

I agree with you about the dangers of relying too much on a global "size" parameter. My initial hypothesis would be that only collection types should have specific sized combinators (e.g. listOf) and in other cases users can always use the generic sized combinator.

On the naming, the downside of using something like TestConfig is that it hides what the actual dependencies of a generator are. I really like how in the current API you can see right away whether a method generates random values by the Random environment parameter. I think it could be nice to do the same thing with Size so you can see that methods like listOf take a size parameter and other ones don't. Of course the downside is that if we end up having a lot more dependencies the type signatures get very verbose, but I'm not sure there are any other dependencies that generic test combinators should have to rely on.

My vote would be for getting this in now if we think it makes sense. The code base is still small and we can iterate quite quickly so might as well lay a strong foundation now so we are building in the right direction.

Shall I submit a PR on this so we can work through it?

@adamgfraser All right, let's do it!

I'd propose:

trait SizedGen { def sizedGen: FiberRef[Int] }

then we can augment the managed mock environment with SizedGen using some suitable default.

The sized combinator now can simply require R <: SizedGen, and we can use that in the list* etc. methods.

Sound reasonable?

jdegoes · 2019-08-14T07:04:52Z

test/shared/src/main/scala/zio/test/Gen.scala

+  /**
+   * A generator of optional values. Shrinks toward `None`.
+   */
+  final def option[R, A](gen: Gen[R, A]): Gen[R with Random, Option[A]] =


R <: Random on the type declaration to infer better, here and elsewhere.

jdegoes · 2019-08-14T07:07:38Z

test/shared/src/main/scala/zio/test/Gen.scala

+  final def some[R, A](gen: Gen[R, A]): Gen[R, Option[A]] =
+    gen.map(Some(_))
+
+  final def string[R](min: Int, max: Int)(char: Gen[R, Char]): Gen[R with Random, String] =


string[R <: Random](char: Gen[R, Char]): Int => Gen[R, String]

etc.

jdegoes · 2019-08-14T07:08:46Z

test/shared/src/main/scala/zio/test/Sample.scala

 */
-final case class Sample[-R, +A](value: A, shrink: ZStream[R, Nothing, A]) { self =>
-  final def <*>[R1 <: R, B](that: Sample[R1, B]): Sample[R1, (A, B)] = self zip that
+final case class Sample[-R, +A](value: A, shrink: ZStream[R, Nothing, Sample[R, A]]) { self =>


jdegoes · 2019-08-14T07:09:29Z

test/shared/src/main/scala/zio/test/Sample.scala

+        }
+    }
+
+  final def unfold[R, A, S](s: S)(f: S => (A, ZStream[R, Nothing, S])): Sample[R, A] = {


Nice handy functions!

jdegoes · 2019-08-14T07:10:50Z

test/shared/src/main/scala/zio/test/package.scala

+      .map(_.map(checkValue))
+      .dropWhile(!_.value.failure) // Drop until we get to a failure
+      .take(1)                     // Get the first failure
+      .flatMap(_.shrinkSearch(_.failure).take(maxShrinks))


Beautiful simplification. ❤️

jdegoes · 2019-08-14T07:11:49Z

test/shared/src/test/scala/zio/test/GenSpec.scala

+  def showTree[R, A](sample: Sample[R, A], offset: Int = 0): ZIO[R, Nothing, String] = {
+    val head = " " * offset + sample.value + "\n"
+    val tail = sample.shrink.mapM(showTree(_, offset + 2)).runCollect.map(_.mkString("\n"))
+    tail.map(head + _)


Awesome work on all these tests!

adamgfraser · 2019-08-14T16:28:56Z

@jdegoes I addressed all your review comments. For now I went with the approach to the "sized" combinators that you originally proposed.

jdegoes

Excellent work on this PR. Some really wonderful combinators and the tree-based shrinking is a massive improvement.

ghostdogpr · 2019-08-15T11:15:24Z

Looks like the test monad associativity fails in every build: https://circleci.com/workflow-run/48d931bc-e57d-43c7-9e96-36c499165353

adamgfraser · 2019-08-15T11:22:03Z

@ghostdogpr I don’t think I have permission to look at the link. Not sure what is going on. That test has been there through several iterations and all of the CI builds passed before merging, right?

ghostdogpr · 2019-08-15T11:24:56Z

@adamgfraser oops, the link shows that 12 builds out of 12 failed with this line:

- Gen
  + monad left identity
  + monad right identity
  - monad associativity <- in red

The build after your last change indeed passed, maybe there was another change applied on master in between that broke it?

ghostdogpr · 2019-08-15T11:29:26Z

This link is public: https://circleci.com/gh/zio/zio/16472

adamgfraser · 2019-08-15T11:31:15Z

Let me investigate.

mgibowski · 2019-08-15T11:38:52Z

@adamgfraser
In case it is useful to you, you can see this test failing on your computer by running:
testJVM/test:runMain zio.test.TestMain
inside of sbt console.

adamgfraser · 2019-08-15T12:10:11Z

I can reproduce locally. Trying to diagnose now.

@iravid @vasilmkd We're running into an issue with a failing monad laws associativity test for our Gen data type, which wraps a ZStream of Sample, which is basically a rose tree. The test passed on my branch which had changes pulled from master two days ago. Do you know of any issues from your efforts on reworking streams that could be causing this?

vasilmkd · 2019-08-15T12:18:03Z

We only ever merged changes that passed all checks in the CI. Looking now.

adamgfraser · 2019-08-15T12:20:44Z

Okay. Yeah, we just merged this so none of these checks would have been in the CI before.

vasilmkd · 2019-08-15T13:02:40Z

Hmmm, it may be a regression when we converted ZStream#map to the new InputStream mechanism. Reverting that change passes the tests. Relevant commit: acfec90

adamgfraser · 2019-08-15T13:11:42Z

Yes, I see the same thing. I wonder if it has something to do with the ordering of effects? Are you okay with reverting that change?

vasilmkd · 2019-08-15T13:12:03Z

Yes, please. Feel free to. I can't at the moment.

adamgfraser · 2019-08-15T13:12:27Z

Okay, will do now.

adamgfraser added 2 commits August 5, 2019 19:30

add Gen combinators

cb288d1

fix deprecation warning

7fadde5

jdegoes reviewed Aug 7, 2019

View reviewed changes

merge upstream changes

64c5646

adamgfraser added 2 commits August 13, 2019 18:13

implement tree

8d30a45

merge upstream changes

4aa8095

jdegoes reviewed Aug 14, 2019

View reviewed changes

address review comments

3bb9627

adamgfraser added 4 commits August 14, 2019 17:00

add weighted combinator

74944fa

fix failing test

e6261a0

fix deprecation warning

f6f78af

fix deprecated Double ordering

1cdaeef

jdegoes approved these changes Aug 15, 2019

View reviewed changes

jdegoes merged commit 63e458c into zio:master Aug 15, 2019

ghostdogpr mentioned this pull request Aug 15, 2019

ZIO Test: Add Gen combinators #1308

Closed

vasilmkd mentioned this pull request Aug 15, 2019

Convert more ZStream combinators to use ZStream#process #1401

Merged

adamgfraser mentioned this pull request Aug 15, 2019

Revert Regression in ZStream#map #1404

Merged

adamgfraser deleted the 1308 branch August 15, 2019 13:55

adamgfraser mentioned this pull request Aug 16, 2019

ZIO Test: Improve Support for Sized Generators #1407

Merged

ZIO Test: Add Gen Combinators #1356

ZIO Test: Add Gen Combinators #1356

Conversation

adamgfraser commented Aug 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdegoes commented Aug 7, 2019

adamgfraser commented Aug 9, 2019

jdegoes commented Aug 10, 2019

adamgfraser commented Aug 10, 2019

jdegoes commented Aug 11, 2019

adamgfraser commented Aug 11, 2019

adamgfraser commented Aug 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgfraser commented Aug 14, 2019

jdegoes left a comment

Choose a reason for hiding this comment

ghostdogpr commented Aug 15, 2019

adamgfraser commented Aug 15, 2019

ghostdogpr commented Aug 15, 2019

ghostdogpr commented Aug 15, 2019

adamgfraser commented Aug 15, 2019

mgibowski commented Aug 15, 2019

adamgfraser commented Aug 15, 2019

vasilmkd commented Aug 15, 2019

adamgfraser commented Aug 15, 2019

vasilmkd commented Aug 15, 2019

adamgfraser commented Aug 15, 2019

vasilmkd commented Aug 15, 2019 • edited

adamgfraser commented Aug 15, 2019

adamgfraser commented Aug 6, 2019 •

edited

vasilmkd commented Aug 15, 2019 •

edited