scalatest's scalacheck forAll wrappers lose nested goodness about scalatest HOT 10 CLOSED

✨ This is an old work account. Please reference @brandonchinn178 for all future communication ✨

I'm seeing this behavior with scalatestplus and ScalaCheckPropertyChecks. Since Checkers is now deprecated, should this issue be reopened? Never mind, it seems like Checkers was deprecated in org.scalacheck.prop, but it's fine in scalatestplus

To summarize, here are the current drawbacks of nested forAlls that I've come across:

1. The minSuccessful param compounds (I'm running with minSuccessful = 100, so two levels of forAlls generate 10000 tests)
2. Exceptions aren't good. For the given test:

forAll { a =>            // line 1
  forAll { b =>          // line 2
    foo(a, b) == bar     // line 3
  }
}

I get a message

GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
Message: ...
Message: ...
Location: (Test.scala:3)
Occurred when passed generated values (
  a = 1
)
Location: (Test.scala:2)
Occurred when passed generated values (
  a = 2
)

The most confusing part here is the line numbers (the fact that both arguments are labelled a is because of how forAll is implemented; I'm not too concerned about that for right now). The bottom location points to the inner forAll but actually shows the generated value of the outer forAll (which errored at the inner forAll line).

from scalatest.

Yes, exceptions make things harder, but it may be possible. We do allow nesting of other constructs such as Inside, which catch and rethrow, adding more info along the way. I'll think about it. (This is also the kind of thing I'm hoping will be nicer with pure assertions that return a result instead of throwing an exception.) In the meantime, for nested properties you could just use ScalaCheck directly, possibly using org.scalatest.prop.Checkers.

from scalatest.

I'm not sure how you'd preserve enough information to flatten the distribution if you only return Unit, without heinous mutable state contexts. You'd want the inner forAll to be able to describe itself to the outer one.

from scalatest.

@scalatest-travis do you have hints about how you think it might be possible with exceptions? It might help others work on the ticket if you're unable to.

from scalatest.

Actually I didn't understand the issue when you first posted. (And I was logged in a scalatest-travis too, so...) I'll have to look at how ScalaCheck does that, but I'm guessing that the outer one drives the inner one when the type passed to what is forAll for us is also another forAll. That's indeed not going to be easy given the result type is Unit. If the result type were not Unit, then you could probably drive it that way, regardless of whether it throws an exception or not, but the trouble is that because of the exceptions people won't think they'll need to connect the assertions. They might write:

forAll[Int] { i =>
  forAll(Gen.choose(i, i +10)) { j =>
    // something with i and j
  }
  assert(/* something else */)
}

In fact they couldn't connect them if they wanted to. So in the above case the outer forAll would not detect the inner forAll by its result type. So that won't work.

One thing I believe you can do now is set the minSuccessful in the inner for:

forAll[Int] { i =>
  forAll(Gen.choose(i, i +10), minSuccessful(1)) { j =>
    // something with i and j
  }
}

I am working towards Pure assertions, assertions and matcher expressions that will return a result rather than throw an exception. For that forAll will be modified to accept different types, and can be made to behave like ScalaCheck. But I think for the traditional, exception-throwing assertions explicitly configuring the inner ones may be the best we can do.

from scalatest.

Unfortunately I don't just want to limit the inner generator though. I think the limited inner one can lead to a different distribution than considering them independently (if you consider rejections in particular) and is harder to reason about. I don't have a good solution unfortunately, but it seems like a pity to recreate functionality provided in another library in a way that materially impacts its usefulness (nested forAlls are pretty common and are generally more pleasant than complex Gens) :(

I guess I'll just add it to my list of reasons why exceptions and weird control flow are evil and non-compositional :)

from scalatest.

I didn't realize ScalaCheck did this with nested properties, and don't quite understand yet what it is actually doing. I'll study it to learn further. The point of GeneratorDrivenPropertyChecks was twofold. 1) to make it so you didn't have to use labels, which I felt cluttered up the expression making it harder to read, and 2) to make the properties more consistent with regular assertions in ScalaTest. I think that seems to work OK for people who don't nest properties. Now that I know this about nested properties, I may want to point people who do that a lot to Checkers instead of GeneratorDrivenPropertyChecks. Checkers was the original integration point with ScalaCheck, and it will solve your issue. Plus it has a shorter, cuter name.

Yeah, exceptions have been somewhat a pain to work with, and we've had do to a few tricks of catching and rethrowing to modify error messages and what not. Some examples are the inside and withClue constructs. Until now we've been able to do everything we wanted one way or another, but in the nested property case I think we're out of luck. I'd suggest you give Checkers a try and see if that doesn't work better for you. Let me know.

from scalatest.

In some situations it might be useful that the inner forAll property is independent of the outer one. For example if you use the outer one to create something (computationally) expensive to test (e.g. a tree) and the inner one to check the "created something" (e.g. arbitrary access paths into the tree).

from scalatest.

I'm going to close this one for now. The recommendation will be that if you are nesting properties you might be better off with Checkers instead of ProperyChecks. In the future if we do pure assertions we will take the opportunity to do the what ScalaCheck does with PropertyChecks.

from scalatest.

Hey Daniel,

I wanted to ask you a question about this issue. One background is that someone came along later on the ScalaCheck list and wanted the existing behavior:

https://groups.google.com/forum/#!topic/scalacheck/UrDs_O863-g

We are planning on adding an exists method that does the same as minSuccessful(1). I realized that could be used to make my above example with minSuccessful(1) more concise:

forAll[Int] { i =>
  exists(Gen.choose(i, i +10)) { j =>
    // something with i and j
  }
}

Can you elaborate on your concern about how the distribution might be affected?

from scalatest.