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

Feat/integrated shrinking #116

Merged
merged 89 commits into from
Jun 4, 2021
Merged

Conversation

sir4ur0n
Copy link
Contributor

@sir4ur0n sir4ur0n commented May 19, 2021

This is a reopening with additional fixes to original PR #109 adding QCheck2 and Integrated shrinking.

Julien Debon and others added 30 commits April 9, 2021 22:24
* Add shrinking documentation
* Go back to the previous implementation of Gen.int_range and adapt
* Fix wrong origins on some char generators
* Reorder and improve consistency of functions
* Add documentation sections
* Change the default origin from center to lower for various generators
* Add Tree.t, getters and pp to API
* Add an optional depth to pp
* change generate_print to generate_tree
@sir4ur0n
Copy link
Contributor Author

I think I have managed to make function shrinking work again 🎉 At least the first tests are positive, see:

let test_basic : QCheck2.Test.t =
  let open QCheck2 in
  Test.make (fun1 Observable.int bool) (fun (Fun (_, f)) -> f 42 = f 17)
test_function alias test/runtest (exit 1)
(cd _build/default/test && ./test_function.exe)
random seed: 52241607
randomgenerated error fail pass / total     time test name
[✗]    1    0    1    0 /  100     0.0s anon_test_1

--- Failure --------------------------------------------------------------------

Test anon_test_1 failed (2 shrink steps):

{17 -> true; _ -> false}
================================================================================
failure (1 tests failed, 0 tests errored, ran 1 tests)

I am going to test it further today, add some documentation on all its APIs, then we should be good to go!

@sir4ur0n
Copy link
Contributor Author

@c-cube IMHO this PR is done.

In the last days I have implemented a first version of function shrinking. It's not perfect but it works (ish).
I also documented all function-related types, modules and functions, e.g. Fn or Tuple.

Please do another (hopefully last 😁 ) review

@sir4ur0n
Copy link
Contributor Author

sir4ur0n commented Jun 1, 2021

I am currently migrating all existing Tezos tests to QCheck2 as a dogfooding experiment.

Current status:

  • I found some missing bits in the migration guide, added it
  • I noticed there was no getter for collect and stats, added it
  • I notice a big performance regression on Gen.string, currently investigating

I think the bulk of the PR can still be reviewed (but this should not be merged until such obvious regressions are fixed 😁 )

@sir4ur0n
Copy link
Contributor Author

sir4ur0n commented Jun 3, 2021

I just pushed:

  • 6720de8 significantly improves performance of the char generator, which in turn improves perf of string generators
  • e44b8ca puts more laziness everywhere related to shrinking. Some measurements tend to show a ~40% perf improvement on tests (that don't fail/shrink, which is the majority of time)

I'd be happy to get a review on these recent code changes as I am not experienced in OCaml performance 🙏

I'm resuming migrating my project to QCheck2 to see if I detect other problems

@sir4ur0n
Copy link
Contributor Author

sir4ur0n commented Jun 3, 2021

Tezos QCheck tests all pass with QCheck2.

I note that the total run time is slower with QCheck2 (~60s with QCheck2, ~50s with QCheck1).
I don't find this outraging considering the added feature of auto shrinking, but if anyone wants to take a look to see if more performance can be squeezed, please do so 😁

Copy link
Owner

@c-cube c-cube left a comment

Choose a reason for hiding this comment

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

Very nice. I think it's almost there, I just added a bunch of suggestions, but I think this can be merged real soon :)

src/core/QCheck.mli Show resolved Hide resolved
src/core/QCheck2.ml Outdated Show resolved Hide resolved
src/core/QCheck2.ml Show resolved Hide resolved
src/core/QCheck2.ml Outdated Show resolved Hide resolved
src/core/QCheck2.ml Show resolved Hide resolved
src/core/QCheck2.ml Show resolved Hide resolved
src/core/QCheck2.mli Outdated Show resolved Hide resolved
src/core/QCheck2.mli Show resolved Hide resolved
src/core/QCheck2.mli Show resolved Hide resolved
src/core/QCheck2.mli Outdated Show resolved Hide resolved
@sir4ur0n
Copy link
Contributor Author

sir4ur0n commented Jun 3, 2021

@c-cube I believe I have managed all threads, please review again 🙏

@c-cube
Copy link
Owner

c-cube commented Jun 4, 2021 via email

@c-cube c-cube merged commit cb1df04 into c-cube:master Jun 4, 2021
@c-cube
Copy link
Owner

c-cube commented Jun 4, 2021

Alright team, this time I think it's merged for good! 🎉

Thank you very much @sir4ur0n for your hard work and patience. This was quite the epic PR!

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