diff --git a/substrate/frame/balances/src/benchmarking.rs b/substrate/frame/balances/src/benchmarking.rs index a6206cd84f..3c2067559f 100644 --- a/substrate/frame/balances/src/benchmarking.rs +++ b/substrate/frame/balances/src/benchmarking.rs @@ -51,10 +51,13 @@ benchmarks! { let _ = as Currency<_>>::make_free_balance_be(&caller, balance); // Transfer `e - 1` existential deposits + 1 unit, which guarantees to create one account, and reap this user. - let recipient = account("recipient", u, SEED); - let recipient_lookup: ::Source = T::Lookup::unlookup(recipient); + let recipient: T::AccountId = account("recipient", u, SEED); + let recipient_lookup: ::Source = T::Lookup::unlookup(recipient.clone()); let transfer_amount = existential_deposit.saturating_mul((e - 1).into()) + 1.into(); }: _(RawOrigin::Signed(caller), recipient_lookup, transfer_amount) + verify { + assert_eq!(Balances::::free_balance(&recipient), transfer_amount); + } // Benchmark `transfer` with the best possible condition: // * Both accounts exist and will continue to exist. diff --git a/substrate/frame/benchmarking/src/lib.rs b/substrate/frame/benchmarking/src/lib.rs index 49f398d59d..6bb10f3d97 100644 --- a/substrate/frame/benchmarking/src/lib.rs +++ b/substrate/frame/benchmarking/src/lib.rs @@ -28,7 +28,7 @@ pub use utils::*; pub use analysis::Analysis; #[doc(hidden)] pub use sp_io::storage::root as storage_root; -pub use sp_runtime::traits::Dispatchable; +pub use sp_runtime::traits::{Dispatchable, Zero}; pub use paste; /// Construct pallet benchmarks for weighing dispatchables. @@ -132,6 +132,25 @@ pub use paste; /// benchmark just like a regular benchmark, but only testing at the lowest and highest values for /// each component. The function will return `Ok(())` if the benchmarks return no errors. /// +/// You can optionally add a `verify` code block at the end of a benchmark to test any final state +/// of your benchmark in a unit test. For example: +/// +/// ```ignore +/// sort_vector { +/// let x in 1 .. 10000; +/// let mut m = Vec::::new(); +/// for i in (0..x).rev() { +/// m.push(i); +/// } +/// }: { +/// m.sort(); +/// } verify { +/// ensure!(m[0] == 0, "You forgot to sort!") +/// } +/// ``` +/// +/// These `verify` blocks will not execute when running your actual benchmarks! +/// /// You can construct benchmark tests like so: /// /// ```ignore @@ -192,6 +211,7 @@ macro_rules! benchmarks_iter { { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: _ ( $origin:expr $( , $arg:expr )* ) + verify $postcode:block $( $rest:tt )* ) => { $crate::benchmarks_iter! { @@ -199,6 +219,7 @@ macro_rules! benchmarks_iter { { $( $common )* } ( $( $names )* ) $name { $( $code )* }: $name ( $origin $( , $arg )* ) + verify $postcode $( $rest )* } }; @@ -208,6 +229,7 @@ macro_rules! benchmarks_iter { { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* ) + verify $postcode:block $( $rest:tt )* ) => { $crate::benchmarks_iter! { @@ -217,6 +239,7 @@ macro_rules! benchmarks_iter { $name { $( $code )* }: { as $crate::Dispatchable>::dispatch(Call::::$dispatch($($arg),*), $origin.into())?; } + verify $postcode $( $rest )* } }; @@ -226,6 +249,7 @@ macro_rules! benchmarks_iter { { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* ) + verify $postcode:block $( $rest:tt )* ) => { $crate::benchmarks_iter! { @@ -235,6 +259,7 @@ macro_rules! benchmarks_iter { $name { $( $code )* }: { as $crate::Dispatchable>::dispatch(Call::::$dispatch($($arg),*), $origin.into())?; } + verify $postcode $( $rest )* } }; @@ -244,6 +269,7 @@ macro_rules! benchmarks_iter { { $( $common:tt )* } ( $( $names:ident )* ) $name:ident { $( $code:tt )* }: $eval:block + verify $postcode:block $( $rest:tt )* ) => { $crate::benchmark_backend! { @@ -253,6 +279,7 @@ macro_rules! benchmarks_iter { { } { $eval } { $( $code )* } + $postcode } $crate::benchmarks_iter!( $instance @@ -266,8 +293,59 @@ macro_rules! benchmarks_iter { $crate::selected_benchmark!( $instance $( $names ),* ); $crate::impl_benchmark!( $instance $( $names ),* ); #[cfg(test)] - $crate::impl_benchmark_tests!( $( $names ),* ); - } + $crate::impl_benchmark_tests!( $instance $( $names ),* ); + }; + // add verify block to _() format + ( + $instance:ident + { $( $common:tt )* } + ( $( $names:ident )* ) + $name:ident { $( $code:tt )* }: _ ( $origin:expr $( , $arg:expr )* ) + $( $rest:tt )* + ) => { + $crate::benchmarks_iter! { + $instance + { $( $common )* } + ( $( $names )* ) + $name { $( $code )* }: _ ( $origin $( , $arg )* ) + verify { } + $( $rest )* + } + }; + // add verify block to name() format + ( + $instance:ident + { $( $common:tt )* } + ( $( $names:ident )* ) + $name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* ) + $( $rest:tt )* + ) => { + $crate::benchmarks_iter! { + $instance + { $( $common )* } + ( $( $names )* ) + $name { $( $code )* }: $dispatch ( $origin $( , $arg )* ) + verify { } + $( $rest )* + } + }; + // add verify block to {} format + ( + $instance:ident + { $( $common:tt )* } + ( $( $names:ident )* ) + $name:ident { $( $code:tt )* }: $eval:block + $( $rest:tt )* + ) => { + $crate::benchmarks_iter!( + $instance + { $( $common )* } + ( $( $names )* ) + $name { $( $code )* }: $eval + verify { } + $( $rest )* + ); + }; } #[macro_export] @@ -281,12 +359,12 @@ macro_rules! benchmark_backend { } { $eval:block } { let $pre_id:tt : $pre_ty:ty = $pre_ex:expr; $( $rest:tt )* - } ) => { + } $postcode:block) => { $crate::benchmark_backend! { $instance $name { $( $common )* } { $( PRE { $( $pre_parsed )* } )* PRE { $pre_id , $pre_ty , $pre_ex } - } { $eval } { $( $rest )* } + } { $eval } { $( $rest )* } $postcode } }; ($instance:ident $name:ident { @@ -296,12 +374,12 @@ macro_rules! benchmark_backend { } { $eval:block } { let $param:ident in ( $param_from:expr ) .. $param_to:expr => $param_instancer:expr; $( $rest:tt )* - }) => { + } $postcode:block) => { $crate::benchmark_backend! { $instance $name { $( $common )* } { $( $parsed )* PARAM { $param , $param_from , $param_to , $param_instancer } - } { $eval } { $( $rest )* } + } { $eval } { $( $rest )* } $postcode } }; // mutation arm to look after defaulting to a common param @@ -312,7 +390,7 @@ macro_rules! benchmark_backend { } { $eval:block } { let $param:ident in ...; $( $rest:tt )* - }) => { + } $postcode:block) => { $crate::benchmark_backend! { $instance $name { $( { $common , $common_from , $common_to , $common_instancer } )* @@ -324,7 +402,7 @@ macro_rules! benchmark_backend { .. ({ $( let $common = $common_to; )* $param }) => ({ $( let $common = || -> Result<(), &'static str> { $common_instancer ; Ok(()) }; )* $param()? }); $( $rest )* - } + } $postcode } }; // mutation arm to look after defaulting only the range to common param @@ -335,7 +413,7 @@ macro_rules! benchmark_backend { } { $eval:block } { let $param:ident in _ .. _ => $param_instancer:expr ; $( $rest:tt )* - }) => { + } $postcode:block) => { $crate::benchmark_backend! { $instance $name { $( { $common , $common_from , $common_to , $common_instancer } )* @@ -347,7 +425,7 @@ macro_rules! benchmark_backend { .. ({ $( let $common = $common_to; )* $param }) => $param_instancer ; $( $rest )* - } + } $postcode } }; // mutation arm to look after a single tt for param_from. @@ -358,12 +436,12 @@ macro_rules! benchmark_backend { } { $eval:block } { let $param:ident in $param_from:tt .. $param_to:expr => $param_instancer:expr ; $( $rest:tt )* - }) => { + } $postcode:block) => { $crate::benchmark_backend! { $instance $name { $( $common )* } { $( $parsed )* } { $eval } { let $param in ( $param_from ) .. $param_to => $param_instancer; $( $rest )* - } + } $postcode } }; // mutation arm to look after the default tail of `=> ()` @@ -374,12 +452,12 @@ macro_rules! benchmark_backend { } { $eval:block } { let $param:ident in $param_from:tt .. $param_to:expr; $( $rest:tt )* - }) => { + } $postcode:block) => { $crate::benchmark_backend! { $instance $name { $( $common )* } { $( $parsed )* } { $eval } { let $param in $param_from .. $param_to => (); $( $rest )* - } + } $postcode } }; // mutation arm to look after `let _ =` @@ -390,12 +468,12 @@ macro_rules! benchmark_backend { } { $eval:block } { let $pre_id:tt = $pre_ex:expr; $( $rest:tt )* - }) => { + } $postcode:block) => { $crate::benchmark_backend! { $instance $name { $( $common )* } { $( $parsed )* } { $eval } { let $pre_id : _ = $pre_ex; $( $rest )* - } + } $postcode } }; // no instance actioning arm @@ -404,7 +482,7 @@ macro_rules! benchmark_backend { } { $( PRE { $pre_id:tt , $pre_ty:ty , $pre_ex:expr } )* $( PARAM { $param:ident , $param_from:expr , $param_to:expr , $param_instancer:expr } )* - } { $eval:block } { $( $post:tt )* } ) => { + } { $eval:block } { $( $post:tt )* } $postcode:block) => { #[allow(non_camel_case_types)] struct $name; #[allow(unused_variables)] @@ -435,6 +513,25 @@ macro_rules! benchmark_backend { Ok(Box::new(move || -> Result<(), &'static str> { $eval; Ok(()) })) } + + fn verify(&self, components: &[($crate::BenchmarkParameter, u32)]) + -> Result Result<(), &'static str>>, &'static str> + { + $( + let $common = $common_from; + )* + $( + // Prepare instance + let $param = components.iter().find(|&c| c.0 == $crate::BenchmarkParameter::$param).unwrap().1; + )* + $( + let $pre_id : $pre_ty = $pre_ex; + )* + $( $param_instancer ; )* + $( $post )* + + Ok(Box::new(move || -> Result<(), &'static str> { $eval; $postcode; Ok(()) })) + } } }; // instance actioning arm @@ -443,7 +540,7 @@ macro_rules! benchmark_backend { } { $( PRE { $pre_id:tt , $pre_ty:ty , $pre_ex:expr } )* $( PARAM { $param:ident , $param_from:expr , $param_to:expr , $param_instancer:expr } )* - } { $eval:block } { $( $post:tt )* } ) => { + } { $eval:block } { $( $post:tt )* } $postcode:block) => { #[allow(non_camel_case_types)] struct $name; #[allow(unused_variables)] @@ -474,6 +571,25 @@ macro_rules! benchmark_backend { Ok(Box::new(move || -> Result<(), &'static str> { $eval; Ok(()) })) } + + fn verify(&self, components: &[($crate::BenchmarkParameter, u32)]) + -> Result Result<(), &'static str>>, &'static str> + { + $( + let $common = $common_from; + )* + $( + // Prepare instance + let $param = components.iter().find(|&c| c.0 == $crate::BenchmarkParameter::$param).unwrap().1; + )* + $( + let $pre_id : $pre_ty = $pre_ex; + )* + $( $param_instancer ; )* + $( $post )* + + Ok(Box::new(move || -> Result<(), &'static str> { $eval; $postcode; Ok(()) })) + } } } } @@ -518,6 +634,14 @@ macro_rules! selected_benchmark { $( Self::$bench => <$bench as $crate::BenchmarkingSetup>::instance(&$bench, components), )* } } + + fn verify(&self, components: &[($crate::BenchmarkParameter, u32)]) + -> Result Result<(), &'static str>>, &'static str> + { + match self { + $( Self::$bench => <$bench as $crate::BenchmarkingSetup>::verify(&$bench, components), )* + } + } } }; ( @@ -544,6 +668,14 @@ macro_rules! selected_benchmark { $( Self::$bench => <$bench as $crate::BenchmarkingSetupInstance>::instance(&$bench, components), )* } } + + fn verify(&self, components: &[($crate::BenchmarkParameter, u32)]) + -> Result Result<(), &'static str>>, &'static str> + { + match self { + $( Self::$bench => <$bench as $crate::BenchmarkingSetupInstance>::verify(&$bench, components), )* + } + } } } } @@ -618,20 +750,25 @@ macro_rules! impl_benchmark { // Run the benchmark `repeat` times. for _ in 0..repeat { - // Set the block number to 1 so events are deposited. - frame_system::Module::::set_block_number(1.into()); // Set up the externalities environment for the setup we want to benchmark. let closure_to_benchmark = >::instance(&selected_benchmark, &c)?; + // Set the block number to at least 1 so events are deposited. + if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { + frame_system::Module::::set_block_number(1.into()); + } + // Commit the externalities to the database, flushing the DB cache. // This will enable worst case scenario for reading from the database. $crate::benchmarking::commit_db(); // Time the extrinsic logic. + frame_support::debug::trace!(target: "benchmark", "Start Benchmark: {:?} {:?}", name, component_value); let start_extrinsic = $crate::benchmarking::current_time(); closure_to_benchmark()?; let finish_extrinsic = $crate::benchmarking::current_time(); let elapsed_extrinsic = finish_extrinsic - start_extrinsic; + frame_support::debug::trace!(target: "benchmark", "End Benchmark: {} ns", elapsed_extrinsic); // Time the storage root recalculation. let start_storage_root = $crate::benchmarking::current_time(); @@ -718,20 +855,25 @@ macro_rules! impl_benchmark { // Run the benchmark `repeat` times. for _ in 0..repeat { - // Set the block number to 1 so events are deposited. - frame_system::Module::::set_block_number(1.into()); // Set up the externalities environment for the setup we want to benchmark. let closure_to_benchmark = >::instance(&selected_benchmark, &c)?; + // Set the block number to at least 1 so events are deposited. + if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { + frame_system::Module::::set_block_number(1.into()); + } + // Commit the externalities to the database, flushing the DB cache. // This will enable worst case scenario for reading from the database. $crate::benchmarking::commit_db(); // Time the extrinsic logic. + frame_support::debug::trace!(target: "benchmark", "Start Benchmark: {:?} {:?}", name, component_value); let start_extrinsic = $crate::benchmarking::current_time(); closure_to_benchmark()?; let finish_extrinsic = $crate::benchmarking::current_time(); let elapsed_extrinsic = finish_extrinsic - start_extrinsic; + frame_support::debug::trace!(target: "benchmark", "End Benchmark: {} ns", elapsed_extrinsic); // Time the storage root recalculation. let start_storage_root = $crate::benchmarking::current_time(); @@ -758,6 +900,7 @@ macro_rules! impl_benchmark { #[macro_export] macro_rules! impl_benchmark_tests { ( + NO_INSTANCE $( $name:ident ),* ) => { $( @@ -783,12 +926,17 @@ macro_rules! impl_benchmark_tests { ) .collect(); - // Set the block number to 1 so events are deposited. - frame_system::Module::::set_block_number(1.into()); - // Set up the externalities environment for the setup we want to benchmark. - let closure_to_benchmark = >::instance(&selected_benchmark, &c)?; - // Run the benchmark - closure_to_benchmark()?; + // Set up the verification state + let closure_to_verify = >::verify(&selected_benchmark, &c)?; + + // Set the block number to at least 1 so events are deposited. + if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { + frame_system::Module::::set_block_number(1.into()); + } + + // Run verification + closure_to_verify()?; + // Reset the state $crate::benchmarking::wipe_db(); } @@ -797,7 +945,54 @@ macro_rules! impl_benchmark_tests { } } )* - } + }; + ( + INSTANCE + $( $name:ident ),* + ) => { + $( + $crate::paste::item! { + fn [] () -> Result<(), &'static str> + where T: frame_system::Trait + { + let selected_benchmark = SelectedBenchmark::$name; + let components = >::components(&selected_benchmark); + + for (_, (name, low, high)) in components.iter().enumerate() { + // Test only the low and high value, assuming values in the middle won't break + for component_value in vec![low, high] { + // Select the max value for all the other components. + let c: Vec<($crate::BenchmarkParameter, u32)> = components.iter() + .enumerate() + .map(|(_, (n, _, h))| + if n == name { + (*n, *component_value) + } else { + (*n, *h) + } + ) + .collect(); + + // Set up the verification state + let closure_to_verify = >::verify(&selected_benchmark, &c)?; + + // Set the block number to at least 1 so events are deposited. + if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { + frame_system::Module::::set_block_number(1.into()); + } + + // Run verification + closure_to_verify()?; + + // Reset the state + $crate::benchmarking::wipe_db(); + } + } + Ok(()) + } + } + )* + }; } diff --git a/substrate/frame/benchmarking/src/tests.rs b/substrate/frame/benchmarking/src/tests.rs index 3e79ca3875..50a39d0fcf 100644 --- a/substrate/frame/benchmarking/src/tests.rs +++ b/substrate/frame/benchmarking/src/tests.rs @@ -23,20 +23,28 @@ use codec::Decode; use sp_std::prelude::*; use sp_runtime::{traits::{BlakeTwo256, IdentityLookup}, testing::{H256, Header}}; use frame_support::{ - dispatch::DispatchResult, decl_module, impl_outer_origin, assert_ok, assert_err, ensure + dispatch::DispatchResult, decl_module, decl_storage, impl_outer_origin, + assert_ok, assert_err, ensure }; use frame_system::{RawOrigin, ensure_signed, ensure_none}; +decl_storage! { + trait Store for Module as Test { + Value get(fn value): Option; + } +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { #[weight = frame_support::weights::SimpleDispatchInfo::default()] - fn dummy(origin, _n: u32) -> DispatchResult { + fn set_value(origin, n: u32) -> DispatchResult { let _sender = ensure_signed(origin)?; + Value::put(n); Ok(()) } #[weight = frame_support::weights::SimpleDispatchInfo::default()] - fn other_dummy(origin, _n: u32) -> DispatchResult { + fn dummy(origin, _n: u32) -> DispatchResult { let _sender = ensure_none(origin)?; Ok(()) } @@ -98,15 +106,17 @@ benchmarks!{ let b in 1 .. 1000 => (); } - dummy { + set_value { let b in ...; - let caller = account("caller", 0, 0); + let caller = account::("caller", 0, 0); }: _ (RawOrigin::Signed(caller), b.into()) + verify { + assert_eq!(Value::get(), Some(b)); + } other_name { let b in ...; - let caller = account("caller", 0, 0); - }: other_dummy (RawOrigin::Signed(caller), b.into()) + }: dummy (RawOrigin::None, b.into()) sort_vector { let x in 1 .. 10000; @@ -116,24 +126,31 @@ benchmarks!{ } }: { m.sort(); + } verify { ensure!(m[0] == 0, "You forgot to sort!") } - broken_benchmark { + bad_origin { + let b in ...; + let caller = account::("caller", 0, 0); + }: dummy (RawOrigin::Signed(caller), b.into()) + + bad_verify { let x in 1 .. 10000; let mut m = Vec::::new(); for i in (0..x).rev() { m.push(i); } - }: { + }: { } + verify { ensure!(m[0] == 0, "You forgot to sort!") } } #[test] fn benchmarks_macro_works() { - // Check benchmark creation for `dummy`. - let selected_benchmark = SelectedBenchmark::dummy; + // Check benchmark creation for `set_value`. + let selected_benchmark = SelectedBenchmark::set_value; let components = >::components(&selected_benchmark); assert_eq!(components, vec![(BenchmarkParameter::b, 1, 1000)]); @@ -161,7 +178,7 @@ fn benchmarks_macro_rename_works() { ).expect("failed to create closure"); new_test_ext().execute_with(|| { - assert_eq!(closure(), Err("Bad origin")); + assert_ok!(closure()); }); } @@ -181,11 +198,27 @@ fn benchmarks_macro_works_for_non_dispatchable() { } #[test] -fn benchmarks_generate_unit_tests() { +fn benchmarks_macro_verify_works() { + // Check postcondition for benchmark `set_value` is valid. + let selected_benchmark = SelectedBenchmark::set_value; + + let closure = >::verify( + &selected_benchmark, + &[(BenchmarkParameter::b, 1)], + ).expect("failed to create closure"); + new_test_ext().execute_with(|| { - assert_ok!(test_benchmark_dummy::()); - assert_err!(test_benchmark_other_name::(), "Bad origin"); - assert_ok!(test_benchmark_sort_vector::()); - assert_err!(test_benchmark_broken_benchmark::(), "You forgot to sort!"); + assert_ok!(closure()); + }); +} + +#[test] +fn benchmarks_generate_unit_tests() { + new_test_ext().execute_with(|| { + assert_ok!(test_benchmark_set_value::()); + assert_ok!(test_benchmark_other_name::()); + assert_ok!(test_benchmark_sort_vector::()); + assert_err!(test_benchmark_bad_origin::(), "Bad origin"); + assert_err!(test_benchmark_bad_verify::(), "You forgot to sort!"); }); } diff --git a/substrate/frame/benchmarking/src/utils.rs b/substrate/frame/benchmarking/src/utils.rs index a6f262eab9..41b968fbfc 100644 --- a/substrate/frame/benchmarking/src/utils.rs +++ b/substrate/frame/benchmarking/src/utils.rs @@ -113,8 +113,11 @@ pub trait BenchmarkingSetup { /// Return the components and their ranges which should be tested in this benchmark. fn components(&self) -> Vec<(BenchmarkParameter, u32, u32)>; - /// Set up the storage, and prepare a closure to test in a single run of the benchmark. + /// Set up the storage, and prepare a closure to run the benchmark. fn instance(&self, components: &[(BenchmarkParameter, u32)]) -> Result Result<(), &'static str>>, &'static str>; + + /// Set up the storage, and prepare a closure to test and verify the benchmark + fn verify(&self, components: &[(BenchmarkParameter, u32)]) -> Result Result<(), &'static str>>, &'static str>; } /// The required setup for creating a benchmark. @@ -122,8 +125,11 @@ pub trait BenchmarkingSetupInstance { /// Return the components and their ranges which should be tested in this benchmark. fn components(&self) -> Vec<(BenchmarkParameter, u32, u32)>; - /// Set up the storage, and prepare a closure to test in a single run of the benchmark. + /// Set up the storage, and prepare a closure to run the benchmark. fn instance(&self, components: &[(BenchmarkParameter, u32)]) -> Result Result<(), &'static str>>, &'static str>; + + /// Set up the storage, and prepare a closure to test and verify the benchmark + fn verify(&self, components: &[(BenchmarkParameter, u32)]) -> Result Result<(), &'static str>>, &'static str>; } /// Grab an account, seeded by a name and index. diff --git a/substrate/frame/collective/src/benchmarking.rs b/substrate/frame/collective/src/benchmarking.rs index 51db4ee109..edef5e2e24 100644 --- a/substrate/frame/collective/src/benchmarking.rs +++ b/substrate/frame/collective/src/benchmarking.rs @@ -50,7 +50,7 @@ benchmarks_instance! { new_members.push(member); } - // Set old members. + // Set old members. // We compute the difference of old and new members, so it should influence timing. let mut old_members = vec![]; for i in 0 .. n { @@ -62,7 +62,11 @@ benchmarks_instance! { Collective::::set_members(SystemOrigin::Root.into(), old_members, prime.clone())?; - }: _(SystemOrigin::Root, new_members, prime) + }: _(SystemOrigin::Root, new_members.clone(), prime) + verify { + new_members.sort(); + assert_eq!(Collective::::members(), new_members); + } execute { let u in ...; @@ -148,7 +152,7 @@ benchmarks_instance! { let caller1: T::AccountId = account("caller1", u, SEED); let caller2: T::AccountId = account("caller2", u, SEED); - + let proposal: Box = Box::new(Call::::close(Default::default(), Default::default()).into()); let proposal_hash = T::Hashing::hash_of(&proposal); @@ -167,7 +171,7 @@ benchmarks_instance! { let caller1: T::AccountId = account("caller1", u, SEED); let caller2: T::AccountId = account("caller2", u, SEED); - + let proposal: Box = Box::new(Call::::close(Default::default(), Default::default()).into()); let proposal_hash = T::Hashing::hash_of(&proposal); @@ -183,3 +187,24 @@ benchmarks_instance! { }: _(SystemOrigin::Signed(caller2), proposal_hash, index) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::tests::{new_test_ext, Test}; + use frame_support::assert_ok; + + #[test] + fn test_benchmarks() { + new_test_ext().execute_with(|| { + assert_ok!(test_benchmark_set_members::()); + assert_ok!(test_benchmark_execute::()); + assert_ok!(test_benchmark_propose::()); + assert_ok!(test_benchmark_propose_else_branch::()); + assert_ok!(test_benchmark_vote::()); + assert_ok!(test_benchmark_vote_not_approve::()); + assert_ok!(test_benchmark_vote_approved::()); + assert_ok!(test_benchmark_close::()); + }); + } +} diff --git a/substrate/frame/collective/src/lib.rs b/substrate/frame/collective/src/lib.rs index 49c1bd3891..53e9853221 100644 --- a/substrate/frame/collective/src/lib.rs +++ b/substrate/frame/collective/src/lib.rs @@ -589,7 +589,7 @@ mod tests { } ); - fn make_ext() -> sp_io::TestExternalities { + pub fn new_test_ext() -> sp_io::TestExternalities { let mut ext: sp_io::TestExternalities = GenesisConfig { collective_Instance1: Some(collective::GenesisConfig { members: vec![1, 2, 3], @@ -603,7 +603,7 @@ mod tests { #[test] fn motions_basic_environment_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { assert_eq!(Collective::members(), vec![1, 2, 3]); assert_eq!(Collective::proposals(), Vec::::new()); }); @@ -615,7 +615,7 @@ mod tests { #[test] fn close_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); @@ -643,7 +643,7 @@ mod tests { #[test] fn close_with_prime_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); assert_ok!(Collective::set_members(Origin::ROOT, vec![1, 2, 3], Some(3))); @@ -666,7 +666,7 @@ mod tests { #[test] fn close_with_voting_prime_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); assert_ok!(Collective::set_members(Origin::ROOT, vec![1, 2, 3], Some(1))); @@ -690,7 +690,7 @@ mod tests { #[test] fn removal_of_old_voters_votes_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); let end = 4; @@ -724,7 +724,7 @@ mod tests { #[test] fn removal_of_old_voters_votes_works_with_set_members() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash = BlakeTwo256::hash_of(&proposal); let end = 4; @@ -758,7 +758,7 @@ mod tests { #[test] fn propose_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash = proposal.blake2_256().into(); let end = 4; @@ -787,7 +787,7 @@ mod tests { #[test] fn motions_ignoring_non_collective_proposals_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); assert_noop!( Collective::propose(Origin::signed(42), 3, Box::new(proposal.clone())), @@ -798,7 +798,7 @@ mod tests { #[test] fn motions_ignoring_non_collective_votes_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); @@ -811,7 +811,7 @@ mod tests { #[test] fn motions_ignoring_bad_index_collective_vote_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { System::set_block_number(3); let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); @@ -825,7 +825,7 @@ mod tests { #[test] fn motions_revoting_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); let end = 4; @@ -876,7 +876,7 @@ mod tests { #[test] fn motions_reproposing_disapproved_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); @@ -889,7 +889,7 @@ mod tests { #[test] fn motions_disapproval_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); assert_ok!(Collective::propose(Origin::signed(1), 3, Box::new(proposal.clone()))); @@ -931,7 +931,7 @@ mod tests { #[test] fn motions_approval_works() { - make_ext().execute_with(|| { + new_test_ext().execute_with(|| { let proposal = make_proposal(42); let hash: H256 = proposal.blake2_256().into(); assert_ok!(Collective::propose(Origin::signed(1), 2, Box::new(proposal.clone())));