c-cube / qcheck

QuickCheck inspired property-based testing for OCaml.
https://c-cube.github.io/qcheck/
BSD 2-Clause "Simplified" License
340 stars 37 forks source link

add bytes generator, printer and shrinker #245

Closed n-osborne closed 1 year ago

n-osborne commented 2 years ago

Add the missing primitives for bytes

Close #83

vch9 commented 2 years ago

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 :).

jmid commented 2 years ago

This is great indeed, thanks @n-osborne! :smiley: 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.

vch9 commented 2 years ago

Looks good!

jmid commented 2 years ago

Thanks @n-osborne! :pray: 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

jmid commented 1 year ago

Thanks again for your patience on this issue. I think it is shaping up nicely!

Meanwhile the shrinker in QCheck.string_printable has been fixed and merged, meaning this PR now contains a known broken shrinker for the new addition QCheck_bytes_printable.

I think the right fix would be to extend the signature of

  Shrink.bytes : bytes Shrink.t

similarly to

  Shrink.string : ?shrink:(char Shrink.t) -> string Shrink.t
jmid commented 1 year ago

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.

jmid commented 1 year ago

I've committed a few more changes I proposed in https://github.com/c-cube/qcheck/pull/245#issuecomment-1162975158

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 commented 1 year ago

@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:

n-osborne commented 1 year ago

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

jmid commented 1 year ago

Hereby merged - thanks! :smiley: