Deriver: more tests and some fixes

[vch9]

I think #201 raised an issue: textual diff tests are not enough (thank you @bobot)

Therefore, I started writing tests using the derived generators, we can then ensure both:

• The deriver produce a well-typed generator (at least for the types tested)
• The deriver uses the same generators as in QCheck

Thank to these tests, I found little bugs in the code, fixed alongside the tests.

(Do not mind extra commits, it is for now rebased on #195, If someone review this before the merge of #195, you should
review commit by commit starting 7bf423b)

[jmid]

This looks good! 👍‍

I think it would be natural to fill out the two TODOs (tuples and records) with even just some very basic tests
before merging - otherwise we aren't getting many guarantees of the derived functions.
For tuples, something similar to your earlier t2...t9 tuple tests could work:

qcheck/test/core/QCheck_expect_test.ml

Lines 232 to 236 in 57c9ba7

	let test_tup3 =
	Test.make ~count:10
	~name:"forall x in (0, 1, 2): x = (0, 1, 2)"
	(tup3 (always 0) (always 1) (always 2))
	(fun x -> x = (0, 1, 2))

The weighted tests of variants and poly-variants seem to only test the "weight 0" case.
Here a test with a skewed distribution, e.g., weights 1 and 4, could check that we agree with a hand-written generator
(or alternatively: produce more of "higher weight" for a fixed seed).
I wonder whether two weighted generators w/weights [1;1;1] and [3;3;3] would produce the same output... 🤔

[vch9]

I pushed proper tests for both tuples and records.

The weighted tests of variants and poly-variants seem to only test the "weight 0" case.
Here a test with a skewed distribution, e.g., weights 1 and 4, could check that we agree with a hand-written generator
(or alternatively: produce more of "higher weight" for a fixed seed).
I wonder whether two weighted generators w/weights [1;1;1] and [3;3;3] would produce the same output... thinking

I do agree with you on these tests. However, my goal here is more to prove that [@weight _] is used in the derivation. Your idea seems more to be frequency tests itself, would you still believe it it'd be relevant here? Even thought, more tests is always better!

[jmid]

I pushed proper tests for both tuples and records.

👍‍

I do agree with you on these tests. However, my goal here is more to prove that [@weight _] is used in the derivation. Your idea seems more to be frequency tests itself, would you still believe it it'd be relevant here? Even thought, more tests is always better!

I see your point - I fear my last speculation ended up muddling the point, sorry. Let me try again:
AFAICS, the annotation [@weight _] could functionally mean "don't produce this variant" regardless of the weight given.
Since the current tests do not compare to the expected output syntax nor test against a hand-written frequency, taking a devil's advocate, blackbox perspective, an implementation could pass these without mapping to frequency(!) 🙃

[vch9]

I see your point - I fear my last speculation ended up muddling the point, sorry. Let me try again: AFAICS, the annotation [@weight _] could functionally mean "don't produce this variant" regardless of the weight given.

Yes, you are right.

Since the current tests do not compare to the expected output syntax nor test against a hand-written frequency, taking a devil's advocate, blackbox perspective, an implementation could pass these without mapping to frequency(!) upside_down_face

The syntax ouput is actually tested in test_weight_konstrs (test_textual.ml). But, you are right, it could also benefit from being tested against a hand-written frequency.

[jmid]

Since the current tests do not compare to the expected output syntax nor test against a hand-written frequency, taking a devil's advocate, blackbox perspective, an implementation could pass these without mapping to frequency(!) upside_down_face

The syntax ouput is actually tested in test_weight_konstrs (test_textual.ml).

Sorry I had forgotten about those tests again! 😬 You are right.

That "we already have a test for that"-moment must (almost) be a first time for QCheck! 😅

Good job @vch9!

[vch9]

Thank you for the review.Thought, we merged fixups commits, this is not dramatic and I'm not sure squashing on master is a good idea for the historic. I'm used to work on GitLab where fixups commits automatically adds a WIP flag which block potential merges.