From dfcbf450d38dd96f5bde16b2b793ac43e4449b2d Mon Sep 17 00:00:00 2001 From: Oliver Wickham Date: Wed, 29 Jun 2022 09:19:40 +0100 Subject: [PATCH] Improve determinism/coverage in Scalacheck tests Change the behaviour of the property test runner to be more fair. Previously faster generators were more likely to be included in the results than slower generators. This also meant non-determinism if a test passes or fails --- build.sbt | 2 +- .../src/weaver/scalacheck/Checkers.scala | 41 +++++--- .../scalacheck/CheckersConcurrencyTest.scala | 96 +++++++++++++++++++ 3 files changed, 123 insertions(+), 16 deletions(-) create mode 100644 modules/scalacheck/test/src-ce3/weaver/scalacheck/CheckersConcurrencyTest.scala diff --git a/build.sbt b/build.sbt index e709cf6a..876bc1cd 100644 --- a/build.sbt +++ b/build.sbt @@ -246,7 +246,7 @@ lazy val scalacheck = projectMatrix testFrameworks := Seq(new TestFramework("weaver.framework.CatsEffect")), libraryDependencies ++= Seq( "org.scalacheck" %%% "scalacheck" % Version.scalacheck - ) + ) ++ Seq("org.typelevel" %%% "cats-effect-testkit" % Version.CE3.cats % Test).filter(_ => virtualAxes.value.contains(CatsEffect3Axis)) ) lazy val specs2 = projectMatrix diff --git a/modules/scalacheck/src/weaver/scalacheck/Checkers.scala b/modules/scalacheck/src/weaver/scalacheck/Checkers.scala index a449ef89..e2cecf33 100644 --- a/modules/scalacheck/src/weaver/scalacheck/Checkers.scala +++ b/modules/scalacheck/src/weaver/scalacheck/Checkers.scala @@ -7,8 +7,6 @@ import cats.{ Applicative, Defer, Show } import org.scalacheck.rng.Seed import org.scalacheck.{ Arbitrary, Gen } -import CECompat.Ref - trait Checkers { self: EffectSuiteAux => import Checkers._ @@ -104,15 +102,24 @@ trait Checkers { def apply[A: Show, B: PropF](gen: Gen[A])(f: A => B)( implicit loc: SourceLocation): F[Expectations] = - Ref[F].of(Status.start[A]).flatMap(forall_(gen, liftProp(f))) + forall_(gen, liftProp(f)) private def forall_[A: Show](gen: Gen[A], f: A => F[Expectations])( - state: Ref[F, Status[A]])( implicit loc: SourceLocation): F[Expectations] = { paramStream - .parEvalMapUnordered(config.perPropertyParallelism) { - testOneTupled(gen, state, f) + .parEvalMap(config.perPropertyParallelism) { + testOneTupled(gen, f) + } + .mapAccumulate(Status.start[A]) { case (oldStatus, testResult) => + val newStatus = testResult match { + case TestResult.Success => oldStatus.addSuccess + case TestResult.Discard => oldStatus.addDiscard + case TestResult.Failure(input, seed, exp) => + oldStatus.addFailure(input, seed, exp) + } + (newStatus, newStatus) } + .map(_._1) .takeWhile(_.shouldContinue, takeFailure = true) .takeRight(1) // getting the first error (which finishes the stream) .compile @@ -144,27 +151,24 @@ trait Checkers { private def testOneTupled[T: Show]( gen: Gen[T], - state: Ref[F, Status[T]], f: T => F[Expectations])(ps: (Gen.Parameters, Seed)) = - testOne(gen, state, f)(ps._1, ps._2) + testOne(gen, f)(ps._1, ps._2) private def testOne[T: Show]( gen: Gen[T], - state: Ref[F, Status[T]], f: T => F[Expectations])( params: Gen.Parameters, - seed: Seed): F[Status[T]] = { + seed: Seed): F[TestResult] = { Defer[F](self.effect).defer { gen(params, seed) .traverse(x => f(x).map(x -> _)) - .flatTap { (x: Option[(T, Expectations)]) => + .map { (x: Option[(T, Expectations)]) => x match { - case Some((_, ex)) if ex.run.isValid => state.update(_.addSuccess) - case Some((t, ex)) => state.update(_.addFailure(t.show, seed, ex)) - case None => state.update(_.addDiscard) + case Some((_, ex)) if ex.run.isValid => TestResult.Success + case Some((t, ex)) => TestResult.Failure(t.show, seed, ex) + case None => TestResult.Discard } } - .productR(state.get) } } @@ -233,4 +237,11 @@ object Checkers { } } + private sealed trait TestResult + private object TestResult { + case object Success extends TestResult + case object Discard extends TestResult + case class Failure(input: String, seed: Seed, exp: Expectations) + extends TestResult + } } diff --git a/modules/scalacheck/test/src-ce3/weaver/scalacheck/CheckersConcurrencyTest.scala b/modules/scalacheck/test/src-ce3/weaver/scalacheck/CheckersConcurrencyTest.scala new file mode 100644 index 00000000..663c5144 --- /dev/null +++ b/modules/scalacheck/test/src-ce3/weaver/scalacheck/CheckersConcurrencyTest.scala @@ -0,0 +1,96 @@ +package weaver +package scalacheck + +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger + +import scala.concurrent.duration._ + +import cats.effect.Outcome._ +import cats.effect.testkit.TestControl +import cats.effect.{ IO, Ref } +import cats.syntax.all._ + +import weaver.TestStatus + +import org.scalacheck.Gen + +object CheckersConcurrencyTest extends SimpleIOSuite { + test("tests should wait for slower tests to succeed before completion") { + + val minTests = 10 + object CheckersConcurrencyTestNested extends SimpleIOSuite with Checkers { + + override def checkConfig: CheckConfig = + super.checkConfig.copy(perPropertyParallelism = minTests * 5, + minimumSuccessful = minTests, + maximumDiscardRatio = 10) + + loggedTest("nested") { log => + val atomicInt = new AtomicInteger(0) + forall(Gen.delay(Gen.const(atomicInt.incrementAndGet()))) { i => + val sleepFor = + Duration.create(math.max(0L, minTests - i.toLong), TimeUnit.SECONDS) + IO.sleep(sleepFor) *> log.info(s"Ran gen $i").as(success) + } + } + } + + val expectedLogs = (1 to minTests).map(i => s"Ran gen $i").toList + TestControl.execute(runSuite(CheckersConcurrencyTestNested)) + .flatMap { control => + control.advanceAndTick(1.second).replicateA(minTests) *> control.results + } + .map { + case Some(Succeeded(r)) => + expect(r.count(_.status == TestStatus.Success) == 1) and + expect(r.forall(_.status == TestStatus.Success)) and + expect.same( + expectedLogs, + expectedLogs.intersect(r.flatMap(_.log.toList.map(_.msg))) + ) + case Some(Errored(e)) => failure(e.toString) + case Some(Canceled()) => failure("property test was cancelled") + case None => failure("property test didn't complete") + } + } + + test("tests should wait for slower tests to fail before completion") { + + object CheckersConcurrencyTestNested extends SimpleIOSuite with Checkers { + + override def checkConfig: CheckConfig = + super.checkConfig.copy(perPropertyParallelism = 50, + minimumSuccessful = 10, + maximumDiscardRatio = 10) + + test("nested") { + val atomicInt = new AtomicInteger(0) + forall(Gen.delay(Gen.const(atomicInt.incrementAndGet()))) { i => + if (i == 1) IO.sleep(1.second).as(failure("first one fails")) + else IO.pure(success) + } + } + } + + TestControl.execute(runSuite(CheckersConcurrencyTestNested)) + .flatMap { control => + control.tick *> control.advanceAndTick(1.second) *> control.results + } + .map { + case Some(Succeeded(r)) => + expect(r.count(_.status == TestStatus.Failure) == 1) and + expect(r.forall(_.status == TestStatus.Failure)) + case Some(Errored(e)) => failure(e.toString) + case Some(Canceled()) => failure("property test was cancelled") + case None => failure("property test didn't complete") + } + } + + private def runSuite(suite: SimpleIOSuite) = for { + ref <- Ref.of(List.empty[TestOutcome]) + _ <- suite.run(Nil)(result => + ref.update(result :: _)) + results <- ref.get + } yield results +}