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 bytes generator, printer and shrinker #245

Merged
merged 27 commits into from
Nov 4, 2022
Merged

Conversation

n-osborne
Copy link
Contributor

@n-osborne n-osborne commented Jun 8, 2022

Add the missing primitives for bytes

Close #83

@vch9
Copy link
Contributor

vch9 commented Jun 8, 2022

Cool! Haven't look very deep in the code yet but it could be nice to both add the generators to QCheck2 and append the changelog :).

src/core/QCheck.mli Outdated Show resolved Hide resolved
src/core/QCheck.mli Outdated Show resolved Hide resolved
src/core/QCheck.ml Outdated Show resolved Hide resolved
@jmid
Copy link
Collaborator

jmid commented Jun 9, 2022

This is great indeed, thanks @n-osborne! 😃
I second @vch9's suggestion of also adding the bytes combinators to QCheck2 to keep the two versions aligned.

I would also appreciate it if you would include tests of them in test/core/QCheck_tests.ml and test/core/QCheck2_tests.ml.
You can run these with make tests.
There are already string tests in Generator, Shrink, and Statistics which could be mirrored for bytes.

@n-osborne n-osborne force-pushed the bytes branch 2 times, most recently from 359ee6b to 7a4e7e9 Compare June 9, 2022 12:11
src/core/QCheck2.mli Outdated Show resolved Hide resolved
src/core/QCheck2.mli Outdated Show resolved Hide resolved
src/core/QCheck2.mli Outdated Show resolved Hide resolved
test/core/QCheck2_tests.ml Outdated Show resolved Hide resolved
@vch9
Copy link
Contributor

vch9 commented Jun 10, 2022

Looks good!

CHANGELOG.md Outdated Show resolved Hide resolved
src/core/QCheck.ml Outdated Show resolved Hide resolved
src/core/QCheck.ml Outdated Show resolved Hide resolved
src/core/QCheck.mli Outdated Show resolved Hide resolved
src/core/QCheck.ml Outdated Show resolved Hide resolved
src/core/QCheck2.ml Outdated Show resolved Hide resolved
src/core/QCheck2.mli Outdated Show resolved Hide resolved
src/core/QCheck.ml Outdated Show resolved Hide resolved
src/core/QCheck.ml Outdated Show resolved Hide resolved
src/core/QCheck.ml Outdated Show resolved Hide resolved
src/core/QCheck2.ml Outdated Show resolved Hide resolved
src/core/QCheck.mli Outdated Show resolved Hide resolved
@jmid
Copy link
Collaborator

jmid commented Jun 22, 2022

Thanks @n-osborne! 🙏 I'm sorry that this drifted into a design PR as this takes extra time to get right.

To summarize, with this PR we arrive at the below bytes and string interfaces:

QCheck.Gen:

  val bytes_size : ?gen:char t -> int t -> bytes t
  val bytes : ?gen:char t -> bytes t
  val bytes_of : char t -> bytes t
  val bytes_printable : bytes t
  val bytes_small : ?gen:char t -> bytes t
  val string_size : ?gen:char t -> int t -> string t
  val string : ?gen:char t -> string t
  val string_of : char t -> string t
  val string_readable : string t  (* deprecated *)
  val string_printable : string t
  val small_string : ?gen:char t -> string t

Here, we could use this opportunity to offer consistent, non-optional small combinators for both bytes and string:

  val bytes_small : bytes t
  val bytes_small_of : char t -> bytes t
  val string_small : string t
  val string_small_of : char t -> string t

(the last two don't necessarily need to be part of this PR)

QCheck:

  val bytes_gen_of_size : int Gen.t -> char Gen.t -> bytes arbitrary
  val bytes_gen : char Gen.t -> bytes arbitrary
  val bytes : bytes arbitrary
  val bytes_small : bytes arbitrary
  val bytes_of_size : int Gen.t -> bytes arbitrary
  val bytes_printable : bytes arbitrary
  val string_gen_of_size : int Gen.t -> char Gen.t -> string arbitrary
  val string_gen : char Gen.t -> string arbitrary
  val string : string arbitrary
  val small_string : string arbitrary
  val string_of_size : int Gen.t -> string arbitrary
  val printable_string : string arbitrary
  val printable_string_of_size : int Gen.t -> string arbitrary
  val small_printable_string : string arbitrary
  val numeral_string : string arbitrary
  val numeral_string_of_size : int Gen.t -> string arbitrary

Here we may want to add string_small, string_printable, ... in a move toward offering type-prefixed names - again not necessarily in this PR.

QCheck2.Gen:

  val bytes_size : ?gen:char t -> int t -> bytes t
  val bytes : bytes t
  val bytes_of : char t -> bytes t
  val bytes_printable : bytes t
  val bytes_small : gen:char t -> bytes t
  val string_size : ?gen:char t -> int t -> string t
  val string : string t
  val string_of : char t -> string t
  val string_printable : string t
  val string_small : gen:char t -> string t
  val small_string : gen:char t -> string t

Looking at it now, I think labeling the parameter to small_string is the right backwards-compatible fix.
However, we might consider aligning the 1-argument bytes* and string* combinators as non-labeled akin to QCheck.Gen above:

  val bytes_small : bytes t
  val bytes_small_of : char t -> bytes t
  val string_small : string t
  val string_small_of : char t -> string t

I would appreciate your eyes and input on these interfaces @vch9 and @c-cube

Copy link
Collaborator

@jmid jmid left a comment

Choose a reason for hiding this comment

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

Thanks! I've now taken another pass over this.

I spotted that some of these new string arbitrary generator aliases are not exposed in the interface. Ideally this would have been caught by just writing a simple test - but on the other hand, we shouldn't have to write duplicate tests for both aliases... 🤷

(I also spotted that some of the shrinkers for the above are broken but that is not caused by this PR, so I'll file a separate issue).

CHANGELOG.md Outdated Show resolved Hide resolved
src/core/QCheck.ml Show resolved Hide resolved
Indicate what the new name is a synonym of, so that
newcommers can easily make their choice.
This is the same fix that has been made in c-cube#258 for strings.
`Shrink.bytes` now takes an optional char shrinker (default value being
`Shrink.char`).
So `bytes_printable` can shrink only on printable chars.
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/core/QCheck2.ml Outdated Show resolved Hide resolved
@jmid
Copy link
Collaborator

jmid commented Oct 28, 2022

Thanks for the printable bytes shrinker fix!

Taking another pass of it I noticed that the CHANGELOG entries are off - and there is also an unintentional reversion of #249 resulting from all the rebasing.

Copy link
Collaborator

@jmid jmid left a comment

Choose a reason for hiding this comment

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

Thanks.
The changelog is still not quite right. I've added a proposal.

CHANGELOG.md Outdated Show resolved Hide resolved
@jmid
Copy link
Collaborator

jmid commented Nov 2, 2022

I've committed a few more changes I proposed in #245 (comment)

For reference, below follows a summary of the resulting bytes and string combinator interfaces.
Ignoring the (* to-be-deprecated: *) sections, I believe this represents a good step towards more consistent bytes and string combinator names.

QCheck.Gen

val bytes_size : ?gen:char t -> int t -> bytes t
val bytes : ?gen:char t -> bytes t
val bytes_of : char t -> bytes t
val bytes_small : bytes t
val bytes_small_of : char t -> bytes t
val bytes_printable : bytes t

val string_size : ?gen:char t -> int t -> string t
val string : ?gen:char t -> string t
val string_of : char t -> string t
val string_small : string t
val string_small_of : char t -> string t
val string_printable : string t

(* deprecated or to-be-deprecated: *)
val string_readable : string t
val small_string : ?gen:char t -> string t

QCheck arbitrary

val bytes_gen_of_size : int Gen.t -> char Gen.t -> bytes arbitrary
val bytes : bytes arbitrary
val bytes_of : char Gen.t -> bytes arbitrary
val bytes_small : bytes arbitrary
val bytes_small_of : char Gen.t -> bytes arbitrary
val bytes_of_size : int Gen.t -> bytes arbitrary
val bytes_printable : bytes arbitrary

val string_gen_of_size : int Gen.t -> char Gen.t -> string arbitrary
val string : string arbitrary
val string_of : char Gen.t -> string arbitrary
val string_small : string arbitrary
val string_small_of : char Gen.t -> string arbitrary
val string_of_size : int Gen.t -> string arbitrary
val string_printable : string arbitrary
val string_printable_of_size : int Gen.t -> string arbitrary
val string_small_printable : string arbitrary
val string_numeral : string arbitrary
val string_numeral_of_size : int Gen.t -> string arbitrary

(* to-be-deprecated *)
val string_gen : char Gen.t -> string arbitrary
val small_string : string arbitrary
val printable_string : string arbitrary
val printable_string_of_size : int Gen.t -> string arbitrary
val small_printable_string : string arbitrary
val numeral_string : string arbitrary
val numeral_string_of_size : int Gen.t -> string arbitrary

QCheck2.Gen

val bytes_size : ?gen:char t -> int t -> bytes t
val bytes : bytes t
val bytes_of : char t -> bytes t
val bytes_small : bytes t
val bytes_small_of : char t -> bytes t
val bytes_printable : bytes t

val string_size : ?gen:char t -> int t -> string t
val string : string t
val string_of : char t -> string t
val string_small : string t
val string_small_of : char t -> string t
val string_printable : string t

(* to-be-deprecated *)
val small_string : gen:char t -> string t

@jmid
Copy link
Collaborator

jmid commented Nov 4, 2022

@n-osborne can I ask you to take a quick look at the last commits - just one at a time - to see if they make sense - before we merge this PR:

  • consistent QCheck.Gen.{bytes,string}_small{,_of} combinators - abf86cd
  • consistent QCheck.{bytes,string}_of combinators - fb1ea0e
  • add consistent QCheck2.Gen.small_{bytes,string}{,_of} combinators - d1cb85e
  • fix comment for QCheck2.Gen.bytes - c587c88
  • update CHANGELOG - 41ceeae
  • add QCheck.{bytes,string}_small_of for consistency - 37d8d8a
  • update CHANGELOG - a14a767

@n-osborne
Copy link
Contributor Author

Looks good to me. The *_of name is indeed nicer than *_gen one.

@jmid jmid merged commit 6f7b5eb into c-cube:master Nov 4, 2022
@jmid
Copy link
Collaborator

jmid commented Nov 4, 2022

Hereby merged - thanks! 😃

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.

Add missing 'bytes' generators/shrinkers/printers/...
3 participants