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

Fix discard ratio and maxSuccess interacting poorly #371

Merged
merged 7 commits into from
Mar 21, 2024

Conversation

MaximilianAlgehed
Copy link
Collaborator

Fixes #338

small + fix `withMaxSuccess` not updating result on discard
@MaximilianAlgehed
Copy link
Collaborator Author

@Rewbert thoughts?

@Rewbert
Copy link
Collaborator

Rewbert commented Mar 20, 2024

This whole discard thing is such a thorn in my side. The choice to compute sizes in this way seems to have been arbitrarily chosen initially (Koen looked guilty when I asked him about this!), so in my mind this is "equally arbitrary".

I have thought a lot about this, and have even tried out a few different approaches altogether, but have not landed in anything that is obviously superior. I think the question of sizes might be a more complicated one than it appears initially.

Regarding the eventual future merge of the parallel branch, this will not impact me negatively.

src/Test/QuickCheck/Test.hs Outdated Show resolved Hide resolved
tests/DiscardRatio.hs Show resolved Hide resolved
MaximilianAlgehed and others added 3 commits March 20, 2024 23:01
regardless of if we are using `withMaxSuccess` and `withDiscardRatio` or
`stdArgs{maxSuccess=...,maxDiscardRatio=...}`
@MaximilianAlgehed
Copy link
Collaborator Author

For posterity, I had to factor out computeSize from State because the computeSize definition was set too early based on the Args arguments to quickCheckWith and thereby would ignore withMaxSuccess and withDiscardRatio.

@MaximilianAlgehed MaximilianAlgehed merged commit d88f5f6 into master Mar 21, 2024
108 checks passed
@MaximilianAlgehed MaximilianAlgehed deleted the PR-fix-discard-ratio-maxSuccess branch March 21, 2024 08:55
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.

quickCheckWithResult with low maxSuccess fails unexpectedly
3 participants