Move testing to single package

FintanH commented 3 years ago

Motivation

As we separate out packages in the workspace, it becomes harder to re-use testing code as it's generally hidden behind cfg(test) and so it can't be re-used from librad in another package.

We would like to consolidate all testing into a single package in the workspace. This package would essentially be the home of all tests (unit + integration), it would also include any support code for testing, e.g. Strategy generators for proptest.

Plan

All library code will add the following line to their Cargo.toml files:

[lib]
test = false

Since librad-test already contains support code for testing, we will use it as the base for moving all the testing code, and rename it to radicle-link-test. The following conventions will be used for moving tests over.

Unit tests

When moving a set of unit tests over we will mimic the module path of the original path, but prefixed with the unit path. For example, if we are moving the tests in librad/src/git/fetch/specs.rs then the path for the tests will live in radicle-link-test/src/unit/librad/git/fetch/specs.rs.

The unit.rs will then declare all the sub-modules, and in turn, radicle-link/src/lib.rs will contain the following line:

mod unit;

Integration tests

When moving a set of integration tests over we place them under the integration path, i.e. radicle-link-test/src/integration/*. For example, if we are moving librad/tests/smoke/clone.rs then the new path will be radicle-link-test/src/integration/smoke/clone.rs.

The integration.rs will then declare all the sub-modules, and in turn, radicle-link/src/lib.rs will contain the following line:

mod integration;

Support code

The support code will mimic the path of where it lives in its respective library, prefixed by the library's name. For example, when moving the gen_public_key and gen_secret_key function from librad/src/keys.rs they will go into radicle-linke/src/librad/keys.rs. Any extra support code can also live in the module, for example, the introduction of fast seed SecretKey.

Pros

We will have one place for testing and support code can be re-used across the tests of several packages.
Compile times should improve for packages and the test package, as mentioned in this article.

Cons

Due to circular dependencies this change needs to be done in one-go, otherwise, the compiler gets confused about where the types come from

Open Questions

How do we evaluate if the API needs to be changed? For example, the Delegations trait is sealed and the generators introduce SomeDelegations which implements this trait. Should it be unsealed and made public? Are there other cases where we move private to public types/traits?

References

Delete Cargo Integration Tests

kim commented 3 years ago

I, for one, welcome the new plan!

Some minor questions / comments:

Does unit subsume property tests?
Do we need the #[cfg(test)] in this package?
I like the idea of putting integration tests in the library code (as opposed to the tests directory). It’s then actually possible to choose which tests to run by a simple prefix.
The unit test module path also needs the crate name, in case of conflicts, no?
If a generator cannot be written without sidestepping the public constructor(s), then perhaps it's a candidate for some fuzzing harness? Otherwise, I think it's fine to export such generators from the respective library. It'll still yield the desired performance improvement, and there is (should) not be anything intrinsically unsafe about generated values.
I don't think sealed traits are in the way in general. In the example, SomeDelegations could be a useful public type, and sealing the trait doesn't prevent construction of the value. Or am I missing something?

FintanH commented 3 years ago

Does unit subsume property tests?

I suppose they don't necessarily need to. We could have properties of a unit and we could have properties of integrated units -- I don't think we have any existing ones for integrated units though.

Do we need the #[cfg(test)] in this package?

Isn't that necessary for tests to actually execute? That's the impression I was under but we could try confirm it.

The unit test module path also needs the crate name, in case of conflicts, no?

Great point :+1: I'll amend.

If a generator cannot be written without sidestepping the public constructor(s), then perhaps it's a candidate for some fuzzing harness?

Would you mind shedding some more light on this. I'm not familiar with what a fuzzing harness looks like :)

I don't think sealed traits are in the way in general. In the example, SomeDelegations could be a useful public type, and sealing the trait doesn't prevent construction of the value. Or am I missing something?

So in this scenario, SomeDelegations would be defined in radicle-link-test, and so it won't be able to derive Sealed and so it won't be able to derive Delegations. Are you saying that SomeDelegations would live in librad and the type + generators would be exported from there?

Otherwise, I think it's fine to export such generators from the respective library. It'll still yield the desired performance improvement, and there is (should) not be anything intrinsically unsafe about generated values.

And on that point, we should conventionalise where this would live. My suggestion would be a sub-module gen.rs that would contain the support code and generators, e.g. SomeDelegations above.

kim commented 3 years ago

We could have properties of a unit and we could have properties of integrated units

What I meant was more related to the naming convention: do we distinguish "unit" tests being example-based and "property" tests being generator-based? Should they live in separate directories/files?

Do we need the #[cfg(test)] in this package?

Isn't that necessary for tests to actually execute?

No actually not: it's just conditionally compiling the annotated code when the test target is used. You can annotate any function with #[test] and the test harness (when you run cargo test) will find it.

Would you mind shedding some more light on this. I'm not familiar with what a fuzzing harness looks like :)

Well I think the distinction is, ehm, fuzzy :) In property testing, we typically generate values within the permissible range of a given type, and then see if all functions which take this type as an input handle this correctly. If not, we usually go and refine the type. Fuzzing often goes one step further, and generates garbage data, feeds that as inputs to the program surface, and sees if the program crashes. That can take a long time, so it is possible to constrain the generators in ways similar to property testing generators, but I think one would do this in a way which still yields invalid data, just to speed up the process (eg. sidestep a parsing stage).

Looking at cargo-fuzz, they use an Arbitrary trait, which can be hidden behind a feature. Since fuzzing takes a long time, we'd certainly have a separate harness for that, which would enable the feature. I think that, if the dependencies all go in only one direction, features may actually work as expected.

Are you saying that SomeDelegations would live in librad and the type + generators would be exported from there?

I'm saying that SomeDelegations could be defined in librad and exported, but I don't see a reason the generators need to be defined there.

My suggestion would be a sub-module gen.rs

+1

FintanH commented 3 years ago

What I meant was more related to the naming convention: do we distinguish "unit" tests being example-based and "property" tests being generator-based? Should they live in separate directories/files?

Ah, hmmmm. So here's an idea. I generally like to write my property tests by writing the functions first and then a thin wrapper in proptest!, i.e.:

fn commutes(f, a, b) {
  f(a, b) == f(b, a)
}

proptest! {
  #[test]
  fn plus_commutes(a in as, b in bs) {
    commutes((+), a, b)
  }
}

That way commutes can be used as a unit test as well, for example, when your proptest fails and you have a small counterexample to work with.

So with that in mind, we could have: radicle-link-test/src/properties/librad/net/membership/patial_view.rs contains the property functions, made pub, and the proptest! themselves. Then they can MAY be reused in the unit tests.

You can annotate any function with #[test] and the test harness (when you run cargo test) will find it.

Magic :mage: Will ammend :)

Well I think the distinction is, ehm, fuzzy :) [..]

There's also propfuzz which builds on top of proptest.

However, since there's a lot going on here already maybe it'd be good to revisit the fuzziness after the move :)

I'm saying that SomeDelegations could be defined in librad and exported, but I don't see a reason the generators need to be defined there.

Makes sense :ok_hand:

FintanH commented 3 years ago

No actually not: it's just conditionally compiling the annotated code when the test target is used. You can annotate any function with #[test] and the test harness (when you run cargo test) will find it.

So, I did a quick run on this. Turns out you're right, BUT, all the imports think they're not being used because the #[test] code is behind cfg(test). So I suppose the question is, "What granularity do we want to have cfg(test) annotations?" We could put them on unit and integration to catch all, or put them on certain submodules. I'm not sure which is best yet but I'll keep exploring.

kim commented 3 years ago

https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2

FintanH commented 3 years ago

https://doc.rust-lang.org/cargo/reference/resolver.html#feature-resolver-version-2

I'm not understanding the relevance :flushed:

kim commented 3 years ago

kim commented 11 hours ago FintanH commented 11 minutes ago

Did you do this on purpose? :D

The relevance is that this appears to fix the issue that cargo always enables the largest set of features within a workspace, regardless of target.

FintanH commented 3 years ago

Did you do this on purpose? :D

It's a coincidence that it takes 11 minutes for me to call defeat on understanding :joy:

The relevance is that this appears to fix the issue that cargo always enables the largest set of features within a workspace, regardless of target.

And that was your point against the PR that introduced the prop feature flag, right?

fwiw, I have a branch that has the changes outlined above, it's almost ready for a PR

radicle-dev / radicle-link