refactor: convert quickcheck -> proptest

[huitseeker]

This is meant to start the discussion on a conversion from quickcheck to proptest.

A few notes:

1. I deleted the frequency function, and replaced it with prop_oneof. It may seem like the weighing of each alternative disappeared, but that is not the case: the prop_oneof! macro supports weights, it just has a more simple form in case all those weighs are identical.
2. I'm under-using the derivation of Arbitrary through proptest-derive. This is voluntary at this stage, for two reasons:
   • backwards-compatibility: the explicit composition of instances of Arbitrary for large enums may have missed certain variants (e.g. the strategy for Tag does not generate Char, Comm, U64 or Key), whereas a derived instance would not miss anything. That could create test failures I didn't want to deal with as part of this PR (I'll get to it eventually of course).
   • deriving Arbitrary requires having access to the instance definitions of all the (transitively) composing fields. This essentially requires moving the Arbitrary impls from test modules to the main code (which can also help usability of the library). I wanted to make sure no one feels strongly against this.

Note: this means that a ton of impl Arbitrary for Foo could disappear in a future PR, as soon as they are basically doing instance composition with no variability (e.g. exploring enums with empty variants like FooTag).

Perf impact is minimal:
https://gist.github.com/huitseeker/d8a203e2bc56ea6043cb67a0e8386312

[johnchandlerburnham]

@huitseeker I've pushed a branch here 49a19f3 integrating your changes with my #239.

If it's acceptable to you and @porcuquine, I would like to close this PR, and include the quickcheck->proptest change in #239 directly, since there's some work there that clarifies a few details (like the Arbitrary impl for FWrap, etc.)

Afaiu, all the quickcheck tests in our test suite were written by me for the purpose of checking ScalarStore serialization, so it makes sense to me to me to fold this into my sequence of digestible PRs refactoring that (of which #237 and #239 are the first).

Some specific thoughts:

I deleted the frequency function, and replaced it with prop_oneof.

The purpose of the weights in frequency was really for controlling generation of tree-like data, and it seems like prop_recursive just unambiguously does this better. https://altsysrq.github.io/proptest-book/proptest/tutorial/recursive.html

-backwards compatibility ... That could create test failures.

I don't think there should be any test failures from using Arbitrary derive. I added the missing cases in my branch, so should be easy to replace with the derives.

deriving Arbitrary requires having access to the instance definitions of all the (transitively) composing fields.

So, I have a solution to this issue I used in my #191 , where you can gate the Arbitrary impls behind a test-utils feature:
https://github.com/lurk-lang/lurk-rs/blob/186cedfbe159352afffef8aed7216e1a3c999a6c/lurk_ff/Cargo.toml#L19

Then you can enable test-utils for testing-only. The key thing though is that crates which import you can also enable test-utils during testing-only. And example of this is how ldon imports lurk_ff in #191

https://github.com/lurk-lang/lurk-rs/blob/186cedfbe159352afffef8aed7216e1a3c999a6c/ldon/Cargo.toml#L32

Ofc if we want to just put the arbitrary instances in the main this doesn't matter, and might be too complicated. But it gives us a third option

[johnchandlerburnham]

Actually, made a PR for #240 so that it's easier for people to look at

[huitseeker]

I have a solution to this issue I used in my #191 , where you can gate the Arbitrary impls behind a test-utils feature:

Right, so there is an issue with the activation of test-only code with features, which is that features are additive. This interaction is with multi-crate environments:

• assume you have a crate foo depending on bar with test-utils active,
• assume that bar depends on bar with test-utils not active,
• further assume that bar depends on foo.

Now bar effectively depends on bar with test-utils activated 🤦

I wrote about it in more details here:
https://gist.github.com/huitseeker/6f117d0157783b571c280c8462f4ab72

TL;DR: this solution quickly becomes indistinguishable from making the Arbitrary instances public (not hidden behind a feature or anything), which I think is not a bad thing anyway.

[johnchandlerburnham]

Right, so there is an issue with the activation of test-only code with features, which is that features are additive.

From your gist, I think possibly we could do something like:

#[cfg(any(all(test, not(feature = "test-utils")), all(not(test), feature = "test-utils")))]
compile_error!("feature \"test-utils\" can only be enabled within test")

to make sure "test-utils" can't be turned on outside of test, and must be turned on inside test.

We could also do the latter half of this constraint at test-time: https://github.com/lurk-lang/lurk-rs/blob/186cedfbe159352afffef8aed7216e1a3c999a6c/ldon/src/lib.rs#L21-L36

Essentially, this is less of a general-purpose feature, and more of a work-around for the Rust limitation that you can't import another crate's test modules.

All that said, it's a lot of complexity versus just adding e.g. Arbitrary publicly.

[huitseeker]

From your gist, I think possibly we could do something like:

Except cfg(test) is not transitive: when foo (whether itself in cfg(test) or not) depends on bar, bar is not put in cfg(test) itself.

So if you defined the code with your cfg(not(test)) XOR cfg(feature = “test-utils”) annotation in, say, “baz”, then there is no way to depend on baz with the test-utils feature.

In other words, the only condition in which it’s both workable and meaningful to hide an Arbitrary behind a cfg(test) flag is when you only expect to use those within the same crate.

If you intend your hidden Arbitrary Impls of crate baz to be used across a crate boundary, you have to use a default feature “production”, hide the Arbitrary behind cfg(not(feature = “production”)), and expect that consumers of this hidden impl will have to define dedicated test crates because they and their dependencies can only ever import your crate with no-default-features. This works, but it is not a very happy place filled with rainbows and unicorns.

All that said, it's a lot of complexity versus just adding e.g. Arbitrary publicly.

Agreed.

[huitseeker]

Closing in favour of #240