Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(infra): concurrent materializer tests #1243

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
13 changes: 13 additions & 0 deletions fendermint/testing/materializer/src/concurrency.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
pub struct Config {
LePremierHomme marked this conversation as resolved.
Show resolved Hide resolved
pub parallelism_level: usize
}

impl Config {
pub fn with_parallelism_level(v: usize) -> Self {
Self { parallelism_level: v }
}
}




LePremierHomme marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions fendermint/testing/materializer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pub mod validation;

#[cfg(feature = "arb")]
mod arb;
pub mod concurrency;

/// An ID identifying a resource within its parent.
#[derive(Clone, Serialize, PartialEq, Eq, PartialOrd, Ord)]
Expand Down
9 changes: 9 additions & 0 deletions fendermint/testing/materializer/src/testnet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,15 @@ where
.ok_or_else(|| anyhow!("account {id} does not exist"))
}

pub fn account_mod_nth(&self, v: usize) -> &M::Account {
let nth = v % self.accounts.len();
self.accounts
.iter()
.nth(nth)
.map(|(_, account)| account)
.unwrap()
}

/// Get a node by name.
pub fn node(&self, name: &NodeName) -> anyhow::Result<&M::Node> {
self.nodes
Expand Down
50 changes: 36 additions & 14 deletions fendermint/testing/materializer/tests/docker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,8 @@ use std::{

use anyhow::{anyhow, Context};
use ethers::providers::Middleware;
use fendermint_materializer::{
docker::{DockerMaterializer, DockerMaterials},
manifest::Manifest,
testnet::Testnet,
HasCometBftApi, HasEthApi, TestnetName,
};
use futures::Future;
use fendermint_materializer::{concurrency, docker::{DockerMaterializer, DockerMaterials}, manifest::Manifest, testnet::Testnet, HasCometBftApi, HasEthApi, TestnetName};
use futures::{future, Future};
use lazy_static::lazy_static;
use tendermint_rpc::Client;

Expand Down Expand Up @@ -59,13 +54,14 @@ fn read_manifest(file_name: &str) -> anyhow::Result<Manifest> {

/// Parse a manifest file in the `manifests` directory, clean up any corresponding
/// testnet resources, then materialize a testnet and run some tests.
pub async fn with_testnet<F, G>(manifest_file_name: &str, alter: G, test: F) -> anyhow::Result<()>
pub async fn with_testnet<F, G>(manifest_file_name: &str, concurrency: Option<concurrency::Config>, alter: G, test: F) -> anyhow::Result<()>
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I consider this inversion of control (IoC) as convenient utility -- rather bespoke -- that the original developer of the materializer used to create an initial batch of tests. I would not glorify it and turn it into a focal entrypoint for every future test.
  2. There isn't a single clear cut way we'll want all potentially tests that require some concurrency to behave, so I'm not sold on introducing concurrency as framework feature.

Instead, I think we're better served if we introduced a non-IoC API here:

  1. Extract the logic that actually materializes the definition into a separate function that returns the Manifest, DockerMaterializer, DockerTestnet.
  2. The test can now call this function to materialize a definition, do whatever it wants (using whatever concurrency it desires).
  3. Something needs to have a drop guard here that destroys the materialized testnet, probably the DockerTestnet? Not sure if that's implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've inverted the inversion, as you suggested, now providing a cleanup function. I don't think it's helpful to maintain 2 patterns, so I migrated prev tests.

It also helped to untangle concurrency from the framework, making it a simple test lib utility.

where
// https://users.rust-lang.org/t/function-that-takes-a-closure-with-mutable-reference-that-returns-a-future/54324
F: for<'a> FnOnce(
// https://users.rust-lang.org/t/function-that-takes-a-closure-with-mutable-reference-that-returns-a-future/54324
F: for<'a> Fn(
&Manifest,
&mut DockerMaterializer,
&'a mut DockerTestnet,
&DockerMaterializer,
&'a DockerTestnet,
usize
) -> Pin<Box<dyn Future<Output = anyhow::Result<()>> + 'a>>,
G: FnOnce(&mut Manifest),
{
Expand Down Expand Up @@ -98,14 +94,40 @@ where
.await
.context("failed to remove testnet")?;

let mut testnet = Testnet::setup(&mut materializer, &testnet_name, &manifest)
let testnet = Testnet::setup(&mut materializer, &testnet_name, &manifest)
.await
.context("failed to set up testnet")?;

let started = wait_for_startup(&testnet).await?;

let res = if started {
test(&manifest, &mut materializer, &mut testnet).await
match concurrency {
None => test(&manifest, &materializer, &testnet, 0).await,
Some(cfg) => {
let mut futures = Vec::new();
let mut test_ids = Vec::new();
for i in 0..cfg.parallelism_level {
let test_id = i;
let task = test(&manifest, &materializer, &testnet, test_id);
LePremierHomme marked this conversation as resolved.
Show resolved Hide resolved
futures.push(task);
test_ids.push(test_id);
}

let results: Vec<Result<(), anyhow::Error>> = future::join_all(futures).await;
let mut err = None;
for (i, result) in results.into_iter().enumerate() {
let test_id = test_ids[i];
match result {
Ok(_) => println!("test completed successfully (test_id={})", test_id),
Err(e) => {
println!("test failed: {} (test_id={})", e, test_id);
err = Some(e);
},
}
}
err.map_or(Ok(()), Err)
}
}
} else {
Err(anyhow!("the startup sequence timed out"))
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const MAX_RETRIES: u32 = 5;
async fn test_topdown_and_bottomup() {
with_testnet(
MANIFEST,
None,
|manifest| {
// Try to make sure the bottom-up checkpoint period is quick enough for reasonable test runtime.
let subnet = manifest
Expand All @@ -36,7 +37,7 @@ async fn test_topdown_and_bottomup() {

subnet.bottom_up_checkpoint.period = CHECKPOINT_PERIOD;
},
|_, _, testnet| {
|_, _, testnet, _| {
let test = async {
let brussels = testnet.node(&testnet.root().node("brussels"))?;
let london = testnet.node(&testnet.root().subnet("england").node("london"))?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ const MANIFEST: &str = "root-only.yaml";
async fn test_full_node_sync() {
with_testnet(
MANIFEST,
None,
|_| {},
|_, _, testnet| {
|_, _, testnet, _| {
let test = async {
// Allow a little bit of time for node-2 to catch up with node-1.
tokio::time::sleep(Duration::from_secs(5)).await;
Expand Down
27 changes: 14 additions & 13 deletions fendermint/testing/materializer/tests/docker_tests/standalone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,9 @@ use ethers::{
signers::{Signer, Wallet},
types::{transaction::eip2718::TypedTransaction, Eip1559TransactionRequest, H160},
};
use fendermint_materializer::{manifest::Rootnet, materials::DefaultAccount, HasEthApi};
use fendermint_materializer::{concurrency, manifest::Rootnet, materials::DefaultAccount, HasEthApi};
use futures::FutureExt;

use crate::with_testnet;
use crate::{with_testnet};

const MANIFEST: &str = "standalone.yaml";

Expand Down Expand Up @@ -46,30 +45,31 @@ where
async fn test_sent_tx_found_in_mempool() {
with_testnet(
MANIFEST,
Some(concurrency::Config::with_parallelism_level(3)),
|manifest| {
// Slow down consensus to where we can see the effect of the transaction not being found by Ethereum hash.
if let Rootnet::New { ref mut env, .. } = manifest.rootnet {
env.insert("CMT_CONSENSUS_TIMEOUT_COMMIT".into(), "10s".into());
};
},
|_, _, testnet| {
let test = async {
let bob = testnet.account("bob")?;
let charlie = testnet.account("charlie")?;

|_, _, testnet, test_id| {
let sender = testnet.account_mod_nth(test_id);
let recipient = testnet.account_mod_nth(test_id + 1);
let test = async move {
println!("running (test_id={})", test_id);
let pangea = testnet.node(&testnet.root().node("pangea"))?;
let provider = pangea
.ethapi_http_provider()?
.expect("ethapi should be enabled");

let middleware = make_middleware(provider, bob)
let middleware = make_middleware(provider, sender)
.await
.context("failed to set up middleware")?;

eprintln!("middleware ready, pending tests");
println!("middleware ready, pending tests (test_id={})", test_id);

// Create the simplest transaction possible: send tokens between accounts.
let to: H160 = charlie.eth_addr().into();
let to: H160 = recipient.eth_addr().into();
let transfer = Eip1559TransactionRequest::new().to(to).value(1);

let pending: PendingTransaction<_> = middleware
Expand All @@ -79,7 +79,7 @@ async fn test_sent_tx_found_in_mempool() {

let tx_hash = pending.tx_hash();

eprintln!("sent pending txn {:?}", tx_hash);
println!("sent pending txn {:?} (test_id={})", tx_hash, test_id);

// We expect that the transaction is pending, however it should not return an error.
match middleware.get_transaction(tx_hash).await {
Expand Down Expand Up @@ -111,8 +111,9 @@ async fn test_out_of_order_mempool() {

with_testnet(
MANIFEST,
None,
|_| {},
|_, _, testnet| {
|_, _, testnet, _| {
let test = async {
let bob = testnet.account("bob")?;
let charlie = testnet.account("charlie")?;
Expand Down
Loading