From a92499f20360f0f6eba3452e6944702d6a50f56d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BC=A0=E7=82=8E=E6=B3=BC?= Date: Tue, 21 Mar 2023 20:45:49 +0800 Subject: [PATCH] Change: StoreBuilder does not need to run a test, it only needs to build a store `StoreBuilder::run_test()` is removed, and a new method `build()` is added. To test a `RaftStorage` implementation, just implementing `StoreBuilder::build()` is enough now. It returns a store instance and a **guard**, for disk backed store to clean up the data when the guard is dropped. --- memstore/src/test.rs | 9 +- openraft/src/testing/store_builder.rs | 56 ++++++------- openraft/src/testing/suite.rs | 115 +++++++++++++++----------- rocksstore-compat07/src/test.rs | 18 ++-- rocksstore/src/test.rs | 18 ++-- sledstore/src/test.rs | 42 ++-------- 6 files changed, 111 insertions(+), 147 deletions(-) diff --git a/memstore/src/test.rs b/memstore/src/test.rs index fb47fea5f..e93f763e9 100644 --- a/memstore/src/test.rs +++ b/memstore/src/test.rs @@ -1,4 +1,3 @@ -use std::future::Future; use std::sync::Arc; use async_trait::async_trait; @@ -13,13 +12,9 @@ use crate::MemStore; struct MemBuilder {} #[async_trait] impl StoreBuilder> for MemBuilder { - async fn run_test(&self, t: Fun) -> Result> - where - Res: Future>> + Send, - Fun: Fn(Arc) -> Res + Sync + Send, - { + async fn build(&self) -> Result<((), Arc), StorageError> { let store = MemStore::new_async().await; - t(store).await + Ok(((), store)) } } diff --git a/openraft/src/testing/store_builder.rs b/openraft/src/testing/store_builder.rs index c2158bc07..0c43f379f 100644 --- a/openraft/src/testing/store_builder.rs +++ b/openraft/src/testing/store_builder.rs @@ -1,11 +1,7 @@ -use std::fmt::Debug; -use std::future::Future; use std::marker::PhantomData; use async_trait::async_trait; -use crate::AppData; -use crate::AppDataResponse; use crate::DefensiveCheckBase; use crate::RaftStorage; use crate::RaftTypeConfig; @@ -13,54 +9,50 @@ use crate::StorageError; use crate::StoreExt; /// The trait to build a [`RaftStorage`] implementation. +/// +/// The generic parameter `C` is type config for a `RaftStorage` implementation, +/// `S` is the type that implements `RaftStorage`, +/// and `G` is a guard type that cleanup resource when being dropped. +/// +/// By default `G` is a trivial guard `()`. To test a store that is backed by a folder on disk, `G` +/// could be the dropper of the temp-dir that stores data. #[async_trait] -pub trait StoreBuilder: Send + Sync +pub trait StoreBuilder: Send + Sync where C: RaftTypeConfig, S: RaftStorage, { - async fn run_test(&self, t: Fun) -> Result> - where - Res: Future>> + Send, - Fun: Fn(S) -> Res + Sync + Send; + /// Build a [`RaftStorage`] implementation + async fn build(&self) -> Result<(G, S), StorageError>; } /// A builder for testing [`StoreExt`]. -pub struct DefensiveStoreBuilder +pub struct DefensiveStoreBuilder where C: RaftTypeConfig, - C::D: AppData + Debug, - C::R: AppDataResponse + Debug, BaseStore: RaftStorage, - BaseBuilder: StoreBuilder, + BaseBuilder: StoreBuilder, { pub base_builder: BaseBuilder, - pub s: PhantomData<(C, BaseStore)>, + pub s: PhantomData<(C, BaseStore, G)>, } #[async_trait] -impl StoreBuilder> - for DefensiveStoreBuilder +impl StoreBuilder, G> + for DefensiveStoreBuilder where C: RaftTypeConfig, - C::D: AppData + Debug, - C::R: AppDataResponse + Debug, BaseStore: RaftStorage, - BaseBuilder: StoreBuilder, + BaseBuilder: StoreBuilder, + G: Send + Sync, { - async fn run_test(&self, t: Fun) -> Result> - where - Res: Future>> + Send, - Fun: Fn(StoreExt) -> Res + Sync + Send, - { - self.base_builder - .run_test(|base_store| async { - let sto_ext = StoreExt::new(base_store); - sto_ext.set_defensive(true); - assert!(sto_ext.is_defensive(), "must impl defensive check"); - t(sto_ext).await - }) - .await + async fn build(&self) -> Result<(G, StoreExt), StorageError> { + let (g, store) = self.base_builder.build().await?; + let sto_ext = StoreExt::new(store); + sto_ext.set_defensive(true); + assert!(sto_ext.is_defensive(), "must impl defensive check"); + + Ok((g, sto_ext)) } } diff --git a/openraft/src/testing/suite.rs b/openraft/src/testing/suite.rs index c409ba9b4..a9905d335 100644 --- a/openraft/src/testing/suite.rs +++ b/openraft/src/testing/suite.rs @@ -46,32 +46,35 @@ macro_rules! btreeset { /// Test suite to ensure a `RaftStore` impl works as expected. /// /// Usage: -pub struct Suite +pub struct Suite where C: RaftTypeConfig, C::D: AppData + Debug, C::R: AppDataResponse + Debug, S: RaftStorage, - B: StoreBuilder, + B: StoreBuilder, + G: Send + Sync, { c: PhantomData, p: PhantomData, f: PhantomData, + g: PhantomData, } -impl Suite +impl Suite where C: RaftTypeConfig, C::D: AppData + Debug, C::R: AppDataResponse + Debug, C::NodeId: From, S: RaftStorage, - B: StoreBuilder, + B: StoreBuilder, + G: Send + Sync, { pub fn test_all(builder: B) -> Result<(), StorageError> { Suite::test_store(&builder)?; - let df_builder = DefensiveStoreBuilder:: { + let df_builder = DefensiveStoreBuilder:: { base_builder: builder, s: Default::default(), @@ -83,33 +86,33 @@ where } pub fn test_store(builder: &B) -> Result<(), StorageError> { - run_fut(builder.run_test(Self::last_membership_in_log_initial))?; - run_fut(builder.run_test(Self::last_membership_in_log))?; - run_fut(builder.run_test(Self::last_membership_in_log_multi_step))?; - run_fut(builder.run_test(Self::get_membership_initial))?; - run_fut(builder.run_test(Self::get_membership_from_log_and_empty_sm))?; - run_fut(builder.run_test(Self::get_membership_from_log_and_sm))?; - run_fut(builder.run_test(Self::get_initial_state_without_init))?; - run_fut(builder.run_test(Self::get_initial_state_membership_from_log_and_sm))?; - run_fut(builder.run_test(Self::get_initial_state_with_state))?; - run_fut(builder.run_test(Self::get_initial_state_last_log_gt_sm))?; - run_fut(builder.run_test(Self::get_initial_state_last_log_lt_sm))?; - run_fut(builder.run_test(Self::get_initial_state_log_ids))?; - run_fut(builder.run_test(Self::save_vote))?; - run_fut(builder.run_test(Self::get_log_entries))?; - run_fut(builder.run_test(Self::try_get_log_entry))?; - run_fut(builder.run_test(Self::initial_logs))?; - run_fut(builder.run_test(Self::get_log_state))?; - run_fut(builder.run_test(Self::get_log_id))?; - run_fut(builder.run_test(Self::last_id_in_log))?; - run_fut(builder.run_test(Self::last_applied_state))?; - run_fut(builder.run_test(Self::purge_logs_upto_0))?; - run_fut(builder.run_test(Self::purge_logs_upto_5))?; - run_fut(builder.run_test(Self::purge_logs_upto_20))?; - run_fut(builder.run_test(Self::delete_logs_since_11))?; - run_fut(builder.run_test(Self::delete_logs_since_0))?; - run_fut(builder.run_test(Self::append_to_log))?; - run_fut(builder.run_test(Self::snapshot_meta))?; + run_fut(run_test(builder, Self::last_membership_in_log_initial))?; + run_fut(run_test(builder, Self::last_membership_in_log))?; + run_fut(run_test(builder, Self::last_membership_in_log_multi_step))?; + run_fut(run_test(builder, Self::get_membership_initial))?; + run_fut(run_test(builder, Self::get_membership_from_log_and_empty_sm))?; + run_fut(run_test(builder, Self::get_membership_from_log_and_sm))?; + run_fut(run_test(builder, Self::get_initial_state_without_init))?; + run_fut(run_test(builder, Self::get_initial_state_membership_from_log_and_sm))?; + run_fut(run_test(builder, Self::get_initial_state_with_state))?; + run_fut(run_test(builder, Self::get_initial_state_last_log_gt_sm))?; + run_fut(run_test(builder, Self::get_initial_state_last_log_lt_sm))?; + run_fut(run_test(builder, Self::get_initial_state_log_ids))?; + run_fut(run_test(builder, Self::save_vote))?; + run_fut(run_test(builder, Self::get_log_entries))?; + run_fut(run_test(builder, Self::try_get_log_entry))?; + run_fut(run_test(builder, Self::initial_logs))?; + run_fut(run_test(builder, Self::get_log_state))?; + run_fut(run_test(builder, Self::get_log_id))?; + run_fut(run_test(builder, Self::last_id_in_log))?; + run_fut(run_test(builder, Self::last_applied_state))?; + run_fut(run_test(builder, Self::purge_logs_upto_0))?; + run_fut(run_test(builder, Self::purge_logs_upto_5))?; + run_fut(run_test(builder, Self::purge_logs_upto_20))?; + run_fut(run_test(builder, Self::delete_logs_since_11))?; + run_fut(run_test(builder, Self::delete_logs_since_0))?; + run_fut(run_test(builder, Self::append_to_log))?; + run_fut(run_test(builder, Self::snapshot_meta))?; // run_fut(Suite::apply_single(builder))?; // run_fut(Suite::apply_multi(builder))?; @@ -1035,31 +1038,32 @@ where // Defensive test: // If a RaftStore impl support defensive check, enable it and check if it returns errors when // abnormal input is seen. A RaftStore with defensive check is able to expose bugs in raft core. -impl Suite +impl Suite where C: RaftTypeConfig, C::D: AppData + Debug, C::R: AppDataResponse + Debug, C::NodeId: From, S: RaftStorage, - B: StoreBuilder, + B: StoreBuilder, + G: Send + Sync, { pub fn test_store_defensive(builder: &B) -> Result<(), StorageError> { - run_fut(builder.run_test(Self::df_get_membership_config_dirty_log))?; - run_fut(builder.run_test(Self::df_get_initial_state_dirty_log))?; - run_fut(builder.run_test(Self::df_save_vote_ascending))?; - run_fut(builder.run_test(Self::df_get_log_entries))?; - run_fut(builder.run_test(Self::df_append_to_log_nonempty_input))?; - run_fut(builder.run_test(Self::df_append_to_log_nonconsecutive_input))?; - run_fut(builder.run_test(Self::df_append_to_log_eq_last_plus_one))?; - run_fut(builder.run_test(Self::df_append_to_log_eq_last_applied_plus_one))?; - run_fut(builder.run_test(Self::df_append_to_log_gt_last_log_id))?; - run_fut(builder.run_test(Self::df_append_to_log_gt_last_applied_id))?; - run_fut(builder.run_test(Self::df_apply_nonempty_input))?; - run_fut(builder.run_test(Self::df_apply_index_eq_last_applied_plus_one))?; - run_fut(builder.run_test(Self::df_apply_gt_last_applied_id))?; - run_fut(builder.run_test(Self::df_purge_applied_le_last_applied))?; - run_fut(builder.run_test(Self::df_delete_conflict_gt_last_applied))?; + run_fut(run_test(builder, Self::df_get_membership_config_dirty_log))?; + run_fut(run_test(builder, Self::df_get_initial_state_dirty_log))?; + run_fut(run_test(builder, Self::df_save_vote_ascending))?; + run_fut(run_test(builder, Self::df_get_log_entries))?; + run_fut(run_test(builder, Self::df_append_to_log_nonempty_input))?; + run_fut(run_test(builder, Self::df_append_to_log_nonconsecutive_input))?; + run_fut(run_test(builder, Self::df_append_to_log_eq_last_plus_one))?; + run_fut(run_test(builder, Self::df_append_to_log_eq_last_applied_plus_one))?; + run_fut(run_test(builder, Self::df_append_to_log_gt_last_log_id))?; + run_fut(run_test(builder, Self::df_append_to_log_gt_last_applied_id))?; + run_fut(run_test(builder, Self::df_apply_nonempty_input))?; + run_fut(run_test(builder, Self::df_apply_index_eq_last_applied_plus_one))?; + run_fut(run_test(builder, Self::df_apply_gt_last_applied_id))?; + run_fut(run_test(builder, Self::df_purge_applied_le_last_applied))?; + run_fut(run_test(builder, Self::df_delete_conflict_gt_last_applied))?; Ok(()) } @@ -1534,3 +1538,16 @@ where rt.block_on(f)?; Ok(()) } + +/// Build a `RaftStorage` implementation and run a test on it. +async fn run_test(builder: &B, test_fn: TestFn) -> Result> +where + C: RaftTypeConfig, + S: RaftStorage, + B: StoreBuilder, + Fu: Future>> + Send, + TestFn: Fn(S) -> Fu + Sync + Send, +{ + let (_g, store) = builder.build().await?; + test_fn(store).await +} diff --git a/rocksstore-compat07/src/test.rs b/rocksstore-compat07/src/test.rs index 54ec1f337..52a0ff5ab 100644 --- a/rocksstore-compat07/src/test.rs +++ b/rocksstore-compat07/src/test.rs @@ -1,10 +1,10 @@ -use std::future::Future; use std::sync::Arc; use async_trait::async_trait; use openraft::testing::StoreBuilder; use openraft::testing::Suite; use openraft::StorageError; +use tempfile::TempDir; use crate::Config; use crate::RocksNodeId; @@ -12,19 +12,11 @@ use crate::RocksStore; struct RocksBuilder {} #[async_trait] -impl StoreBuilder> for RocksBuilder { - async fn run_test(&self, t: Fun) -> Result> - where - Res: Future>> + Send, - Fun: Fn(Arc) -> Res + Sync + Send, - { +impl StoreBuilder, TempDir> for RocksBuilder { + async fn build(&self) -> Result<(TempDir, Arc), StorageError> { let td = tempfile::TempDir::new().expect("couldn't create temp dir"); - let r = { - let store = RocksStore::new(td.path()).await; - t(store).await - }; - td.close().expect("could not close temp directory"); - r + let store = RocksStore::new(td.path()).await; + Ok((td, store)) } } diff --git a/rocksstore/src/test.rs b/rocksstore/src/test.rs index 06cd205c5..0bce5704a 100644 --- a/rocksstore/src/test.rs +++ b/rocksstore/src/test.rs @@ -1,10 +1,10 @@ -use std::future::Future; use std::sync::Arc; use async_trait::async_trait; use openraft::testing::StoreBuilder; use openraft::testing::Suite; use openraft::StorageError; +use tempfile::TempDir; use crate::Config; use crate::RocksNodeId; @@ -12,19 +12,11 @@ use crate::RocksStore; struct RocksBuilder {} #[async_trait] -impl StoreBuilder> for RocksBuilder { - async fn run_test(&self, t: Fun) -> Result> - where - Res: Future>> + Send, - Fun: Fn(Arc) -> Res + Sync + Send, - { +impl StoreBuilder, TempDir> for RocksBuilder { + async fn build(&self) -> Result<(TempDir, Arc), StorageError> { let td = tempfile::TempDir::new().expect("couldn't create temp dir"); - let r = { - let store = RocksStore::new(td.path()).await; - t(store).await - }; - td.close().expect("could not close temp directory"); - r + let store = RocksStore::new(td.path()).await; + Ok((td, store)) } } /// To customize a builder: diff --git a/sledstore/src/test.rs b/sledstore/src/test.rs index ca1021d39..96fff6ae4 100644 --- a/sledstore/src/test.rs +++ b/sledstore/src/test.rs @@ -1,19 +1,15 @@ -use std::future::Future; -use std::sync::atomic::AtomicUsize; -use std::sync::atomic::Ordering; use std::sync::Arc; use async_trait::async_trait; use openraft::testing::StoreBuilder; use openraft::testing::Suite; use openraft::StorageError; +use tempfile::TempDir; use crate::ExampleNodeId; use crate::ExampleTypeConfig; use crate::SledStore; -static GLOBAL_TEST_COUNT: AtomicUsize = AtomicUsize::new(0); - struct SledBuilder {} #[test] @@ -22,34 +18,14 @@ pub fn test_sled_store() -> Result<(), StorageError> { } #[async_trait] -impl StoreBuilder> for SledBuilder { - async fn run_test(&self, t: Fun) -> Result> - where - Res: Future>> + Send, - Fun: Fn(Arc) -> Res + Sync + Send, - { - let pid = std::process::id(); +impl StoreBuilder, TempDir> for SledBuilder { + async fn build(&self) -> Result<(TempDir, Arc), StorageError> { let td = tempfile::TempDir::new().expect("couldn't create temp dir"); - let temp_dir_path = td.path().to_str().expect("Could not convert temp dir"); - let r = { - let old_count = GLOBAL_TEST_COUNT.fetch_add(1, Ordering::SeqCst); - let db_dir_str = format!("{}pid{}/num{}/", &temp_dir_path, pid, old_count); - - let db_dir = std::path::Path::new(&db_dir_str); - if !db_dir.exists() { - std::fs::create_dir_all(db_dir).unwrap_or_else(|_| panic!("could not create: {:?}", db_dir.to_str())); - } - - let db: sled::Db = sled::open(db_dir).unwrap_or_else(|_| panic!("could not open: {:?}", db_dir.to_str())); - - let store = SledStore::new(Arc::new(db)).await; - let test_res = t(store).await; - - if db_dir.exists() { - std::fs::remove_dir_all(db_dir).expect("Could not clean up test directory"); - } - test_res - }; - r + + let db: sled::Db = sled::open(td.path()).unwrap(); + + let store = SledStore::new(Arc::new(db)).await; + + Ok((td, store)) } }