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

Factor shrink loggers #172

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Factor shrink loggers #172

wants to merge 6 commits into from

Conversation

jmid
Copy link
Collaborator

@jmid jmid commented Sep 7, 2021

This PR pulls out the shrink logging from the expect tests as it was cluttering the .expected output files needlessly and conflating

  • checking basic functionality and
  • highlighting the underlying shrinking algorithms (and their potential limitations).

There is now a separate directory shrink_algo_logs with 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 how QCheck.char doesn't shrink (issue QCheck.char doesn't shrink (but QCheck2.Gen.char does) #166)
  • fun_{first,last}_foldleftright_qcheck{,2}.expected shows 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)
  • the 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 counterexample
    and 2 limitations of QCheck design (issue List shrinker performance #64):
    • (1) it wastes many needless shrink steps to restart and try smaller sizes repeatedly
    • (2) an aggressive int shrinker (trying 0 first like QCheck2) could cut down a number of steps (and restarts!)
  • 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

  • renamed test.ml to a more descriptive name
  • simplified the dune logic
  • removed shrink logging from existing tests and expect-outputs

@jmid
Copy link
Collaborator Author

jmid commented Sep 7, 2021

(also paging @sir4ur0n which I couldn't add as a reviewer)

@sir4ur0n
Copy link
Contributor

sir4ur0n commented Sep 7, 2021

I am in vacation but if you're in no hurry I can review when I'm back 😄

Copy link
Collaborator

@Gbury Gbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, ^^

test/core/shrink_algo_logs/char_never_abcdef_qcheck.ml Outdated Show resolved Hide resolved
test/core/shrink_algo_logs/char_never_abcdef_qcheck2.ml Outdated Show resolved Hide resolved
holds 0
holds 1
holds 2
fails 4611686018427387903
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit of a problem that this doesn't not shrink, or am I missing something ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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.

3 participants