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

Add command shrinker function #272

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

Conversation

nikolaushuber
Copy link
Contributor

This PR adds support for creating a function to automatically shrink the arguments of tested functions when creating counter-examples.

This fixes #269.

@jmid
Copy link
Contributor

jmid commented Nov 4, 2024

Wow Nikolaus, you are on fire! 🔥

Have you observed whether this change significantly affects the time one has to wait to get a response (like the increased response time caused long arrays)?

Unrelated: Will the output shrinker reduce list arguments by suitable lifting of an element shrinker, as in Shrink.(list int)?

@nikolaushuber
Copy link
Contributor Author

Have you observed whether this change significantly affects the time one has to wait to get a response (like the increased response time caused long arrays)?

Since all the tests run in milliseconds since ocaml-multicore/multicoretests#472 I didn't notice any change when running the tests, they still all run in under 1 second.

Unrelated: Will the output shrinker reduce list arguments by suitable lifting of an element shrinker, as in Shrink.(list int)?

It did, but incorrectly so. I just ran it on-top of #271, which uses of_list in the array example. I pushed a small fix, since the argument shrinker to Shrink.array and Shrink.list is a labelled argument.

@jmid
Copy link
Contributor

jmid commented Nov 4, 2024

Cool, nice catch 😃 👍

@n-osborne
Copy link
Collaborator

Thank you!
I'll try to take a look this week.

Copy link
Collaborator

@n-osborne n-osborne left a comment

Choose a reason for hiding this comment

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

Thanks! Nicely done!

I just have a remark on how shrink_cmd_cas is defined in 8c50ae7.

You filter list and array to be able to apply the shrinker with a labelled argument, according to the QCheck.Shrink signature.

Note that bytes and string also ask for an optional shrinker argument.

Now, if users want to override one of these shrinker, they have to be careful to follow the QCheck.Shrink.*** signature, which is reasonable, but should be documented.

Regarding shrinkers for types defined in the tested library, it should also be documented that users should go for unlabelled arguments.

One way to simplify the user's life would be to override on our side shrinkers with optional argument with shrinker with unlabelled arguments (i.e: array shrink = array ~shrink), as shrinker_of_ty is returning a shrinker, not an optional shrinker anyway. This should be done either by writing a module in the runtime that will be included in the generated code or directly in the generated code.

@nikolaushuber
Copy link
Contributor Author

Note that bytes and string also ask for an optional shrinker argument.

That's true! I was considering to set the argument shrinker for bytes and string as well, but they already have a standard shrinker (i.e. the standard Shrink.char). I thought that it's probably very rare that a user would like to overwrite that with something else (e.g., Shrink.char_printable or so).

Regarding shrinkers for types defined in the tested library, it should also be documented that users should go for unlabelled arguments.

Indeed, that should be documented. In the current implementation it is also more or less required to define a shrinker for any custom defined types, as we do not inspect the config module. I guess one could make the custom shrinker function is the config module an optional value, but that probably gets ugly quite quickly as well ....

Regarding the place for redefining the Shrink module, do you have a preference of either putting it in the runtime or in the generated code? IMO it's probably a bit cleaner in the runtime as it does not overcrowd the generated code.

@n-osborne
Copy link
Collaborator

Regarding the place for redefining the Shrink module, do you have a preference of either putting it in the runtime or in the generated code? IMO it's probably a bit cleaner in the runtime as it does not overcrowd the generated code.

I agree, the runtime is cleaner.

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.

[qcheck-STM] Emit a function to shrink cmd arguments
3 participants