Skip to content

Commit

Permalink
Improve determinism/coverage in Scalacheck tests
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ollyw committed Jun 29, 2022
1 parent 9d165c5 commit dfcbf45
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 16 deletions.
2 changes: 1 addition & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
41 changes: 26 additions & 15 deletions modules/scalacheck/src/weaver/scalacheck/Checkers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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._
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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
}
}
Original file line number Diff line number Diff line change
@@ -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
}

0 comments on commit dfcbf45

Please sign in to comment.