ockam
ockam copied to clipboard
Move `ockam_vault_test_suite` to `ockam_core::vault::test_support`
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:
-
Add a new
pub mod test_support;intoockam_core::vault; -
Move the code from
ockam_vault_test_suiteintoockam_core/src/vault/test_support. (renaming fromlib.rstomod.rsand such) -
Replace uses of
ockam_vault_test_suitewithockam_core::vault::test_support. -
Delete the
ockam_vault_test_suitecrate, and references too it (be sure to check .github/workflows, and to do a search forockam_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 I can help with this as well. Can you assign this to me?
Has this been resolved?
@Jackbaude I don't think so. Feel free to grab it!
@Jackbaude I don't think so. Feel free to grab it!
Yes I have started working on it! Thank you!
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
This has been fixed with this PR: https://github.com/build-trust/ockam/pull/2579