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

Fail at generating Command sequences less often #633

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonaskoelker
Copy link
Contributor

See https://gist.github.com/jonaskoelker/ac60ded3a310b8f0fb548a1eda9f8859 and https://gist.github.com/jonaskoelker/b4a1d00a3da97c6d56e7b3d8fee02716.

Without this change, the BoundedQueueSpec would fail at command generation unless you added .retryUntil(cmd => cmd.preCondition(state)) to the end of genCommand. With this change, command generation no longer fails.

I found one semi-related discussion at #568. However that issue gets resolved, I don't think that has any bearing on whether retrying in a Commands context is a good idea or not: while it's reasonable to expect listOfN to satisfy some sensible properties (e.g. listOfN(1, g) fails with the same probability as g, and I suppose listOfN(k, g) succeeds with probability p^k where p is the success probability of g), I would prefer the thing-similar-to-listOfN done in Commands to be more concerned about convenience in the form of a high success rate and dealing with the sequential data dependency due to state threading through a sequence of commands.

I can think of one argument against doing this: the user can always add .retryUntil(cmd => cmd.preCondition(state)) themselves, but why not scrap that boilerplate for them? It's not obvious to me what the existing code does better.

I picked the number 100 somewhat arbitrarily: I guess (without having measured) that it's large enough to paper over most practically occurring generation failures and small enough to not be a performance nuisance if some generator always fails.

@ashawley
Copy link
Contributor

ashawley commented Mar 2, 2020

I haven't looked at your code, but are you using ScalaCheck 1.14.3? There was a defect with command shrinking with Scala 2.12, see #447.

@jonaskoelker
Copy link
Contributor Author

jonaskoelker commented Mar 3, 2020

I haven't looked at your code, but are you using ScalaCheck 1.14.3?

I created jonaskoelker/scalacheck by branching off from the master branch of typelevel/scalacheck. My build.sbt says lazy val versionNumber = "1.15.0". My Commands.scala says private implicit val shrinkActions: Shrink[Actions] = Shrink[Actions] { as => ... }. (compare with the commit which fixed #447).

I'm pretty sure I'm not using ScalaCheck 1.14.3.

There was a defect with command shrinking with Scala 2.12, see #447

Yes, I saw that. That pull request was about command shrinking, this pull request is about Command generation. While related, the two are distinct. This PR addresses a situation where you have nothing to shrink because you never generate any command sequence to begin with.

Unless I've been running in a weird setup, if you run my BoundedQueue example, you should see this command generation failure as well.

@ashawley
Copy link
Contributor

I ran your code example, but it didn't compile. Is this something that depends on #632?

override def shrinkCommand: Shrink[Command] = Shrink { ...

Base automatically changed from master to main March 26, 2021 18:56
@jonaskoelker
Copy link
Contributor Author

Is this something that depends on #632?

Yes. When I remove the override modifier, I get a test success when I run on this PR and a test failure if I run against the parent commit. I get the same behavior when I remove the two shrinkers.

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

Successfully merging this pull request may close these issues.

2 participants