Aren't we adding to much log tests?

[vch9]

Ping @jmid as you're mostly producing these tests.

To be honest, I don't especially agree with how we are integrating tests right now.
I'd like to review some of the merge requests but, for instance, #176 has 103 files
changed and +385k lines of code. I know we can use the dune machinery to validate
these logs, but reading as an human is really difficult with these larges merge requests.

This is not a blame @jmid, you're doing an incredible effort. I understand
the idea of having regression tests and gaining insurances about shrinking.

[jmid]

First of all: I agree that the large logs create a ton of noise in PRs and that they make reviewing harder.

Please note - this is an incremental effort:

  jmid:factor-shrink-loggers #172 --- jmid:check_exn-tests #174 --- jmid:more-tests #176 --- jmid:shrinker-performance-benchmark #177
                    \
                      `----- jmid:qcheck-int-shrinker-fast-path #173

I'll be happy to squash/rebase/cherry-pick/reorder/... this stuff (or let @c-cube do so 😄).
For example, in #172 when I pulled out the logging I didn't use the same seed as in the expect-tests. This is later fixed in #176.

So why bother? I've come to realize that we have been developing QCheck blindfolded, following a principle of "seems-to-work"
due to our lack of tests and benchmarks (especially of the shrinkers) ... until we realize it doesn't 😬

Examples:

• my own attempts at fixing the list shrinker in List shrinker performance #64 continued the 'try-all-chunk-positions', not realizing, e.g.,
  • that trying 325.000 positions in a long list may not be a good idea Segfault on fac test with OCaml 4.12.0 #175 (comment)
  • that the change failed to shrink one-element lists Fix singleton list shrinking #79
• our QCheck int-shrinkers may actually output an Iter.t with duplicates under certain circumstances (see Test.check_exn and more unit tests #174)
• 1696 reported shrink steps in ~10 seconds may seem a bit high on lists shorter than 432 until you realize that it tried 5.118.102 times (over 5 million! - see Shrinker performance benchmark #177)
• I've ended up reimplementing a string shrinker in the past https://github.com/jmid/wasm-prop-tester/blob/05bd76f989896548ac21b681c7afec8fc10505e0/src/shrinker.ml#L402-L428 because the built-in string shrinker's performance was too bad - but when trying to fix it by mimicking the list shrinker's approach I then repeated the excessive chunk-position mistake 😬
• not having studied the shrinker output too much, I don't think we collectively understood why a splittable RNG (even a poor-man's one)
  is needed to make integrated shrinking work well
• ...

So how do shrink-loggers help?

• They document how a shrinker is an engineering trade-off: too many reduction attempts (large file) hurts performance but too few results in a non-minimal counterexample. They do not only count successful shrink steps (like our end-user UI) which may be misleading.
• We can compare both performance and the underlying operations of the QCheck and QCheck2 shrinkers
• Picking a large/puzzling log, one can get an idea of how/why a non-performing shrinker is spending its time
• New ideas for improvements can be tried out to see if they actually pay off
• Now it is documented in the repo how
  • the QCheck list and string shrinkers have performance problems
  • lack of a splittable RNG is hurting QCheck2 string shrinking (see test/core/shrink_algo_logs/string_never_has_000_char_qcheck2.expected)
• The CI will show how any change affects functionality:
  • list/string/... shrinker improvements will have a large diff (reduction) log - this can be reviewed - also counting LOC 😀
  • intended non-breaking changes can verified and caught if they happen to break anything
• We can have engineering discussions rooted in facts, e.g., of whether QCheck's int shrinker should be aggressive like QCheck2 and return 0 up front QCheck: improve int shrinker fast path #173

Shrink-loggers are not perfect:

• They contain code-duplication which is error prone. That sucks. Potentially this can be avoided with some meta-programming and dune-file magic, since the tests now reside in a reusable library (Warning: this will cause changes in at least 90 files... 😄)
• A large shrink counts is not necessarily bad - but spending too much time is. That motivated me to develop the benchmark in Shrinker performance benchmark #177 which can address some (but not all) of the above.

[vch9]

Maybe this should/might be added in the CI using logs located in another repository? We would still benefit from these logs but they won't pollute the current project.

[c-cube]

another possibility is to use an orphan branch (git checkout --orphan <name>) to store the logs, and use git worktree add <name> <name> to get a copy of it in a directory. I like to do that to edit gh-pages branches typically. It'd still be in the same repo but would leave master/main untouched.

[vch9]

Yep, that's more suitable than a separate repo.