Comprehensive review of Test.Parameters.

[non]

This commit does several things:

1. Ensures that overrideParameters applies after cmdline
2. Fixes initialSeed to work with Test.Parameters
3. Adds parameter to disable Shrink-based shrinking
4. Fixes numerous bugs around cmdline parsing
5. Adds tests to ensure Test.Parameters is working
6. Internal clean up and refactoring

This commit includes the work from #463 and subsumes it. I believe it also fixes #360, #289, and #221.

Given how broken parameters are I think this change is a strict improvement. Without making bigger breaking changes I think it will be hard to "rationalize" how parameters work in ScalaCheck, but I think this gets us much closer to having a story that makes sense.

Using the -disableLegacyShrinking argument to disable shrinking, this PR can partially mitigate #443, #432, #347, #338, #317, and #129.

[non]

I tagged @ashawley on the review here but I'd appreciate reviews from others, especially folks who have a good handle on the SBT testing hooks.

[charleso]

💯 for a disableLegacyShrinking option. That's a great idea, I wish I had thought of that.

That said, I'm just curious if it's intended to be the same/similar to my #444 PR? The only reason I ask is that it appears (but I'm not 100% sure) to disable shrinking for Arbitrary as well, which I would argue is actually working fine. I guess the question is - what do you mean by "legacy" shrinking? I'm happy to talk this offline/elsewhere, I don't want to bog this down with another shrinking discussion here if that's unhelpful.

[ashawley]

The only reason I ask is that it appears (but I'm not 100% sure) to disable shrinking for Arbitrary as well, which I would argue is actually working fine.

Yes, they would disable shrinking for properties run with Arbitrary. What Erik has added here is, as he said, "partial mitigation" for shrinking problems. They are a blunt instrument. He thought they would be helpful for users to include in a release, sooner rather than later.

These features for users to disable shrinking at various levels do not preclude additional improvements to shrinking, later. Those will be major changes to the library that we'll be able to entertain after this patch release.

[non]

@charleso Yes, this would disable all shrinking.

It's possible that we could differentiate Gen and Arbitrary here. But since Arbitrary is built from Gen, and we don't have reliable ways to shrink values generated by Gen instances, I'm not convinced that authors of Shrink instances have been careful to avoid these pitfalls. Turning off all shrinking ensures that users can be certain they won't shrink to invalid values. I thinks it's likely that leaving shrinking on for Arbitrary will mean some folks will still end up with unexpected, invalid values after shrinking (although I agree with you that the vast majority of bugs we see are related to Gen not Arbitrary).

[non]

@charleso The meta-point is that the future of "correct" shrinking in ScalaCheck is to use a totally different strategy. That's partially why I want to characterize the Shrink trait as "legacy shrinking" to get folks used to the idea that we might stop using it in favor of a more robust shrinking strategy.

[non]

@ashawley I'm assuming that you meant the parameter name changes are binary-compatible but are source-incompatible, right?

I'll go ahead and change those parameter names back -- I don't think there's a strong reason for me to have changed them. I'm going to think more about your other comment.

[ashawley]

Sorry for the confusion. I corrected the comment. I predict there aren't many cases where users leveraged the named arguments for these methods. You managed to be so hygienic about avoiding breaking changes here, I figured I'd raise it.

[non]

@ashawley I didn't do exactly what you suggested but I did clean up the naming and also added methods to disableLegacyShrinking and enableLegacyShrinking. What do you think?

[ashawley]

I did clean up the naming and also added methods to disableLegacyShrinking and enableLegacyShrinking. What do you think?

Yeah, this is better:

override def overrideParameters(prms: Test.Parameters) = {
  prms.disableLegacyShrinking
}

[charleso]

@non Thanks for the reply/clarification, I appreciate it.

I'm not convinced that authors of Shrink instances have been careful to avoid these pitfalls.

Oh for sure. That's not to mention anyone who might have locally overridden an Arbitrary implicit (instead of using forAll with Gen).

I guess I was taking a somewhat optimistic view that normal usage with the ScalaCheck provided Arbitrary/Shrink instances (ie Int, String, List), which should (in theory) line up. From my experience most people don't write their own Shrink instances anyway.

although I agree with you that the vast majority of bugs we see are related to Gen not Arbitrary

Cool. I guess I still wasn't 100% sure people didn't think I was crazy/wrong about that point. :)