Use initial Seed in CheckConfig and failure message. #39
Conversation
| case class CheckConfig( | ||
| minimumSuccessful: Int, | ||
| maximumDiscardRatio: Int, | ||
| maximumGeneratorSize: Int, | ||
| perPropertyParallelism: Int, | ||
| initialSeed: Option[Long] | ||
| initialSeed: Option[Seed] |
There was a problem hiding this comment.
I noticed that this was already implemented in disneystreaming/weaver-test#633, which also future-proofs the CheckConfig datatype.
Perhaps we should retarget that instead?
There was a problem hiding this comment.
The future-proofing of CheckConfig would be greatly appreciated. Feel free to retarget it need be (or copy and credit the original author)
| @@ -106,22 +106,18 @@ trait Checkers { | |||
|
|
|||
| private def forall_[A: Show](gen: Gen[A], f: A => F[Expectations])( | |||
| implicit loc: SourceLocation): F[Expectations] = { | |||
There was a problem hiding this comment.
The changes here are a bit dense. To summarize:
paramStream: fs2.Stream[F, (Gen.Parameters, Seed)]has a fixedGen.Parametersvalue, so can be better expressed as(Gen.Parameters, Stream[F, Seed]).
Since we need the first seed, I've expressed it as:val (params, initialSeed) = startParams seedStream(initialSeed)
mapAccumulate(...).map(_.1)can be reduced toscan.takeRight(1).compile.lastcan be reduced tocompile.last.
| .takeWhile(_.shouldContinue(config), takeFailure = true) | ||
| .takeRight(1) // getting the first error (which finishes the stream) | ||
| .compile | ||
| .last | ||
| .map { (x: Option[Status[A]]) => |
There was a problem hiding this comment.
This stream will always contain at least one value due to the properties of scan, so the None case will never be hit.
We could use lastOrError here instead. What's your preference?
| private def paramStream: fs2.Stream[F, (Gen.Parameters, Seed)] = { | ||
| val initial = startSeed( | ||
| private def startParams: (Gen.Parameters, Seed) = { | ||
| startSeed( |
There was a problem hiding this comment.
Question: do we need the startSeed function to be public? It seems a bit odd for users to want to override it.
There was a problem hiding this comment.
as long as the user as a way of overriding the seed to reproduce failed tests deterministically, it's fine (ie the startX function shouldn't be public)
| } | ||
|
|
||
| val expectedMessage = | ||
| s"Property test failed on try $attempt with seed $seed and input $value" | ||
|
|
||
| s"""Property test failed on try $attempt with seed Seed.fromBase64("$seed") and input $value""" |
There was a problem hiding this comment.
We should probably improve the message by displaying the method override necessary for the seed value to be injected in the suite (I think munit does that). That'd improve the UX
|
|
||
| override def productArity: Int = 5 | ||
|
|
||
| override def productElement(n: Int): Any = n match { |
There was a problem hiding this comment.
mmm, I had not remembered the implementation on the PR you merged in, not a fan of how it was done here.
It's fine to use a case-class implementation (to get toString/equals/hashCode), as long as the constructor is made private (to hide copy methods), the default extractor is overriden as described here, and an explicit apply method is provided in the companion. It's less error-prone.
There was a problem hiding this comment.
Thanks! That article is incredibly valuable.
|
Great work 👍, just one last nitpick on the config data class |
| @@ -3,104 +3,37 @@ package scalacheck | |||
|
|
|||
| import org.scalacheck.rng.Seed | |||
|
|
|||
| final class CheckConfig( | |||
There was a problem hiding this comment.
you still need the unapply override in the companion object, which signals to the compiler that the traditional case class extractor should be eluded. Otherwise adding fields changes the shape of the generated unapply in non bincompat ways.
IE private def unapply(c: CheckConfig): Some[CheckConfig] = Some(c)
This PR:
org.scalacheck.rng.Seedinstead ofLongdisneystreaming/weaver-test#632Checkers.forall_logic, in particular the stream.