ockam icon indicating copy to clipboard operation
ockam copied to clipboard

Move `ockam_vault_test_suite` to `ockam_core::vault::test_support`

Open thomcc opened this issue 4 years ago • 5 comments

Note: This must be done after #2288. Alternatively, it can be done at the same time as it, if the same person wants to do both at the same time.

ockam_vault_test_suite is a set of utilities that can be used to test an implementation of the core vault traits. We're moving these traits into ockam_core (in #2288), and tentatively we're going to move the test suite there too. (While normally this might be behind a cargo feature, in this case it's probably more trouble than it's worth... or at least, we'll hold off for now[^1]).

I think we'll also change the name from test_suite to test_support — the old name is confusing since you think it's internal tests otherwise.

The basic steps here are:

  1. Add a new pub mod test_support; into ockam_core::vault;

  2. Move the code from ockam_vault_test_suite into ockam_core/src/vault/test_support. (renaming from lib.rs to mod.rs and such)

  3. Replace uses of ockam_vault_test_suite with ockam_core::vault::test_support.

  4. Delete the ockam_vault_test_suite crate, and references too it (be sure to check .github/workflows, and to do a search for ockam_vault_test_suite).

I believe that ockam_core requires documentation, and this code does not have any. Someone needs to write these, and if a contributor takes this issue (I've marked it as "good first issue"), that might be... difficult for them.

So, as a short term fix, I've written a brief blurb to document the module at least. You should put the following as the header in the ockam_core/src/vault/test_support/mod.rs file.

//! Various functions to help with testing implementations of the traits in
//! [`ockam_core::vault`](crate::vault).
#![allow(missing_docs)]

And then after the PR for this lands, I'll file an issue for us getting docs on file for these functions.

[^1]: These helpers have no additional dependencies that ockam_core doesn't also have, so the main benefit to feature-gating would be to reduce compile-time, by not compiling them.

While that *sounds* good, in practice the usage pattern here would mean the user would need different feature sets in `dev-dependencies` vs `dependencies`, which... can cause *more* build work under `resolver="2"` (which is required for this to be reasonable at all). That is, we'd actually cause slower builds, rather than faster ones (in some case).

Given that the amount of code here is pretty small, this doesn't seem worth it. OTOH, very few people will need the vault test helpers, so maybe it's worth it in practice... IDK — holding off on feature-gating it for now seems fine.

thomcc avatar Nov 30 '21 23:11 thomcc

@thomcc I can help with this as well. Can you assign this to me?

totsteps avatar Dec 08 '21 00:12 totsteps

Has this been resolved?

Jackbaude avatar Mar 17 '22 19:03 Jackbaude

@Jackbaude I don't think so. Feel free to grab it!

totsteps avatar Mar 17 '22 20:03 totsteps

@Jackbaude I don't think so. Feel free to grab it!

Yes I have started working on it! Thank you!

Jackbaude avatar Mar 18 '22 17:03 Jackbaude

This was my first PR - and first PR to really any open souce project - so let me know what I can do better and if I need to make any changes. I ran all tests and they passed locally. https://github.com/ockam-network/ockam/pull/2568.

Best, Jack

Jackbaude avatar Mar 19 '22 07:03 Jackbaude

This has been fixed with this PR: https://github.com/build-trust/ockam/pull/2579

etorreborre avatar Feb 01 '23 09:02 etorreborre