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

pvf: use test-utils feature to export test only #7538

Merged
merged 11 commits into from
Aug 14, 2023
Merged

pvf: use test-utils feature to export test only #7538

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cb9d3f2
pvf: use test-utils feature to export test only
jpserrat Jul 22, 2023
b72c655
adding comment to test-utils feature
jpserrat Jul 25, 2023
db467ff
make prepare-worker and execute-worker as optional dependencies and a…
jpserrat Aug 2, 2023
49067f9
Merge branch 'master' into jpserrat-pvf-add-test-utils-and-remove-doc…
mrcnski Aug 2, 2023
05f8c9a
remove doc hidden from pvf testing
jpserrat Aug 2, 2023
e6cca27
add prepare worker and execute worker entrypoints to test-utils feature
jpserrat Aug 2, 2023
f8e10ce
pvf: add sp_tracing as optional dependency of test-utils
jpserrat Aug 8, 2023
735344a
add test-utils for polkadot and malus
jpserrat Aug 9, 2023
41190ef
add test-utils feature to prepare and execute workers script
jpserrat Aug 9, 2023
6c46d31
remove required features from prepare and executing
jpserrat Aug 9, 2023
ca94de7
Try to trigger CI again to fix broken jobs
mrcnski Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions node/core/pvf/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ edition.workspace = true
[[bin]]
name = "puppet_worker"
path = "bin/puppet_worker.rs"
required-features = ["test-utils"]
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
mrcnski marked this conversation as resolved.
Show resolved Hide resolved

[dependencies]
mrcnski marked this conversation as resolved.
Show resolved Hide resolved
always-assert = "0.1"
Expand Down Expand Up @@ -42,9 +43,12 @@ substrate-build-script-utils = { git = "https://github.com/paritytech/substrate"
[dev-dependencies]
assert_matches = "1.4.0"
hex-literal = "0.3.4"
polkadot-node-core-pvf-common = { path = "common", features = ["test-utils"] }

adder = { package = "test-parachain-adder", path = "../../../parachain/test-parachains/adder" }
halt = { package = "test-parachain-halt", path = "../../../parachain/test-parachains/halt" }

[features]
ci-only-tests = []
# This feature is used to export test code to other crates without putting it in the production build.
test-utils = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment please on how to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm going to add. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggwpez Comment added. Is this what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding comments. 🙌 But I think it is not entirely correct. The feature is not just for exporting code but also for building the puppet worker which is part of this crate.

4 changes: 4 additions & 0 deletions node/core/pvf/common/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,7 @@ tempfile = "3.3.0"

[build-dependencies]
substrate-build-script-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }

[features]
# This feature is used to export test code to other crates without putting it in the production build.
test-utils = []
2 changes: 1 addition & 1 deletion node/core/pvf/common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const LOG_TARGET: &str = "parachain::pvf-common";
use std::mem;
use tokio::io::{self, AsyncRead, AsyncReadExt as _, AsyncWrite, AsyncWriteExt as _};

#[doc(hidden)]
#[cfg(feature = "test-utils")]
pub mod tests {
use std::time::Duration;

Expand Down
6 changes: 3 additions & 3 deletions node/core/pvf/common/src/pvf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ impl PvfPrepData {
}

/// Creates a structure for tests.
#[doc(hidden)]
#[cfg(feature = "test-utils")]
pub fn from_discriminator_and_timeout(num: u32, timeout: Duration) -> Self {
let descriminator_buf = num.to_le_bytes().to_vec();
Self::from_code(
Expand All @@ -96,13 +96,13 @@ impl PvfPrepData {
}

/// Creates a structure for tests.
#[doc(hidden)]
#[cfg(feature = "test-utils")]
pub fn from_discriminator(num: u32) -> Self {
Self::from_discriminator_and_timeout(num, crate::tests::TEST_PREPARATION_TIMEOUT)
}

/// Creates a structure for tests.
#[doc(hidden)]
#[cfg(feature = "test-utils")]
pub fn from_discriminator_precheck(num: u32) -> Self {
let mut pvf =
Self::from_discriminator_and_timeout(num, crate::tests::TEST_PREPARATION_TIMEOUT);
Expand Down
2 changes: 1 addition & 1 deletion node/core/pvf/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ mod worker_intf;
pub mod testing;

// Used by `decl_puppet_worker_main!`.
#[doc(hidden)]
#[cfg(feature = "test-utils")]
pub use sp_tracing;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the dependency itself become optional too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in this case, the dependency will only be enabled if the test-utils feature is being used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be re-exported with the feature, however it's always there in toml file:

sp-tracing = { git = "https://github.com/paritytech/substrate", branch = "master" }

My question is could it be made optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the misunderstanding. Done.


pub use error::{InvalidCandidate, ValidationError};
Expand Down
3 changes: 3 additions & 0 deletions node/core/pvf/src/testing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@
#[doc(hidden)]
mrcnski marked this conversation as resolved.
Show resolved Hide resolved
pub use crate::worker_intf::{spawn_with_program_path, SpawnErr};

#[cfg(feature = "test-utils")]
mrcnski marked this conversation as resolved.
Show resolved Hide resolved
use polkadot_primitives::ExecutorParams;

/// A function that emulates the stitches together behaviors of the preparation and the execution
/// worker in a single synchronous function.
#[cfg(feature = "test-utils")]
pub fn validate_candidate(
code: &[u8],
params: &[u8],
Expand All @@ -52,6 +54,7 @@ pub fn validate_candidate(
/// Use this macro to declare a `fn main() {}` that will check the arguments and dispatch them to
/// the appropriate worker, making the executable that can be used for spawning workers.
#[macro_export]
#[cfg(feature = "test-utils")]
macro_rules! decl_puppet_worker_main {
() => {
fn main() {
Expand Down
6 changes: 4 additions & 2 deletions node/core/pvf/tests/it/worker_common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@
// You should have received a copy of the GNU General Public License
// along with Polkadot. If not, see <http://www.gnu.org/licenses/>.

use crate::PUPPET_EXE;
use polkadot_node_core_pvf::testing::{spawn_with_program_path, SpawnErr};
use std::time::Duration;

use polkadot_node_core_pvf::testing::{spawn_with_program_path, SpawnErr};

use crate::PUPPET_EXE;

// Test spawning a program that immediately exits with a failure code.
#[tokio::test]
async fn spawn_immediate_exit() {
Expand Down
15 changes: 10 additions & 5 deletions parachain/test-parachains/adder/collator/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ path = "src/main.rs"
[[bin]]
name = "adder_collator_puppet_worker"
path = "bin/puppet_worker.rs"
required-features = ["test-utils"]
mrcnski marked this conversation as resolved.
Show resolved Hide resolved

[dependencies]
parity-scale-codec = { version = "3.6.1", default-features = false, features = ["derive"] }
Expand All @@ -31,17 +32,21 @@ sc-cli = { git = "https://github.com/paritytech/substrate", branch = "master" }
sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" }
sc-service = { git = "https://github.com/paritytech/substrate", branch = "master" }

# This one is tricky. Even though it is not used directly by the collator, we still need it for the
# `puppet_worker` binary, which is required for the integration test. However, this shouldn't be
# a big problem since it is used transitively anyway.
polkadot-node-core-pvf = { path = "../../../../node/core/pvf" }

[dev-dependencies]
polkadot-parachain = { path = "../../.." }
polkadot-test-service = { path = "../../../../node/test/service" }
# This one is tricky. Even though it is not used directly by the collator, we still need it for the
# `puppet_worker` binary, which is required for the integration test. However, this shouldn't be
# a big problem since it is used transitively anyway.
polkadot-node-core-pvf = { path = "../../../../node/core/pvf", features = ["test-utils"] }

substrate-test-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }
sc-service = { git = "https://github.com/paritytech/substrate", branch = "master" }
sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" }

tokio = { version = "1.24.2", features = ["macros"] }

[features]
# This feature is used to export test code to other crates without putting it in the production build.
# This is also used by the `puppet_worker` binary.
test-utils = ["polkadot-node-core-pvf/test-utils"]
15 changes: 10 additions & 5 deletions parachain/test-parachains/undying/collator/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ path = "src/main.rs"
[[bin]]
name = "undying_collator_puppet_worker"
path = "bin/puppet_worker.rs"
required-features = ["test-utils"]

[dependencies]
parity-scale-codec = { version = "3.6.1", default-features = false, features = ["derive"] }
Expand All @@ -31,17 +32,21 @@ sc-cli = { git = "https://github.com/paritytech/substrate", branch = "master" }
sp-core = { git = "https://github.com/paritytech/substrate", branch = "master" }
sc-service = { git = "https://github.com/paritytech/substrate", branch = "master" }

# This one is tricky. Even though it is not used directly by the collator, we still need it for the
# `puppet_worker` binary, which is required for the integration test. However, this shouldn't be
# a big problem since it is used transitively anyway.
polkadot-node-core-pvf = { path = "../../../../node/core/pvf" }

[dev-dependencies]
polkadot-parachain = { path = "../../.." }
polkadot-test-service = { path = "../../../../node/test/service" }
# This one is tricky. Even though it is not used directly by the collator, we still need it for the
# `puppet_worker` binary, which is required for the integration test. However, this shouldn't be
# a big problem since it is used transitively anyway.
polkadot-node-core-pvf = { path = "../../../../node/core/pvf", features = ["test-utils"] }

substrate-test-utils = { git = "https://github.com/paritytech/substrate", branch = "master" }
sc-service = { git = "https://github.com/paritytech/substrate", branch = "master" }
sp-keyring = { git = "https://github.com/paritytech/substrate", branch = "master" }

tokio = { version = "1.24.2", features = ["macros"] }

[features]
# This feature is used to export test code to other crates without putting it in the production build.
# This is also used by the `puppet_worker` binary.
test-utils = ["polkadot-node-core-pvf/test-utils"]