Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2rs2ts opened this issue Jan 7, 2015 · 6 comments
Open

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

2rs2ts opened this issue Jan 7, 2015 · 6 comments

Comments

@2rs2ts
Copy link

2rs2ts commented Jan 7, 2015

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
Copy link
Contributor

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.

@rickynils rickynils self-assigned this Jan 14, 2015
@rickynils rickynils added this to the 2.0.0 milestone Jan 14, 2015
@mlangc
Copy link

mlangc commented May 27, 2015

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
    )

@rickynils rickynils removed their assignment Nov 18, 2016
@dvtomas
Copy link

dvtomas commented Dec 7, 2016

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

rtyley added a commit to rtyley/levenshtein that referenced this issue Jul 9, 2018
Scalacheck was right to tell me something was wrong - but
generating zero-display-width characters in Gen.alphaStr was
definitely UNHELPFUL.

typelevel/scalacheck#129
https://twitter.com/rtyley/status/1016244507761889280
@charleso
Copy link

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 added a commit to charleso/scalacheck that referenced this issue Nov 23, 2018
Shrinking is only safe when used with Arbitrary.
Generators can have a smaller range than the global shrinking,
which results in invalid shrunken values.

This fixes issues like typelevel#129.
@charleso
Copy link

charleso commented Nov 23, 2018

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
Copy link
Contributor

ashawley commented Dec 7, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants