typelevel / scalacheck

Property-based testing for Scala
http://www.scalacheck.org
BSD 3-Clause "New" or "Revised" License
1.94k stars 407 forks source link

Gen.suchThat not respected by shrinking after Gen.map #129

Open 2rs2ts opened 9 years ago

2rs2ts commented 9 years ago

My team has a library which contains a bunch of predefined generators. Here's a simple one:

lazy val genNonEmptyAlphaStr: Gen[String] = Gen.nonEmptyListOf(Gen.alphaChar).map(_.mkString)

Unfortunately when I go to use it in practice, this doesn't get respected at all. (We use specs2 along with ScalaCheck, so bear with me.)

import org.scalacheck.Prop._
import org.specs2.{ScalaCheck, SpecificationLike}
import com.paypal.cascade.common.tests.scalacheck._

class ShrinkTestSpecs
  extends SpecificationLike
  with ScalaCheck { def is = s2"""
  lalal ${test}
  """

  def fail = throw new RuntimeException("aaa!")

  def test = {
    forAll(genNonEmptyAlphaStr) { s =>
      fail
      true must beTrue
    }
  }
}

This results in some output like

Testing started at 1:37 PM ...

A counter-example is '': java.lang.RuntimeException: aaa! (after 0 try)
java.lang.RuntimeException: aaa!

If I use forAllNoShrink, or if I write genNonEmptyAlphaStr.suchThat(_.size > 0), then there are no problems and I get single-letter strings. If I use Gen.nonEmptyListOf(Gen.alphaChar) (in other words, I don't map on it,) then I also have no problems. It's only after I .map(_.mkString) that it will shrink to the empty string.

tl;dr Gen.map invalidates suchThat.

rickynils commented 9 years ago

This is a known issue. With the current design it is not possible retain the suchThat filter after a map. This will hopefully change in ScalaCheck 2, where the generators have been redesigned to better handle these cases.

mlangc commented 9 years ago

Hmm, I guess this is the same issue then:

Using scalacheck-1.12.2 with scalatest-2.2.5 and scala-2.11.6 executing

 "Test scalacheck" in {
    val gen = for {
      n <- Gen.choose(1, 3)
      list <- Gen.listOfN(n, Gen.choose(0, 9))
    } yield list

    forAll(gen) { list =>
      list should not be (empty)
      if (list.sum < 18) throw new IllegalArgumentException("ups")
    }
  }

results in

TestFailedException was thrown during property evaluation.
Message: List() was empty
Location: (Wherever.scala:25)
  Occurred when passed generated values (
    arg0 = List() // 2 shrinks
  )

This problem is also mentioned on http://stackoverflow.com/questions/20037900/scalacheck-wont-properly-report-the-failing-case although the bug mentioned there was closed a while ago.

Here is another example where Gen.choose(...) is not respected:

"Test scalacheck" in {
    val gen = for {
      n <- Gen.choose(1, 5)
      list <- Gen.listOfN(n, Gen.choose(0, 5))
    } yield(n, list)

    forAll(gen) { case (n, list) =>
      if (list.sum > 18) throw new RuntimeException()
      n should not be (0)
    }
  }

results in

 TestFailedException was thrown during property evaluation.
    Message: 0 was equal to 0
    Location: (Wherever.scala:27)
    Occurred when passed generated values (
      arg0 = (0,List()) // 4 shrinks
    )
dvtomas commented 7 years ago

Yeah, I keep hitting this issue as well, very annoying.

charleso commented 6 years ago

Apologies for just repeating the same question slightly differently but isn't the fundamental problem that forAll takes in an implicit Shrink. Shrink in this case can't know about Gen's restrictions, such as being just an nonEmptyAlphaStr and will shrink blindly resulting in invalid cases.

  def forAll[T1,P](g1: Gen[T1])(f: T1 => P)(implicit s1: Shrink[T1]): Prop

FWIW The Haskell version of forAll doesn't shrink for Gen, only Arbitrary which to me seems like the less-confusing behaviour.

Is using the implicit Shrink intentional, or something that should potentially be removed given people keep hitting this issue (I've met others offline with the same problem)?

Having a separate gen and shrink mechanism is one of the (unfortunate) design flaws in the original QuickCheck. Not to hijack this issue for my own agenda but if people are interested it's something that has been "fixed" in a new-ish library Hedgehog which combines Gen/Shrink (Admission: I'm the current maintainer of the Scala Hedgehog port, but a long time QuickCheck/ScalaCheck user and admirer).

https://www.youtube.com/watch?v=AIv_9T0xKEo

charleso commented 6 years ago

As suggested by @ashawley (in gitter) here is a link to (my) scala implementation of the hedgehog concept/library in Scala.

https://github.com/hedgehogqa/scala-hedgehog

Sorry again for hijacking this thread to spruik a replacement/alternative to ScalaCheck. As I said earlier, I've been a long-time user and fan of this library!

ashawley commented 5 years ago

Indeed, shrinking that doesn't respect generators is confusing. It could probably be improved with documentation. This issue isn't covered in the ScalaCheck book, nor the online User Guide.

There is a proposal in #440 to disable shrinking by default in the next major release, 1.15.0. Technically, such a change could avoid breaking existing passing tests and preserve binary compatibility, but a user who upgrades to 1.15.0 and found out that shrinking was dismantled would be a pretty surprising change. It's a signicant enough change that is should probably ship in a 2.0 release.