Add proptest support

[Hoverbear]

Adds proptest support (feature gated behind property-testing) for integration tests or user use.

I made this a feature so that any users using proptest could easily call any::<Key> for example.

I'll be amending #66 to base off this PR since it's a much nicer way.

[nrc]

What would this look like using proptest without proptest-derive? Could we separate the proptest stuff from the rest of kv.rs? I'm a bit worried about how noisy the proptest-derive attributes make that module, and if we're supporting more in the future it might make things quite hard to read.

[nrc]

I would use await rather than block_on since the latter does not permit concurrency

[Hoverbear]

I would too!

But, the proptest macro isn't compatible with async/await, I tried using a bare approach with async test functions but since the TestRunner operates in a closure (that doesn't return a result or an async) you can't use .await or ? anyways.

I'm considering trying to patch this upstream, but in the meantime, this seems easiest as it avoids a fair bit of testrunner/handling boilerplate.

[nrc]

I was thinking you would just use another function and call that from the test function, but if you think that is not worth the boilerplate, then we can keep things like this

[nrc]

From #69, I'm now thinking whether we need the prop testing features at all - what is the advantage over just having them always on (if integration testing is on)?

[Hoverbear]

@nrc I can imagine a very valid future case where we will want property testing on for use in non-integration tests, and it is handy to support it for downstream users.

[nrc]

I can imagine a very valid future case where we will want property testing on for use in non-integration tests, and it is handy to support it for downstream users.

Agree. I mean why not have it always on?

[Hoverbear]

@nrc Because some users won't want to build proptest during release etc. (Compile times, complexity). This lets users opt into the dependency.

[nrc]

Because some users won't want to build proptest during release etc. (Compile times, complexity). This lets users opt into the dependency.

Can we make proptest a dev dependency for that reason?

[Hoverbear]

It is, but I think that it's possible users might want this, and that letting it be opt-in is not very complex.

[nrc]

Hmm, I think it is worth stepping back a bit since I think we have different goals here :-)

My mental model is that we have two audiences - those using the client as a dependency and those developing the client. The former don't want to run any tests and want compilation to be as fast as possible. They want features to opt-in to some functionality or to support platform-specific functionality, e.g., encryption or async/await support.

Developers also want fast compilation, but want to run tests. IMO, the best developer experience is when cargo test --all runs all the tests. If this is not the case then tests will often pass locally and fail on CI which is frustrating and a poor use of CI resources. Exceptions to this are where tests are platform-specific (e.g., benchmarks), take a very long time, or require some setup beyond what Cargo can do automatically (e.g., the integration tests which require a TiKV server).

So, in general, my motivation for wanting to avoid a feature here is that I want to avoid the complexity of having features (having to think about them when building/testing, working out how they interact with each other, ensuring the right defaults, etc) and I want local tests to be similar to CI tests.

I'm trying to understand the motivation for including the feature - is it because the property tests take a long time? Or are not widely useful? Or that the dependencies take a long time to build? IOW, what is the benefit to the developer that outweighs the costs of added complexity and not running tests?

[Hoverbear]

@nrc Consider three use cases:

1. A developer who wants to use this as a dependency, and doesn't want proptest support
   • They don't want to pay the compile time cost for proptest, so they just use default features and don't get it.
2. A developer who wants to use this as a dependency, and wants to use our proptest support in their own tests.
   • They want to opt-in to proptest after they adopt the library themselves, they can enable the feature flag and have it enabled in their own dev builds.
3. A developer who wants to test this.
   • They explicitly want proptest, and it's there by default (since it's a dev dep)

As you can see, we're enabling proptest always on dev builds, but only on opt-in for folks depending on us. (at least as far as I'm able to tell from looking at the dependencies)

Is my understanding of how features and dev/normal dependencies incorrect?

[Hoverbear]

@nrc The merge above is non-trivial, but there were conflicts with master and reorganizing in the merge ended up cleaner.

The merge moved all the proptests into /src/proptests and flagged them behind cfg(test). This means proptest is on by default for dev. We can enable proptest as a feature for public later if wanted, as this structure allows it. (I'd rather get this merged and do it later)

Note we have to have things inside /src/ since /test integration tests won't have access to cfg(test) functionality.

Overall I think this solution is much cleaner and simpler. Thanks for your feedback :)

[nrc]

LGTM