Open
Conversation
Collaborator
Author
|
(also paging @sir4ur0n which I couldn't add as a reviewer) |
Contributor
|
I am in vacation but if you're in no hurry I can review when I'm back 😄 |
Gbury
reviewed
Sep 9, 2021
Collaborator
Gbury
left a comment
There was a problem hiding this comment.
The split is nice ! The diff is quite long so I didn't read everything, but considering it's only some more tests, it can't be wrong, ^^
| holds 0 | ||
| holds 1 | ||
| holds 2 | ||
| fails 4611686018427387903 |
Collaborator
There was a problem hiding this comment.
it's a bit of a problem that this doesn't not shrink, or am I missing something ?
Collaborator
Author
There was a problem hiding this comment.
I characterized it as "debatable" myself when I added the tests originally 😉
#153 (comment)
incl. what I saw as arguments for and against.
There's also a level-headed reply from @sir4ur0n here:
#153 (comment)
Closed
This was referenced Apr 2, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR pulls out the shrink logging from the expect tests as it was cluttering the
.expectedoutput files needlessly and conflatingThere is now a separate directory
shrink_algo_logswith a bunch of expect tests.The idea is that these (in some cases terrifying) expect logs should gradually become nicer as each limitation is addressed 😀
One could say it turns a bunch of prior shrinking algorithm observations scattered around in comments into actual tests for everyone to see and improve on.
Highlights (many of which have been observed before):
char_char_never_abcdef*logs howQCheck.chardoesn't shrink (issueQCheck.chardoesn't shrink (butQCheck2.Gen.chardoes) #166)fun_{first,last}_foldleftright_qcheck{,2}.expectedshows how the QCheck function shrinker's "generate-function-last-for-best-result" comes at a price of many shrink steps (room for algorithmic improvement?) and that QCheck2's function shrinker also could use a helping hand (issue Sub-optimal QCheck2 function shrinkers #163)int_*tests show QCheck2 repeatedly tests for 0 (aggressive shrinking - see below)list_shorter*shows how QCheck2 will try new random lists rather than cut down the first counterexampleand 2 limitations of QCheck design (issue List shrinker performance #64):
string_empty*shows how QCheck doesn't shrink the string's characters (mentioned in issue Improve QCheck2 string shrinking #157)string_never_has_000*shows how QCheck2's string shrinker tries random (unrelated) strings to cut down the size, while QCheck's shrink algorithm uses a simple iterative algorithm rather than bisection or something list-shrinking-inspired (both mentioned in Improve QCheck2 string shrinking #157).Finally, it