Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

PVF: add test-utils feature and remove #[doc(hidden)] #7285

Closed
mrcnski opened this issue May 24, 2023 · 6 comments · Fixed by #7538
Closed

PVF: add test-utils feature and remove #[doc(hidden)] #7285

mrcnski opened this issue May 24, 2023 · 6 comments · Fixed by #7538
Assignees
Labels
I8-refactor Code needs refactoring. J7-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@mrcnski
Copy link
Contributor

mrcnski commented May 24, 2023

ISSUE

Overview

In the PVF modules we export a good amount of stuff for only testing in other crates. #[cfg(test)] does not expose code across crates so we use #[doc(hidden)] for exposing test methods. However it's unfortunate that test code ends up in production builds this way. This can be done in a nicer way by adding a new feature.

Credit to @s0me0ne-unkn0wn here.

@mrcnski mrcnski added I8-refactor Code needs refactoring. J7-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. labels May 24, 2023
@jpserrat
Copy link
Contributor

Hi @mrcnski, I want to work on this, just have a few questions.
The ones that I would have to change to feature would be this ones:

#[doc(hidden)]
pub async fn spawn_with_program_path(

#[doc(hidden)]
pub mod tests {

#[doc(hidden)]
pub fn from_discriminator_and_timeout(num: u32, timeout: Duration) -> Self {

#[doc(hidden)]
pub fn from_discriminator(num: u32) -> Self {

#[doc(hidden)]
pub fn from_discriminator_precheck(num: u32) -> Self {

#[doc(hidden)]
pub mod testing;

// Used by `decl_puppet_worker_main!`.
#[doc(hidden)]
pub use sp_tracing;

Is that correct?
May I assume that all functions using them are test functions?

@s0me0ne-unkn0wn
Copy link
Contributor

from_discriminator_* are definitely test-only functions (they create meaningless dummy objects just for tests), spawn_with_program_path is documented as test-only, but I'd check it twice just in case.

Also, I'm not sure if the CI scripts in scripts/ have to be patched to make use of the new feature (see test.yml in the first place), or it will just work as-is with cargo test.

@ggwpez
Copy link
Member

ggwpez commented Jul 20, 2023

Yea I am using the dev-dependency feature approach in another project and it works fine so far. Should be much better than #[test] anyway.

@s0me0ne-unkn0wn
Copy link
Contributor

@jpserrat as far as I see, the spawn_with_program_path is used in production code but just wouldn't need to be publicly exposed if it wasn't needed for integration tests. So it's okay to just keep #[doc(hidden)] and leave it alone.

@mrcnski
Copy link
Contributor Author

mrcnski commented Jul 20, 2023

Would also be nice to gate the puppet worker behind the new feature (as anything related to the puppet worker is test-only). I think it can be done with required-features in the [[bin]] section in Cargo.toml. This would improve compile times as we wouldn't always build the puppet worker on non-test builds. 🙌

@jpserrat
Copy link
Contributor

Thanks a lot for the help and suggestions with this one. Let me know if there are some improvements to be made.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I8-refactor Code needs refactoring. J7-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants