Use initial Seed in CheckConfig and failure message. (#39)

* Use builder pattern for CheckConfig * Use initial Seed in CheckConfig and failure message. * Rewrites and sanitise some of the code to follow best TL/library best practices * Adds a useful error message when a property test fails to facilitate overriding the seed --------- Co-authored-by: Dario Abdulrehman <dabdulrehman@equalexperts.com> Co-authored-by: zainab-ali <zainab@kebab-ca.se>
typelevel · May 3, 2024 · 52916a5 · 52916a5
1 parent f9b5683
commit 52916a5
Show file tree

Hide file tree

Showing 5 changed files with 102 additions and 75 deletions.
diff --git a/modules/scalacheck/shared/src/main/scala/weaver/scalacheck/CheckConfig.scala b/modules/scalacheck/shared/src/main/scala/weaver/scalacheck/CheckConfig.scala
@@ -1,18 +1,40 @@
 package weaver
 package scalacheck
 
-case class CheckConfig(
-    minimumSuccessful: Int,
-    maximumDiscardRatio: Int,
-    maximumGeneratorSize: Int,
-    perPropertyParallelism: Int,
-    initialSeed: Option[Long]
-) {
+import org.scalacheck.rng.Seed
+import org.typelevel.scalaccompat.annotation.unused
+
+case class CheckConfig private (
+    val minimumSuccessful: Int,
+    val maximumDiscardRatio: Int,
+    val maximumGeneratorSize: Int,
+    val perPropertyParallelism: Int,
+    val initialSeed: Option[Seed]) {
   assert(maximumDiscardRatio >= 0)
   assert(maximumDiscardRatio <= 100)
   assert(minimumSuccessful > 0)
 
   def maximumDiscarded = minimumSuccessful * maximumDiscardRatio / 100
+
+  def withMinimumSuccessful(minimumSuccessful: Int) = copy(
+    minimumSuccessful = minimumSuccessful
+  )
+
+  def withMaximumDiscardRatio(maximumDiscardRatio: Int) = copy(
+    maximumDiscardRatio = maximumDiscardRatio
+  )
+
+  def withMaximumGeneratorSize(maximumGeneratorSize: Int) = copy(
+    maximumGeneratorSize = maximumGeneratorSize
+  )
+
+  def withPerPropertyParallelism(perPropertyParallelism: Int) = copy(
+    perPropertyParallelism = perPropertyParallelism
+  )
+
+  def withInitialSeed(initialSeed: Option[Seed]) = copy(
+    initialSeed = initialSeed
+  )
 }
 
 object CheckConfig {
@@ -23,4 +45,19 @@ object CheckConfig {
     perPropertyParallelism = 10,
     initialSeed = None
   )
+
+  def apply(
+      minimumSuccessful: Int,
+      maximumDiscardRatio: Int,
+      maximumGeneratorSize: Int,
+      perPropertyParallelism: Int,
+      initialSeed: Option[Seed]): CheckConfig =
+    new CheckConfig(minimumSuccessful,
+                    maximumDiscardRatio,
+                    maximumGeneratorSize,
+                    perPropertyParallelism,
+                    initialSeed)
+
+  @unused
+  private def unapply(c: CheckConfig): Some[CheckConfig] = Some(c)
 }
diff --git a/modules/scalacheck/shared/src/main/scala/weaver/scalacheck/Checkers.scala b/modules/scalacheck/shared/src/main/scala/weaver/scalacheck/Checkers.scala
@@ -106,54 +106,33 @@ trait Checkers {
 
     private def forall_[A: Show](gen: Gen[A], f: A => F[Expectations])(
         implicit loc: SourceLocation): F[Expectations] = {
-      paramStream
-        .parEvalMap(config.perPropertyParallelism) {
-          testOneTupled(gen, f)
-        }
-        .mapAccumulate(Status.start[A]) { case (oldStatus, testResult) =>
-          val newStatus = testResult match {
+      val params = Gen.Parameters.default.withNoInitialSeed.withSize(
+        config.maximumGeneratorSize)
+      val initialSeed = config.initialSeed.getOrElse(Seed.random())
+      seedStream(initialSeed)
+        .parEvalMap(config.perPropertyParallelism)(testOne(gen, f)(params, _))
+        .scan(Status.start[A]) { case (oldStatus, testResult) =>
+          testResult match {
             case TestResult.Success => oldStatus.addSuccess
             case TestResult.Discard => oldStatus.addDiscard
-            case TestResult.Failure(input, seed, exp) =>
-              oldStatus.addFailure(input, seed, exp)
+            case TestResult.Failure(input, exp) =>
+              oldStatus.addFailure(input, initialSeed, exp)
           }
-          (newStatus, newStatus)
         }
-        .map(_._1)
         .takeWhile(_.shouldContinue(config), takeFailure = true)
-        .takeRight(1) // getting the first error (which finishes the stream)
         .compile
-        .last
-        .map { (x: Option[Status[A]]) =>
-          x match {
-            case Some(status) => status.endResult(config)
-            case None         => Expectations.Helpers.success
-          }
-        }
-    }
-
-    private def paramStream: fs2.Stream[F, (Gen.Parameters, Seed)] = {
-      val initial = startSeed(
-        Gen.Parameters.default
-          .withSize(config.maximumGeneratorSize)
-          .withInitialSeed(config.initialSeed.map(Seed(_))))
-
-      fs2.Stream.iterate(initial) {
-        case (p, s) => (p, s.slide)
-      }
+        .lastOrError // This will never fail as there will always be at least one status
+        .map { status => status.endResult(config) }
     }
 
+    private def seedStream(initial: Seed): fs2.Stream[F, Seed] =
+      fs2.Stream.iterate[F, Seed](initial)(_.slide)
   }
 
   object forall extends PartiallyAppliedForall(checkConfig) {
     def withConfig(config: CheckConfig) = new PartiallyAppliedForall(config)
   }
 
-  private def testOneTupled[T: Show](
-      gen: Gen[T],
-      f: T => F[Expectations])(ps: (Gen.Parameters, Seed)) =
-    testOne(gen, f)(ps._1, ps._2)
-
   private def testOne[T: Show](
       gen: Gen[T],
       f: T => F[Expectations])(
@@ -165,19 +144,13 @@ trait Checkers {
         .map { (x: Option[(T, Expectations)]) =>
           x match {
             case Some((_, ex)) if ex.run.isValid => TestResult.Success
-            case Some((t, ex)) => TestResult.Failure(t.show, seed, ex)
+            case Some((t, ex)) => TestResult.Failure(t.show, ex)
             case None          => TestResult.Discard
           }
         }
     }
   }
 
-  def startSeed(params: Gen.Parameters): (Gen.Parameters, Seed) =
-    params.initialSeed match {
-      case Some(seed) => (params.withNoInitialSeed, seed)
-      case None       => (params, Seed.random())
-    }
-
   private[scalacheck] case class Status[T](
       succeeded: Int,
       discarded: Int,
@@ -192,7 +165,10 @@ trait Checkers {
         val ith = succeeded + discarded + 1
         val failure = Expectations.Helpers
           .failure(
-            s"Property test failed on try $ith with seed ${seed} and input $input")
+            s"""Property test failed on try $ith with seed $seed and input $input.
+                |You can reproduce this by adding the following override to your suite:
+                |
+                |override def checkConfig = super.checkConfig.withInitialSeed($seed.toOption)""".stripMargin)
           .and(exp)
         copy(failure = Some(failure))
       } else this
@@ -242,7 +218,7 @@ object Checkers {
   private object TestResult {
     case object Success extends TestResult
     case object Discard extends TestResult
-    case class Failure(input: String, seed: Seed, exp: Expectations)
+    case class Failure(input: String, exp: Expectations)
         extends TestResult
   }
 }
diff --git a/modules/scalacheck/shared/src/test/scala/weaver/scalacheck/CheckersConcurrencyTest.scala b/modules/scalacheck/shared/src/test/scala/weaver/scalacheck/CheckersConcurrencyTest.scala
@@ -19,9 +19,10 @@ object CheckersConcurrencyTest extends SimpleIOSuite {
     object CheckersConcurrencyTestNested extends SimpleIOSuite with Checkers {
 
       override def checkConfig: CheckConfig =
-        super.checkConfig.copy(perPropertyParallelism = minTests * 5,
-                               minimumSuccessful = minTests,
-                               maximumDiscardRatio = 10)
+        super.checkConfig
+          .withPerPropertyParallelism(minTests * 5)
+          .withMinimumSuccessful(minTests)
+          .withMaximumDiscardRatio(10)
 
       loggedTest("nested") { log =>
         val atomicInt = new AtomicInteger(0)
@@ -57,9 +58,9 @@ object CheckersConcurrencyTest extends SimpleIOSuite {
     object CheckersConcurrencyTestNested extends SimpleIOSuite with Checkers {
 
       override def checkConfig: CheckConfig =
-        super.checkConfig.copy(perPropertyParallelism = 50,
-                               minimumSuccessful = 10,
-                               maximumDiscardRatio = 10)
+        super.checkConfig.withPerPropertyParallelism(50)
+          .withMinimumSuccessful(10)
+          .withMaximumDiscardRatio(10)
 
       test("nested") {
         val atomicInt = new AtomicInteger(0)

diff --git a/modules/scalacheck/shared/src/test/scala/weaver/scalacheck/CheckersTest.scala b/modules/scalacheck/shared/src/test/scala/weaver/scalacheck/CheckersTest.scala
@@ -10,7 +10,7 @@ import org.scalacheck.Gen
 object CheckersTest extends SimpleIOSuite with Checkers {
 
   override def checkConfig: CheckConfig =
-    super.checkConfig.copy(perPropertyParallelism = 100)
+    super.checkConfig.withPerPropertyParallelism(100)
 
   test("universal") {
     forall(Gen.posNum[Int]) { a =>

diff --git a/modules/scalacheck/shared/src/test/scala/weaver/scalacheck/PropertyDogFoodTest.scala b/modules/scalacheck/shared/src/test/scala/weaver/scalacheck/PropertyDogFoodTest.scala
@@ -8,6 +8,7 @@ import cats.effect.{ IO, Resource }
 import weaver.framework._
 
 import org.scalacheck.Gen
+import org.scalacheck.rng.Seed
 
 object PropertyDogFoodTest extends IOSuite {
 
@@ -24,24 +25,29 @@ object PropertyDogFoodTest extends IOSuite {
         case LoggedEvent.Error(msg) => msg
       }
       exists(errorLogs) { log =>
+        val seed = Meta.FailedChecks.initialSeed.toBase64
         // Go into software engineering they say
         // Learn how to make amazing algorithms
         // Build robust and deterministic software
-        val (attempt, value, seed) =
+        val (attempt, value) =
           if (ScalaCompat.isScala3) {
-            ("4",
-             "-2147483648",
-             """Seed.fromBase64("AkTFK0oQzv-BOkf-rqnsdb_Etapzkj9gQD9rHj7UnKM=")""")
+            ("4", "-2147483648")
           } else {
-            ("2",
-             "0",
-             """Seed.fromBase64("Nj62qCHF96VYEMGcD2OBlfmuyihbPQQhQLH9acYL5RA=")""")
+            ("2", "0")
           }
 
-        val expectedMessage =
-          s"Property test failed on try $attempt with seed $seed and input $value"
-
-        expect(log.contains(expectedMessage))
+        val actualLines = log.split(System.lineSeparator()).toList
+        val expectedLines = s"""foobar
+          |Property test failed on try $attempt with seed Seed.fromBase64("$seed") and input $value.
+          |You can reproduce this by adding the following override to your suite:
+          |
+          |override def checkConfig = super.checkConfig.withInitialSeed(Seed.fromBase64("$seed").toOption)"""
+          .stripMargin
+          .linesIterator.toList
+
+        forEach(actualLines.zip(expectedLines))({ case (actual, expected) =>
+          expect(actual.contains(expected))
+        })
       }
     }
   }
@@ -102,13 +108,17 @@ object Meta {
 
     override def checkConfig: CheckConfig =
       super.checkConfig
-        .copy(perPropertyParallelism = 100, minimumSuccessful = 100)
+        .withPerPropertyParallelism(100)
+        .withMinimumSuccessful(100)
   }
 
   object FailedChecks extends SimpleIOSuite with Checkers {
 
+    val initialSeed = Seed(5L)
     override def checkConfig: CheckConfig =
-      super.checkConfig.copy(perPropertyParallelism = 1, initialSeed = Some(5L))
+      super.checkConfig
+        .withPerPropertyParallelism(1)
+        .withInitialSeed(Some(initialSeed))
 
     test("foobar") {
       forall { (x: Int) =>
@@ -127,8 +137,9 @@ object Meta {
   object ConfigOverrideChecks extends DiscardedChecks {
 
     val configOverride =
-      super.checkConfig.copy(minimumSuccessful = 200,
-                             perPropertyParallelism = 1)
+      super.checkConfig
+        .withMinimumSuccessful(200)
+        .withPerPropertyParallelism(1)
 
     override def partiallyAppliedForall: PartiallyAppliedForall =
       forall.withConfig(configOverride)
@@ -139,8 +150,10 @@ object Meta {
     override def partiallyAppliedForall: PartiallyAppliedForall = forall
 
     override def checkConfig =
-      super.checkConfig.copy(minimumSuccessful = 100,
-                             // to avoid overcounting of discarded checks
-                             perPropertyParallelism = 1)
+      super.checkConfig
+        .withMinimumSuccessful(100)
+        .withPerPropertyParallelism(
+          1
+        ) // to avoid overcounting of discarded checks
   }
 }