From 24fb596537258dca3ac29d540f8c73797e9a4a2e Mon Sep 17 00:00:00 2001 From: ucover <91202993+ucover@users.noreply.github.com> Date: Tue, 28 Sep 2021 16:20:53 +0200 Subject: [PATCH 01/13] Generate one #[test] fn per bench case. --- frame/benchmarking/src/lib.rs | 280 +++++++++++++++++++++++++++++++++- 1 file changed, 273 insertions(+), 7 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 4a6c5e15ae20c..bb5ceb96f8acd 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -193,6 +193,7 @@ macro_rules! benchmarks { $( $rest:tt )* ) => { $crate::benchmarks_iter!( + { } { } { } ( ) @@ -212,6 +213,7 @@ macro_rules! benchmarks_instance { $( $rest:tt )* ) => { $crate::benchmarks_iter!( + { } { I: Instance } { } ( ) @@ -231,6 +233,7 @@ macro_rules! benchmarks_instance_pallet { $( $rest:tt )* ) => { $crate::benchmarks_iter!( + { } { I: 'static } { } ( ) @@ -244,8 +247,34 @@ macro_rules! benchmarks_instance_pallet { #[macro_export] #[doc(hidden)] macro_rules! benchmarks_iter { + // detect and extract `impl_benchmark_test_suite` call: + ( + { } + { $( $instance:ident: $instance_bound:tt )? } + { $( $where_clause:tt )* } + ( $( $names:tt )* ) + ( $( $names_extra:tt )* ) + ( $( $names_skip_meta:tt )* ) + impl_benchmark_test_suite!( + $bench_module:ident, + $new_test_ext:expr, + $test:path + $(, $( $args:tt )* )?) + $( $rest:tt )* + ) => { + $crate::benchmarks_iter! { + { $bench_module, $new_test_ext, $test $(, $( $args )* )? } + { $( $instance: $instance_bound )? } + { $( $where_clause )* } + ( $( $names )* ) + ( $( $names_extra )* ) + ( $( $names_skip_meta )* ) + $( $rest )* + } + }; // detect and extract where clause: ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -255,6 +284,7 @@ macro_rules! benchmarks_iter { $( $rest:tt )* ) => { $crate::benchmarks_iter! { + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound)? } { $( $where_bound )* } ( $( $names )* ) @@ -265,6 +295,7 @@ macro_rules! benchmarks_iter { }; // detect and extract `#[skip_meta]` tag: ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -275,6 +306,7 @@ macro_rules! benchmarks_iter { $( $rest:tt )* ) => { $crate::benchmarks_iter! { + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound )? } { $( $where_clause )* } ( $( $names )* ) @@ -284,8 +316,9 @@ macro_rules! benchmarks_iter { $( $rest )* } }; - // detect and extract `#[extra] tag: + // detect and extract `#[extra]` tag: ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -296,6 +329,7 @@ macro_rules! benchmarks_iter { $( $rest:tt )* ) => { $crate::benchmarks_iter! { + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound )? } { $( $where_clause )* } ( $( $names )* ) @@ -307,6 +341,7 @@ macro_rules! benchmarks_iter { }; // mutation arm: ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) // This contains $( $( { $instance } )? $name:ident )* @@ -317,6 +352,7 @@ macro_rules! benchmarks_iter { $( $rest:tt )* ) => { $crate::benchmarks_iter! { + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound )? } { $( $where_clause )* } ( $( $names )* ) @@ -329,6 +365,7 @@ macro_rules! benchmarks_iter { }; // mutation arm: ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -340,6 +377,7 @@ macro_rules! benchmarks_iter { ) => { $crate::paste::paste! { $crate::benchmarks_iter! { + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound )? } { $( $where_clause )* } ( $( $names )* ) @@ -373,6 +411,7 @@ macro_rules! benchmarks_iter { }; // iteration arm: ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -400,6 +439,7 @@ macro_rules! benchmarks_iter { ); $crate::benchmarks_iter!( + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound )? } { $( $where_clause )* } ( $( $names )* { $( $instance )? } $name ) @@ -408,8 +448,40 @@ macro_rules! benchmarks_iter { $( $rest )* ); }; - // iteration-exit arm + // iteration-exit arm which generates a #[test] function for each case. ( + { $bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )? } + { $( $instance:ident: $instance_bound:tt )? } + { $( $where_clause:tt )* } + ( $( $names:tt )* ) + ( $( $names_extra:tt )* ) + ( $( $names_skip_meta:tt )* ) + ) => { + $crate::selected_benchmark!( + { $( $where_clause)* } + { $( $instance: $instance_bound )? } + $( $names )* + ); + $crate::impl_benchmark!( + { $( $where_clause )* } + { $( $instance: $instance_bound )? } + ( $( $names )* ) + ( $( $names_extra ),* ) + ( $( $names_skip_meta ),* ) + ); + $crate::impl_test_function!( + ( $( $names )* ) + ( $( $names_extra )* ) + ( $( $names_skip_meta )* ) + $bench_module, + $new_test_ext, + $test + $(, $( $args )* )? + ); + }; + // iteration-exit arm which generates one #[test] function for all cases. + ( + { } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -431,6 +503,7 @@ macro_rules! benchmarks_iter { }; // add verify block to _() format ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -440,6 +513,7 @@ macro_rules! benchmarks_iter { $( $rest:tt )* ) => { $crate::benchmarks_iter! { + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound )? } { $( $where_clause )* } ( $( $names )* ) @@ -452,6 +526,7 @@ macro_rules! benchmarks_iter { }; // add verify block to name() format ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -461,6 +536,7 @@ macro_rules! benchmarks_iter { $( $rest:tt )* ) => { $crate::benchmarks_iter! { + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound )? } { $( $where_clause )* } ( $( $names )* ) @@ -473,6 +549,7 @@ macro_rules! benchmarks_iter { }; // add verify block to {} format ( + { $($bench_module:ident, $new_test_ext:expr, $test:path $(, $( $args:tt )* )?)? } { $( $instance:ident: $instance_bound:tt )? } { $( $where_clause:tt )* } ( $( $names:tt )* ) @@ -482,6 +559,7 @@ macro_rules! benchmarks_iter { $( $rest:tt )* ) => { $crate::benchmarks_iter!( + { $($bench_module, $new_test_ext, $test $(, $( $args )* )?)? } { $( $instance: $instance_bound )? } { $( $where_clause )* } ( $( $names )* ) @@ -695,6 +773,110 @@ macro_rules! benchmark_backend { }; } +// Creates #[test] functions for the given bench cases. +#[macro_export] +#[doc(hidden)] +macro_rules! impl_bench_case_tests { + ( + { $module:ident, $new_test_exec:expr, $exec_name:ident, $test:path, $extra:expr } + { $( $names_extra:tt )* } + $( { $( $bench_inst:ident )? } $bench:ident )* + ) + => { + $crate::impl_bench_name_tests!( + $module, $new_test_exec, $exec_name, $test, $extra, + { $( $names_extra )* }, + $( { $bench } )+ + ); + } +} + +// Creates a #[test] function for the given bench name. +#[macro_export] +#[doc(hidden)] +macro_rules! impl_bench_name_tests { + // recursion anchor + ( + $module:ident, $new_test_exec:expr, $exec_name:ident, $test:path, $extra:expr, + { $( $names_extra:tt )* }, + { $name:ident } + ) => { + $crate::paste::paste! { + #[test] + fn [] () { + $new_test_exec.$exec_name(|| { + // Skip all #[extra] benchmarks if $extra is false. + if !($extra) { + let disabled = $crate::vec![ $( stringify!($names_extra).as_ref() ),* ]; + if disabled.contains(&stringify!($name)) { + $crate::log::error!( + "INFO: extra benchmark skipped - {}", + stringify!($name), + ); + return (); + } + } + + // Same per-case logic as when all cases are run in the + // same function. + match std::panic::catch_unwind(|| { + $module::<$test>::[< test_benchmark_ $name >] () + }) { + Err(err) => { + println!( + "{}: {:?}", + stringify!($name), + err, + ); + assert!(false); + }, + Ok(Err(err)) => { + match err { + $crate::BenchmarkError::Stop(err) => { + println!( + "{}: {:?}", + stringify!($name), + err, + ); + assert!(false); + }, + $crate::BenchmarkError::Override(_) => { + // This is still considered a success condition. + $crate::log::error!( + "WARNING: benchmark error overrided - {}", + stringify!($name), + ); + }, + $crate::BenchmarkError::Skip => { + // This is considered a success condition. + $crate::log::error!( + "WARNING: benchmark error skipped - {}", + stringify!($name), + ); + } + } + }, + Ok(Ok(())) => (), + } + }); + } + } + }; + // recursion tail + ( + $module:ident, $new_test_exec:expr, $exec_name:ident, $test:path, $extra:expr, + { $( $names_extra:tt )* }, + { $name:ident } $( { $rest:ident } )+ + ) => { + // car + $crate::impl_bench_name_tests!($module, $new_test_exec, $exec_name, $test, $extra, + { $( $names_extra )* }, { $name }); + // cdr + $crate::impl_bench_name_tests!($module, $new_test_exec, $exec_name, $test, $extra, + { $( $names_extra )* }, $( { $rest } )+); + }; +} + // Creates a `SelectedBenchmark` enum implementing `BenchmarkingSetup`. // // Every variant must implement [`BenchmarkingSetup`]. @@ -1109,16 +1291,50 @@ macro_rules! impl_benchmark_test { // just iterate over the `Benchmarking::benchmarks` list to run the actual implementations. #[macro_export] macro_rules! impl_benchmark_test_suite { + ( + $bench_module:ident, + $new_test_ext:expr, + $test:path + $(, $( $rest:tt )* )? + ) => { + $crate::impl_test_function!( + () + () + () + $bench_module, + $new_test_ext, + $test + $(, $( $rest )* )? + ); + } +} + +// Takes all arguments from `impl_benchmark_test_suite` and three additional arguments. +// +// Can be configured to generate one #[test] fn per bench case or +// one #[test] fn for all bench cases. +// This depends on whether or not the first argument contains a non-empty list of bench names. +#[macro_export] +#[doc(hidden)] +macro_rules! impl_test_function { // user might or might not have set some keyword arguments; set the defaults // // The weird syntax indicates that `rest` comes only after a comma, which is otherwise optional ( + ( $( $names:tt )* ) + ( $( $names_extra:tt )* ) + ( $( $names_skip_meta:tt )* ) + $bench_module:ident, $new_test_ext:expr, $test:path $(, $( $rest:tt )* )? ) => { - $crate::impl_benchmark_test_suite!( + $crate::impl_test_function!( + @cases: + ( $( $names )* ) + ( $( $names_extra )* ) + ( $( $names_skip_meta )* ) @selected: $bench_module, $new_test_ext, @@ -1132,6 +1348,10 @@ macro_rules! impl_benchmark_test_suite { }; // pick off the benchmarks_path keyword argument ( + @cases: + ( $( $names:tt )* ) + ( $( $names_extra:tt )* ) + ( $( $names_skip_meta:tt )* ) @selected: $bench_module:ident, $new_test_ext:expr, @@ -1143,7 +1363,11 @@ macro_rules! impl_benchmark_test_suite { benchmarks_path = $benchmarks_path:ident $(, $( $rest:tt )* )? ) => { - $crate::impl_benchmark_test_suite!( + $crate::impl_test_function!( + @cases: + ( $( $names )* ) + ( $( $names_extra )* ) + ( $( $names_skip_meta )* ) @selected: $bench_module, $new_test_ext, @@ -1157,6 +1381,10 @@ macro_rules! impl_benchmark_test_suite { }; // pick off the extra keyword argument ( + @cases: + ( $( $names:tt )* ) + ( $( $names_extra:tt )* ) + ( $( $names_skip_meta:tt )* ) @selected: $bench_module:ident, $new_test_ext:expr, @@ -1168,7 +1396,11 @@ macro_rules! impl_benchmark_test_suite { extra = $extra:expr $(, $( $rest:tt )* )? ) => { - $crate::impl_benchmark_test_suite!( + $crate::impl_test_function!( + @cases: + ( $( $names )* ) + ( $( $names_extra )* ) + ( $( $names_skip_meta )* ) @selected: $bench_module, $new_test_ext, @@ -1182,6 +1414,10 @@ macro_rules! impl_benchmark_test_suite { }; // pick off the exec_name keyword argument ( + @cases: + ( $( $names:tt )* ) + ( $( $names_extra:tt )* ) + ( $( $names_skip_meta:tt )* ) @selected: $bench_module:ident, $new_test_ext:expr, @@ -1193,7 +1429,11 @@ macro_rules! impl_benchmark_test_suite { exec_name = $exec_name:ident $(, $( $rest:tt )* )? ) => { - $crate::impl_benchmark_test_suite!( + $crate::impl_test_function!( + @cases: + ( $( $names )* ) + ( $( $names_extra )* ) + ( $( $names_skip_meta )* ) @selected: $bench_module, $new_test_ext, @@ -1205,8 +1445,34 @@ macro_rules! impl_benchmark_test_suite { $( $( $rest )* )? ); }; - // all options set; nothing else in user-provided keyword arguments + // iteration-exit arm which generates a #[test] function for each case. + ( + @cases: + ( $( $names:tt )+ ) + ( $( $names_extra:tt )* ) + ( $( $names_skip_meta:tt )* ) + @selected: + $bench_module:ident, + $new_test_ext:expr, + $test:path, + benchmarks_path = $path_to_benchmarks_invocation:ident, + extra = $extra:expr, + exec_name = $exec_name:ident, + @user: + $(,)? + ) => { + $crate::impl_bench_case_tests!( + { $bench_module, $new_test_ext, $exec_name, $test, $extra } + { $( $names_extra:tt )* } + $($names)+ + ); + }; + // iteration-exit arm which generates one #[test] function for all cases. ( + @cases: + () + () + () @selected: $bench_module:ident, $new_test_ext:expr, From 6ea45571bde480bfeabe76b81ec052be3ac9fada Mon Sep 17 00:00:00 2001 From: ucover <91202993+ucover@users.noreply.github.com> Date: Tue, 28 Sep 2021 16:23:41 +0200 Subject: [PATCH 02/13] Update benchmark macro syntax in frame pallets. --- frame/assets/src/benchmarking.rs | 7 +++---- frame/babe/src/benchmarking.rs | 12 ++++++------ frame/bags-list/src/benchmarks.rs | 13 ++++++------- frame/balances/src/benchmarking.rs | 16 +++++++--------- frame/benchmarking/src/tests_instance.rs | 12 ++++++------ frame/bounties/src/benchmarking.rs | 6 +++--- frame/contracts/src/benchmarking/mod.rs | 14 +++++++------- 7 files changed, 38 insertions(+), 42 deletions(-) diff --git a/frame/assets/src/benchmarking.rs b/frame/assets/src/benchmarking.rs index 43eadffbe8497..d9de9ed3dedd4 100644 --- a/frame/assets/src/benchmarking.rs +++ b/frame/assets/src/benchmarking.rs @@ -21,8 +21,7 @@ use super::*; use frame_benchmarking::{ - account, benchmarks_instance_pallet, impl_benchmark_test_suite, whitelist_account, - whitelisted_caller, + account, benchmarks_instance_pallet, whitelist_account, whitelisted_caller, }; use frame_support::{ dispatch::UnfilteredDispatchable, @@ -438,6 +437,6 @@ benchmarks_instance_pallet! { verify { assert_last_event::(Event::ApprovalCancelled(id, caller, delegate).into()); } -} -impl_benchmark_test_suite!(Assets, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Assets, crate::mock::new_test_ext(), crate::mock::Test) +} diff --git a/frame/babe/src/benchmarking.rs b/frame/babe/src/benchmarking.rs index 372dfa532a894..7747c9bd1fc8c 100644 --- a/frame/babe/src/benchmarking.rs +++ b/frame/babe/src/benchmarking.rs @@ -63,6 +63,12 @@ benchmarks! { } verify { assert!(sp_consensus_babe::check_equivocation_proof::
(equivocation_proof2)); } + + impl_benchmark_test_suite!( + Pallet, + crate::mock::new_test_ext(3), + crate::mock::Test, + ) } #[cfg(test)] @@ -70,12 +76,6 @@ mod tests { use super::*; use crate::mock::*; - frame_benchmarking::impl_benchmark_test_suite!( - Pallet, - crate::mock::new_test_ext(3), - crate::mock::Test, - ); - #[test] fn test_generate_equivocation_report_blob() { let (pairs, mut ext) = new_test_ext_with_pairs(3); diff --git a/frame/bags-list/src/benchmarks.rs b/frame/bags-list/src/benchmarks.rs index a820eeba13b12..d86adc674c44a 100644 --- a/frame/bags-list/src/benchmarks.rs +++ b/frame/bags-list/src/benchmarks.rs @@ -134,11 +134,10 @@ frame_benchmarking::benchmarks! { ] ); } -} -use frame_benchmarking::impl_benchmark_test_suite; -impl_benchmark_test_suite!( - Pallet, - crate::mock::ExtBuilder::default().build(), - crate::mock::Runtime, -); + impl_benchmark_test_suite!( + Pallet, + crate::mock::ExtBuilder::default().build(), + crate::mock::Runtime, + ) +} diff --git a/frame/balances/src/benchmarking.rs b/frame/balances/src/benchmarking.rs index 06d202ea37002..1c48820094187 100644 --- a/frame/balances/src/benchmarking.rs +++ b/frame/balances/src/benchmarking.rs @@ -21,9 +21,7 @@ use super::*; -use frame_benchmarking::{ - account, benchmarks_instance_pallet, impl_benchmark_test_suite, whitelisted_caller, -}; +use frame_benchmarking::{account, benchmarks_instance_pallet, whitelisted_caller}; use frame_system::RawOrigin; use sp_runtime::traits::Bounded; @@ -215,10 +213,10 @@ benchmarks_instance_pallet! { assert!(Balances::::reserved_balance(&user).is_zero()); assert_eq!(Balances::::free_balance(&user), balance); } -} -impl_benchmark_test_suite!( - Balances, - crate::tests_composite::ExtBuilder::default().build(), - crate::tests_composite::Test, -); + impl_benchmark_test_suite!( + Balances, + crate::tests_composite::ExtBuilder::default().build(), + crate::tests_composite::Test, + ) +} diff --git a/frame/benchmarking/src/tests_instance.rs b/frame/benchmarking/src/tests_instance.rs index caccebd39c70b..0ad156ce5a88d 100644 --- a/frame/benchmarking/src/tests_instance.rs +++ b/frame/benchmarking/src/tests_instance.rs @@ -173,11 +173,11 @@ mod benchmarks { } verify { ensure!(m[0] == 0, "You forgot to sort!") } - } - crate::impl_benchmark_test_suite!( - Pallet, - crate::tests_instance::new_test_ext(), - crate::tests_instance::Test - ); + impl_benchmark_test_suite!( + Pallet, + crate::tests_instance::new_test_ext(), + crate::tests_instance::Test + ) + } } diff --git a/frame/bounties/src/benchmarking.rs b/frame/bounties/src/benchmarking.rs index 1aa1eabdb5177..33af02fbb9ea0 100644 --- a/frame/bounties/src/benchmarking.rs +++ b/frame/bounties/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_system::RawOrigin; use sp_runtime::traits::Bounded; @@ -209,6 +209,6 @@ benchmarks! { ensure!(missed_any == false, "Missed some"); assert_last_event::(Event::BountyBecameActive(b - 1).into()) } -} -impl_benchmark_test_suite!(Bounties, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Bounties, crate::tests::new_test_ext(), crate::tests::Test) +} diff --git a/frame/contracts/src/benchmarking/mod.rs b/frame/contracts/src/benchmarking/mod.rs index 981af218ea5a2..e382e616f27fd 100644 --- a/frame/contracts/src/benchmarking/mod.rs +++ b/frame/contracts/src/benchmarking/mod.rs @@ -36,7 +36,7 @@ use crate::{ Pallet as Contracts, *, }; use codec::Encode; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_support::weights::Weight; use frame_system::RawOrigin; use pwasm_utils::parity_wasm::elements::{BlockType, BrTableData, Instruction, ValueType}; @@ -2325,10 +2325,10 @@ benchmarks! { ) .result?; } -} -impl_benchmark_test_suite!( - Contracts, - crate::tests::ExtBuilder::default().build(), - crate::tests::Test, -); + impl_benchmark_test_suite!( + Contracts, + crate::tests::ExtBuilder::default().build(), + crate::tests::Test, + ) +} From 6a38664d2f2480c13cfe9047c2d88c659d089041 Mon Sep 17 00:00:00 2001 From: ucover <91202993+ucover@users.noreply.github.com> Date: Tue, 28 Sep 2021 16:27:45 +0200 Subject: [PATCH 03/13] Explain new benchmark macro syntax in example pallet. --- frame/example/src/benchmarking.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/frame/example/src/benchmarking.rs b/frame/example/src/benchmarking.rs index cdf6c152a4880..9f2bb20fe63ac 100644 --- a/frame/example/src/benchmarking.rs +++ b/frame/example/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use crate::*; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{benchmarks, whitelisted_caller}; use frame_system::RawOrigin; // To actually run this benchmark on pallet-example, we need to put this pallet into the @@ -65,12 +65,14 @@ benchmarks! { // The benchmark execution phase could also be a closure with custom code m.sort(); } -} -// This line generates test cases for benchmarking, and could be run by: -// `cargo test -p pallet-example --all-features`, you will see an additional line of: -// `test benchmarking::benchmark_tests::test_benchmarks ... ok` in the result. -// -// The line generates three steps per benchmark, with repeat=1 and the three steps are -// [low, mid, high] of the range. -impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test); + // This line generates test cases for benchmarking, and could be run by: + // `cargo test -p pallet-example --all-features`, you will see one line per case: + // `test benchmarking::bench_sort_vector ... ok` + // `test benchmarking::bench_accumulate_dummy ... ok` + // `test benchmarking::bench_set_dummy_benchmark ... ok` in the result. + // + // The line generates three steps per benchmark, with repeat=1 and the three steps are + // [low, mid, high] of the range. + impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test) +} From 767de62a4f915bf790d895f5fdae56f91801305e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 28 Sep 2021 18:23:02 -0400 Subject: [PATCH 04/13] support with and without a semicolon --- frame/benchmarking/src/lib.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index bb5ceb96f8acd..4f5b917cfc341 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -248,6 +248,32 @@ macro_rules! benchmarks_instance_pallet { #[doc(hidden)] macro_rules! benchmarks_iter { // detect and extract `impl_benchmark_test_suite` call: + // - with a semi-colon + ( + { } + { $( $instance:ident: $instance_bound:tt )? } + { $( $where_clause:tt )* } + ( $( $names:tt )* ) + ( $( $names_extra:tt )* ) + ( $( $names_skip_meta:tt )* ) + impl_benchmark_test_suite!( + $bench_module:ident, + $new_test_ext:expr, + $test:path + $(, $( $args:tt )* )?); + $( $rest:tt )* + ) => { + $crate::benchmarks_iter! { + { $bench_module, $new_test_ext, $test $(, $( $args )* )? } + { $( $instance: $instance_bound )? } + { $( $where_clause )* } + ( $( $names )* ) + ( $( $names_extra )* ) + ( $( $names_skip_meta )* ) + $( $rest )* + } + }; + // - without a semicolon ( { } { $( $instance:ident: $instance_bound:tt )? } From 0f2ef551c85d8bb11c9568b6bfd81ebb20bdf99e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 28 Sep 2021 19:19:58 -0400 Subject: [PATCH 05/13] update pallets to use individual tests --- .../pallets/template/src/benchmarking.rs | 6 +++--- frame/collective/src/benchmarking.rs | 8 +++----- frame/democracy/src/benchmarking.rs | 9 +++++++-- frame/elections-phragmen/src/benchmarking.rs | 18 ++++++++---------- frame/gilt/src/benchmarking.rs | 6 +++--- frame/identity/src/benchmarking.rs | 5 ++--- frame/indices/src/benchmarking.rs | 6 +++--- frame/lottery/src/benchmarking.rs | 6 +++--- frame/membership/src/lib.rs | 8 +++----- .../merkle-mountain-range/src/benchmarking.rs | 4 ++-- frame/offences/benchmarking/src/lib.rs | 6 +++--- frame/proxy/Cargo.toml | 1 + frame/proxy/src/benchmarking.rs | 6 +++--- frame/scheduler/src/benchmarking.rs | 6 +++--- frame/staking/src/benchmarking.rs | 1 + frame/system/benchmarking/src/lib.rs | 6 +++--- frame/timestamp/src/benchmarking.rs | 6 +++--- frame/tips/src/benchmarking.rs | 6 +++--- frame/transaction-storage/src/benchmarking.rs | 6 +++--- frame/uniques/src/benchmarking.rs | 7 +++---- frame/utility/src/benchmarking.rs | 6 +++--- frame/vesting/src/benchmarking.rs | 14 +++++++------- 22 files changed, 73 insertions(+), 74 deletions(-) diff --git a/bin/node-template/pallets/template/src/benchmarking.rs b/bin/node-template/pallets/template/src/benchmarking.rs index 2117c048cfbdb..d496a9fc89b1a 100644 --- a/bin/node-template/pallets/template/src/benchmarking.rs +++ b/bin/node-template/pallets/template/src/benchmarking.rs @@ -4,7 +4,7 @@ use super::*; #[allow(unused)] use crate::Pallet as Template; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{benchmarks, whitelisted_caller}; use frame_system::RawOrigin; benchmarks! { @@ -15,6 +15,6 @@ benchmarks! { verify { assert_eq!(Something::::get(), Some(s)); } -} -impl_benchmark_test_suite!(Template, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Template, crate::mock::new_test_ext(), crate::mock::Test); +} diff --git a/frame/collective/src/benchmarking.rs b/frame/collective/src/benchmarking.rs index c7e695babf27d..c26a2b43f5b75 100644 --- a/frame/collective/src/benchmarking.rs +++ b/frame/collective/src/benchmarking.rs @@ -23,9 +23,7 @@ use crate::Pallet as Collective; use sp_runtime::traits::Bounded; use sp_std::mem::size_of; -use frame_benchmarking::{ - account, benchmarks_instance_pallet, impl_benchmark_test_suite, whitelisted_caller, -}; +use frame_benchmarking::{account, benchmarks_instance_pallet, whitelisted_caller}; use frame_system::{Call as SystemCall, Pallet as System, RawOrigin as SystemOrigin}; const SEED: u32 = 0; @@ -638,6 +636,6 @@ benchmarks_instance_pallet! { assert_eq!(Collective::::proposals().len(), (p - 1) as usize); assert_last_event::(Event::Disapproved(last_hash).into()); } -} -impl_benchmark_test_suite!(Collective, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Collective, crate::tests::new_test_ext(), crate::tests::Test); +} diff --git a/frame/democracy/src/benchmarking.rs b/frame/democracy/src/benchmarking.rs index a00e6f4686fd3..f346ce01a3eaa 100644 --- a/frame/democracy/src/benchmarking.rs +++ b/frame/democracy/src/benchmarking.rs @@ -19,7 +19,7 @@ use super::*; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelist_account}; +use frame_benchmarking::{account, benchmarks, whitelist_account}; use frame_support::{ assert_noop, assert_ok, codec::Decode, @@ -772,4 +772,9 @@ benchmarks! { } } -impl_benchmark_test_suite!(Democracy, crate::tests::new_test_ext(), crate::tests::Test); +// TODO: individual tests +frame_benchmarking::impl_benchmark_test_suite!( + Democracy, + crate::tests::new_test_ext(), + crate::tests::Test +); diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 6e3ce0234c4fb..9bc63848607ab 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -21,9 +21,7 @@ use super::*; -use frame_benchmarking::{ - account, benchmarks, impl_benchmark_test_suite, whitelist, BenchmarkError, BenchmarkResult, -}; +use frame_benchmarking::{account, benchmarks, whitelist, BenchmarkError, BenchmarkResult}; use frame_support::{ dispatch::{DispatchResultWithPostInfo, UnfilteredDispatchable}, traits::OnInitialize, @@ -549,11 +547,11 @@ benchmarks! { MEMBERS.with(|m| *m.borrow_mut() = vec![]); } } -} -impl_benchmark_test_suite!( - Elections, - crate::tests::ExtBuilder::default().desired_members(13).desired_runners_up(7), - crate::tests::Test, - exec_name = build_and_execute, -); + impl_benchmark_test_suite!( + Elections, + crate::tests::ExtBuilder::default().desired_members(13).desired_runners_up(7), + crate::tests::Test, + exec_name = build_and_execute, + ); +} diff --git a/frame/gilt/src/benchmarking.rs b/frame/gilt/src/benchmarking.rs index cfc503cf897b4..9c6d22a48398d 100644 --- a/frame/gilt/src/benchmarking.rs +++ b/frame/gilt/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use super::*; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{benchmarks, whitelisted_caller}; use frame_support::{ dispatch::UnfilteredDispatchable, traits::{Currency, EnsureOrigin, Get}, @@ -126,6 +126,6 @@ benchmarks! { .dispatch_bypass_filter(T::AdminOrigin::successful_origin())?; }: { Gilt::::pursue_target(q) } -} -impl_benchmark_test_suite!(Gilt, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Gilt, crate::mock::new_test_ext(), crate::mock::Test); +} diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 8bda24ddc73e1..68869a43992f9 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -22,7 +22,7 @@ use super::*; use crate::Pallet as Identity; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_support::{ensure, traits::Get}; use frame_system::RawOrigin; use sp_runtime::traits::Bounded; @@ -411,6 +411,5 @@ benchmarks! { ensure!(!SuperOf::::contains_key(&caller), "Sub not removed"); } + impl_benchmark_test_suite!(Identity, crate::tests::new_test_ext(), crate::tests::Test); } - -impl_benchmark_test_suite!(Identity, crate::tests::new_test_ext(), crate::tests::Test); diff --git a/frame/indices/src/benchmarking.rs b/frame/indices/src/benchmarking.rs index ba0152008c41e..873dc18b20265 100644 --- a/frame/indices/src/benchmarking.rs +++ b/frame/indices/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use super::*; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_system::RawOrigin; use sp_runtime::traits::Bounded; @@ -91,6 +91,6 @@ benchmarks! { } // TODO in another PR: lookup and unlookup trait weights (not critical) -} -impl_benchmark_test_suite!(Indices, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Indices, crate::mock::new_test_ext(), crate::mock::Test); +} diff --git a/frame/lottery/src/benchmarking.rs b/frame/lottery/src/benchmarking.rs index 7af20bbb0e11f..5407e16cd633f 100644 --- a/frame/lottery/src/benchmarking.rs +++ b/frame/lottery/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_support::traits::{EnsureOrigin, OnInitialize}; use frame_system::RawOrigin; use sp_runtime::traits::{Bounded, Zero}; @@ -163,6 +163,6 @@ benchmarks! { assert_eq!(Lottery::::pot().1, 0u32.into()); assert!(!T::Currency::free_balance(&winner).is_zero()) } -} -impl_benchmark_test_suite!(Lottery, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Lottery, crate::mock::new_test_ext(), crate::mock::Test); +} diff --git a/frame/membership/src/lib.rs b/frame/membership/src/lib.rs index 57a12c7c8a453..8fa2abb0ad3f3 100644 --- a/frame/membership/src/lib.rs +++ b/frame/membership/src/lib.rs @@ -357,9 +357,7 @@ impl, I: 'static> SortedMembers for Pallet { #[cfg(feature = "runtime-benchmarks")] mod benchmark { use super::{Pallet as Membership, *}; - use frame_benchmarking::{ - account, benchmarks_instance_pallet, impl_benchmark_test_suite, whitelist, - }; + use frame_benchmarking::{account, benchmarks_instance_pallet, whitelist}; use frame_support::{assert_ok, traits::EnsureOrigin}; use frame_system::RawOrigin; @@ -494,9 +492,9 @@ mod benchmark { assert!(::get_prime().is_none()); #[cfg(test)] crate::tests::clean(); } - } - impl_benchmark_test_suite!(Membership, crate::tests::new_bench_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Membership, crate::tests::new_bench_ext(), crate::tests::Test); + } } #[cfg(test)] diff --git a/frame/merkle-mountain-range/src/benchmarking.rs b/frame/merkle-mountain-range/src/benchmarking.rs index c269afb75855c..f164f70fca6cd 100644 --- a/frame/merkle-mountain-range/src/benchmarking.rs +++ b/frame/merkle-mountain-range/src/benchmarking.rs @@ -33,6 +33,6 @@ benchmarks_instance_pallet! { } verify { assert_eq!(crate::NumberOfLeaves::::get(), leaves); } -} -impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::mock::Test); +} diff --git a/frame/offences/benchmarking/src/lib.rs b/frame/offences/benchmarking/src/lib.rs index 35e3c1aec9403..dde8aa92c2405 100644 --- a/frame/offences/benchmarking/src/lib.rs +++ b/frame/offences/benchmarking/src/lib.rs @@ -23,7 +23,7 @@ mod mock; use sp_std::{prelude::*, vec}; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite}; +use frame_benchmarking::{account, benchmarks}; use frame_support::traits::{Currency, ValidatorSet, ValidatorSetWithIdentification}; use frame_system::{Config as SystemConfig, Pallet as System, RawOrigin}; @@ -399,6 +399,6 @@ benchmarks! { + n // nominators slashed ); } -} -impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test); +} diff --git a/frame/proxy/Cargo.toml b/frame/proxy/Cargo.toml index 83db82990d105..4da712dadf27b 100644 --- a/frame/proxy/Cargo.toml +++ b/frame/proxy/Cargo.toml @@ -42,5 +42,6 @@ std = [ runtime-benchmarks = [ "frame-benchmarking", "frame-support/runtime-benchmarks", + "frame-system/runtime-benchmarks", ] try-runtime = ["frame-support/try-runtime"] diff --git a/frame/proxy/src/benchmarking.rs b/frame/proxy/src/benchmarking.rs index e66f6782c19e1..1eb3ec5770544 100644 --- a/frame/proxy/src/benchmarking.rs +++ b/frame/proxy/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; use crate::Pallet as Proxy; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_system::RawOrigin; use sp_runtime::traits::Bounded; @@ -245,6 +245,6 @@ benchmarks! { verify { assert!(!Proxies::::contains_key(&anon)); } -} -impl_benchmark_test_suite!(Proxy, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Proxy, crate::tests::new_test_ext(), crate::tests::Test); +} diff --git a/frame/scheduler/src/benchmarking.rs b/frame/scheduler/src/benchmarking.rs index 2c164eaede229..1065f17027744 100644 --- a/frame/scheduler/src/benchmarking.rs +++ b/frame/scheduler/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use super::*; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite}; +use frame_benchmarking::benchmarks; use frame_support::{ensure, traits::OnInitialize}; use frame_system::RawOrigin; use sp_std::{prelude::*, vec}; @@ -139,6 +139,6 @@ benchmarks! { "didn't append schedule" ); } -} -impl_benchmark_test_suite!(Scheduler, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Scheduler, crate::tests::new_test_ext(), crate::tests::Test); +} diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index fe60d516e144c..8b357538dda31 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -1002,6 +1002,7 @@ mod tests { } } +// TODO: individual tests impl_benchmark_test_suite!( Staking, crate::mock::ExtBuilder::default().has_stakers(true), diff --git a/frame/system/benchmarking/src/lib.rs b/frame/system/benchmarking/src/lib.rs index beb61829bce37..e7371b1099e5e 100644 --- a/frame/system/benchmarking/src/lib.rs +++ b/frame/system/benchmarking/src/lib.rs @@ -20,7 +20,7 @@ #![cfg_attr(not(feature = "std"), no_std)] use codec::Encode; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{benchmarks, whitelisted_caller}; use frame_support::{storage, traits::Get, weights::DispatchClass}; use frame_system::{Call, DigestItemOf, Pallet as System, RawOrigin}; use sp_core::{storage::well_known_keys, ChangesTrieConfiguration}; @@ -140,6 +140,6 @@ benchmarks! { verify { assert_eq!(storage::unhashed::get_raw(&last_key), None); } -} -impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test); +} diff --git a/frame/timestamp/src/benchmarking.rs b/frame/timestamp/src/benchmarking.rs index 97ddd4cddd63f..98e05439df72b 100644 --- a/frame/timestamp/src/benchmarking.rs +++ b/frame/timestamp/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use super::*; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite, TrackedStorageKey}; +use frame_benchmarking::{benchmarks, TrackedStorageKey}; use frame_support::{ensure, traits::OnFinalize}; use frame_system::RawOrigin; @@ -55,6 +55,6 @@ benchmarks! { verify { ensure!(!DidUpdate::::exists(), "Time was not removed."); } -} -impl_benchmark_test_suite!(Timestamp, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Timestamp, crate::tests::new_test_ext(), crate::tests::Test); +} diff --git a/frame/tips/src/benchmarking.rs b/frame/tips/src/benchmarking.rs index 5e08121855210..d8227332bb334 100644 --- a/frame/tips/src/benchmarking.rs +++ b/frame/tips/src/benchmarking.rs @@ -19,7 +19,7 @@ #![cfg(feature = "runtime-benchmarks")] -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_support::ensure; use frame_system::RawOrigin; use sp_runtime::traits::Saturating; @@ -190,6 +190,6 @@ benchmarks! { let hash = T::Hashing::hash_of(&(&reason_hash, &beneficiary)); ensure!(Tips::::contains_key(hash), "tip does not exist"); }: _(RawOrigin::Root, hash) -} -impl_benchmark_test_suite!(TipsMod, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(TipsMod, crate::tests::new_test_ext(), crate::tests::Test); +} diff --git a/frame/transaction-storage/src/benchmarking.rs b/frame/transaction-storage/src/benchmarking.rs index d5da6a42b46f0..6ca9b247f0228 100644 --- a/frame/transaction-storage/src/benchmarking.rs +++ b/frame/transaction-storage/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use super::*; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{benchmarks, whitelisted_caller}; use frame_support::traits::{Currency, OnFinalize, OnInitialize}; use frame_system::{EventRecord, Pallet as System, RawOrigin}; use sp_runtime::traits::{Bounded, One, Zero}; @@ -143,6 +143,6 @@ benchmarks! { verify { assert_last_event::(Event::ProofChecked.into()); } -} -impl_benchmark_test_suite!(TransactionStorage, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(TransactionStorage, crate::mock::new_test_ext(), crate::mock::Test); +} diff --git a/frame/uniques/src/benchmarking.rs b/frame/uniques/src/benchmarking.rs index 5c777dc961e9e..0e161bf7bfe85 100644 --- a/frame/uniques/src/benchmarking.rs +++ b/frame/uniques/src/benchmarking.rs @@ -21,8 +21,7 @@ use super::*; use frame_benchmarking::{ - account, benchmarks_instance_pallet, impl_benchmark_test_suite, whitelist_account, - whitelisted_caller, + account, benchmarks_instance_pallet, whitelist_account, whitelisted_caller, }; use frame_support::{ dispatch::UnfilteredDispatchable, @@ -379,6 +378,6 @@ benchmarks_instance_pallet! { verify { assert_last_event::(Event::ApprovalCancelled(class, instance, caller, delegate).into()); } -} -impl_benchmark_test_suite!(Uniques, crate::mock::new_test_ext(), crate::mock::Test); + impl_benchmark_test_suite!(Uniques, crate::mock::new_test_ext(), crate::mock::Test); +} diff --git a/frame/utility/src/benchmarking.rs b/frame/utility/src/benchmarking.rs index 210a6156499cf..70cc61f87b9c9 100644 --- a/frame/utility/src/benchmarking.rs +++ b/frame/utility/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use super::*; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_system::RawOrigin; const SEED: u32 = 0; @@ -63,6 +63,6 @@ benchmarks! { verify { assert_last_event::(Event::BatchCompleted.into()) } -} -impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test); +} diff --git a/frame/vesting/src/benchmarking.rs b/frame/vesting/src/benchmarking.rs index 5cdc14c8fdaca..b52ddac3e8857 100644 --- a/frame/vesting/src/benchmarking.rs +++ b/frame/vesting/src/benchmarking.rs @@ -19,7 +19,7 @@ #![cfg(feature = "runtime-benchmarks")] -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller}; use frame_support::assert_ok; use frame_system::{Pallet as System, RawOrigin}; use sp_runtime::traits::{Bounded, CheckedDiv, CheckedMul}; @@ -374,10 +374,10 @@ benchmarks! { T::Currency::transfer(&caller, &test_dest, expected_balance, ExistenceRequirement::AllowDeath) ); } -} -impl_benchmark_test_suite!( - Vesting, - crate::mock::ExtBuilder::default().existential_deposit(256).build(), - crate::mock::Test, -); + impl_benchmark_test_suite!( + Vesting, + crate::mock::ExtBuilder::default().existential_deposit(256).build(), + crate::mock::Test, + ); +} From a42edd1ed20ff18d532549b8b75a070de4dd0a5e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 28 Sep 2021 19:45:28 -0400 Subject: [PATCH 06/13] migrate staking too --- frame/staking/src/benchmarking.rs | 15 +++++++-------- frame/staking/src/lib.rs | 1 + 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 8b357538dda31..220e8f1e6a24c 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -887,6 +887,13 @@ benchmarks! { verify { assert!(!T::SortedListProvider::contains(&stash)); } + + impl_benchmark_test_suite!( + Staking, + crate::mock::ExtBuilder::default().has_stakers(true), + crate::mock::Test, + exec_name = build_and_execute + ); } #[cfg(test)] @@ -1001,11 +1008,3 @@ mod tests { }); } } - -// TODO: individual tests -impl_benchmark_test_suite!( - Staking, - crate::mock::ExtBuilder::default().has_stakers(true), - crate::mock::Test, - exec_name = build_and_execute -); diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index d8e72e267ea9a..582e9e49bd356 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -272,6 +272,7 @@ //! validators is stored in the Session pallet's `Validators` at the end of each era. #![cfg_attr(not(feature = "std"), no_std)] +#![recursion_limit = "256"] #[cfg(feature = "runtime-benchmarks")] pub mod benchmarking; From d847173240c881e65829b50ab97166b68380a91e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 28 Sep 2021 19:59:04 -0400 Subject: [PATCH 07/13] migrate more pallets --- .../src/benchmarking.rs | 14 +++++++------- frame/grandpa/src/benchmarking.rs | 12 ++++++------ frame/im-online/src/benchmarking.rs | 6 +++--- frame/merkle-mountain-range/src/benchmarking.rs | 2 +- frame/multisig/src/benchmarking.rs | 6 +++--- frame/session/benchmarking/src/lib.rs | 6 +++--- frame/treasury/src/benchmarking.rs | 6 +++--- 7 files changed, 26 insertions(+), 26 deletions(-) diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index b8d7bc45c4487..9648b8e0f2465 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -19,7 +19,7 @@ use super::*; use crate::{unsigned::IndexAssignmentOf, Pallet as MultiPhase}; -use frame_benchmarking::{account, impl_benchmark_test_suite}; +use frame_benchmarking::account; use frame_support::{assert_ok, traits::Hooks}; use frame_system::RawOrigin; use rand::{prelude::SliceRandom, rngs::SmallRng, SeedableRng}; @@ -497,10 +497,10 @@ frame_benchmarking::benchmarks! { log!(trace, "actual encoded size = {}", encoding.len()); assert!(encoding.len() <= desired_size); } -} -impl_benchmark_test_suite!( - MultiPhase, - crate::mock::ExtBuilder::default().build_offchainify(10).0, - crate::mock::Runtime, -); + impl_benchmark_test_suite!( + MultiPhase, + crate::mock::ExtBuilder::default().build_offchainify(10).0, + crate::mock::Runtime, + ); +} diff --git a/frame/grandpa/src/benchmarking.rs b/frame/grandpa/src/benchmarking.rs index 815a18d13531e..1e6be01ce8dbf 100644 --- a/frame/grandpa/src/benchmarking.rs +++ b/frame/grandpa/src/benchmarking.rs @@ -68,6 +68,12 @@ benchmarks! { verify { assert!(Grandpa::::stalled().is_some()); } + + impl_benchmark_test_suite!( + Pallet, + crate::mock::new_test_ext(vec![(1, 1), (2, 1), (3, 1)]), + crate::mock::Test, + ); } #[cfg(test)] @@ -75,12 +81,6 @@ mod tests { use super::*; use crate::mock::*; - frame_benchmarking::impl_benchmark_test_suite!( - Pallet, - crate::mock::new_test_ext(vec![(1, 1), (2, 1), (3, 1)]), - crate::mock::Test, - ); - #[test] fn test_generate_equivocation_report_blob() { let authorities = crate::tests::test_authorities(); diff --git a/frame/im-online/src/benchmarking.rs b/frame/im-online/src/benchmarking.rs index b39b0057c48e8..012da53a183e5 100644 --- a/frame/im-online/src/benchmarking.rs +++ b/frame/im-online/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite}; +use frame_benchmarking::benchmarks; use frame_support::{traits::UnfilteredDispatchable, WeakBoundedVec}; use frame_system::RawOrigin; use sp_core::{offchain::OpaqueMultiaddr, OpaquePeerId}; @@ -100,6 +100,6 @@ benchmarks! { .expect("call is encoded above, encoding must be correct") .dispatch_bypass_filter(RawOrigin::None.into())?; } -} -impl_benchmark_test_suite!(ImOnline, crate::mock::new_test_ext(), crate::mock::Runtime); + impl_benchmark_test_suite!(ImOnline, crate::mock::new_test_ext(), crate::mock::Runtime); +} diff --git a/frame/merkle-mountain-range/src/benchmarking.rs b/frame/merkle-mountain-range/src/benchmarking.rs index f164f70fca6cd..d6ef76d01ac3a 100644 --- a/frame/merkle-mountain-range/src/benchmarking.rs +++ b/frame/merkle-mountain-range/src/benchmarking.rs @@ -18,7 +18,7 @@ //! Benchmarks for the MMR pallet. use crate::*; -use frame_benchmarking::{benchmarks_instance_pallet, impl_benchmark_test_suite}; +use frame_benchmarking::benchmarks_instance_pallet; use frame_support::traits::OnInitialize; benchmarks_instance_pallet! { diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index 2e23dff156e07..edfeba253e5f0 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; use core::convert::TryInto; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite}; +use frame_benchmarking::{account, benchmarks}; use frame_system::RawOrigin; use sp_runtime::traits::Bounded; @@ -297,6 +297,6 @@ benchmarks! { assert!(!Multisigs::::contains_key(multi_account_id, call_hash)); assert!(!Calls::::contains_key(call_hash)); } -} -impl_benchmark_test_suite!(Multisig, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Multisig, crate::tests::new_test_ext(), crate::tests::Test); +} diff --git a/frame/session/benchmarking/src/lib.rs b/frame/session/benchmarking/src/lib.rs index c0131957c8732..8ca713b1bbf61 100644 --- a/frame/session/benchmarking/src/lib.rs +++ b/frame/session/benchmarking/src/lib.rs @@ -24,7 +24,7 @@ mod mock; use sp_std::{prelude::*, vec}; -use frame_benchmarking::{benchmarks, impl_benchmark_test_suite}; +use frame_benchmarking::benchmarks; use frame_support::{ codec::Decode, traits::{KeyOwnerProofSystem, OnInitialize}, @@ -115,6 +115,8 @@ benchmarks! { verify { assert!(Historical::::check_proof(key, key_owner_proof2).is_some()); } + + impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test, extra = false); } /// Sets up the benchmark for checking a membership proof. It creates the given @@ -161,5 +163,3 @@ fn check_membership_proof_setup( (key, Historical::::prove(key).unwrap()) } - -impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test, extra = false); diff --git a/frame/treasury/src/benchmarking.rs b/frame/treasury/src/benchmarking.rs index 2fe0bad704f2b..8570b0efdb945 100644 --- a/frame/treasury/src/benchmarking.rs +++ b/frame/treasury/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::{Pallet as Treasury, *}; -use frame_benchmarking::{account, benchmarks_instance_pallet, impl_benchmark_test_suite}; +use frame_benchmarking::{account, benchmarks_instance_pallet}; use frame_support::{ensure, traits::OnInitialize}; use frame_system::RawOrigin; @@ -94,6 +94,6 @@ benchmarks_instance_pallet! { }: { Treasury::::on_initialize(T::BlockNumber::zero()); } -} -impl_benchmark_test_suite!(Treasury, crate::tests::new_test_ext(), crate::tests::Test); + impl_benchmark_test_suite!(Treasury, crate::tests::new_test_ext(), crate::tests::Test); +} From 8d1897fb9d07defc7b5adf47e5133e88f07b573e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 28 Sep 2021 20:24:27 -0400 Subject: [PATCH 08/13] fix up democracy and use individual tests --- frame/democracy/src/benchmarking.rs | 15 +++++++-------- frame/democracy/src/lib.rs | 2 +- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/frame/democracy/src/benchmarking.rs b/frame/democracy/src/benchmarking.rs index f346ce01a3eaa..7ca4d65e86a92 100644 --- a/frame/democracy/src/benchmarking.rs +++ b/frame/democracy/src/benchmarking.rs @@ -70,7 +70,7 @@ fn add_referendum(n: u32) -> Result { let referendum_index: ReferendumIndex = ReferendumCount::::get() - 1; T::Scheduler::schedule_named( (DEMOCRACY_ID, referendum_index).encode(), - DispatchTime::At(1u32.into()), + DispatchTime::At(2u32.into()), None, 63, frame_system::RawOrigin::Root.into(), @@ -770,11 +770,10 @@ benchmarks! { Err(Error::::PreimageInvalid.into()) ); } -} -// TODO: individual tests -frame_benchmarking::impl_benchmark_test_suite!( - Democracy, - crate::tests::new_test_ext(), - crate::tests::Test -); + impl_benchmark_test_suite!( + Democracy, + crate::tests::new_test_ext(), + crate::tests::Test + ); +} diff --git a/frame/democracy/src/lib.rs b/frame/democracy/src/lib.rs index 8bc6921c4f8ad..cffcb3b9166d4 100644 --- a/frame/democracy/src/lib.rs +++ b/frame/democracy/src/lib.rs @@ -149,7 +149,7 @@ //! - `cancel_queued` - Cancels a proposal that is queued for enactment. //! - `clear_public_proposal` - Removes all public proposals. -#![recursion_limit = "128"] +#![recursion_limit = "256"] #![cfg_attr(not(feature = "std"), no_std)] use codec::{Decode, Encode, Input}; From b0b7c965b110e6babd36f8b31e5bc1544732884e Mon Sep 17 00:00:00 2001 From: ucover <91202993+ucover@users.noreply.github.com> Date: Wed, 29 Sep 2021 14:40:21 +0200 Subject: [PATCH 09/13] Fix comment --- frame/benchmarking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 4f5b917cfc341..475e6dfa8279b 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -505,7 +505,7 @@ macro_rules! benchmarks_iter { $(, $( $args )* )? ); }; - // iteration-exit arm which generates one #[test] function for all cases. + // iteration-exit arm which doesn't generate a #[test] function for all cases. ( { } { $( $instance:ident: $instance_bound:tt )? } From e86084ff524d23cdfc16612b684e5fdb741019f3 Mon Sep 17 00:00:00 2001 From: ucover <91202993+ucover@users.noreply.github.com> Date: Wed, 29 Sep 2021 14:40:47 +0200 Subject: [PATCH 10/13] Put println message in panic --- frame/benchmarking/src/lib.rs | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 475e6dfa8279b..b05bffbab28be 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -849,22 +849,12 @@ macro_rules! impl_bench_name_tests { $module::<$test>::[< test_benchmark_ $name >] () }) { Err(err) => { - println!( - "{}: {:?}", - stringify!($name), - err, - ); - assert!(false); + panic!("{}: {:?}", stringify!($name), err); }, Ok(Err(err)) => { match err { $crate::BenchmarkError::Stop(err) => { - println!( - "{}: {:?}", - stringify!($name), - err, - ); - assert!(false); + panic!("{}: {:?}", stringify!($name), err); }, $crate::BenchmarkError::Override(_) => { // This is still considered a success condition. From 77b778d582b246b354cfcda616bbd5064900746c Mon Sep 17 00:00:00 2001 From: ucover <91202993+ucover@users.noreply.github.com> Date: Thu, 30 Sep 2021 15:19:19 +0200 Subject: [PATCH 11/13] Remove `another_set_dummy` from doc `another_set_dummy` is not present in the benchmarking.rs (anymore). --- frame/benchmarking/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index b05bffbab28be..f4b09fdc8f33a 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1228,7 +1228,6 @@ macro_rules! impl_benchmark_test { /// new_test_ext().execute_with(|| { /// assert_ok!(test_benchmark_accumulate_dummy::()); /// assert_ok!(test_benchmark_set_dummy::()); -/// assert_ok!(test_benchmark_another_set_dummy::()); /// assert_ok!(test_benchmark_sort_vector::()); /// }); /// } From 3c3bf2932c91d9b6444bea1cba4625b991c87841 Mon Sep 17 00:00:00 2001 From: ucover <91202993+ucover@users.noreply.github.com> Date: Thu, 30 Sep 2021 15:20:28 +0200 Subject: [PATCH 12/13] Update doc for benchmarks macro --- frame/benchmarking/src/lib.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index f4b09fdc8f33a..09fba8e5fae5c 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -155,6 +155,12 @@ macro_rules! whitelist { /// 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. /// +/// It is also possible to generate one #[test] function per benchmark by calling the +/// `impl_benchmark_test_suite` macro inside the `benchmarks` block. The functions will be named +/// `bench_` and can be run via `cargo test`. +/// You will see one line of output per benchmark. This approach will give you more understandable +/// error messages and allows for parallel benchmark execution. +/// /// 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: /// @@ -174,7 +180,8 @@ macro_rules! whitelist { /// /// These `verify` blocks will not affect your benchmark results! /// -/// You can construct benchmark tests like so: +/// You can construct benchmark by using the `impl_benchmark_test_suite` macro or +/// by manually implementing them like so: /// /// ```ignore /// #[test] From 3f46e91e7bd75c2da6c0881b6084d57ae1288614 Mon Sep 17 00:00:00 2001 From: ucover <91202993+ucover@users.noreply.github.com> Date: Thu, 30 Sep 2021 15:27:34 +0200 Subject: [PATCH 13/13] Update doc for impl_benchmark_test_suite macro --- frame/benchmarking/src/lib.rs | 42 +++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 09fba8e5fae5c..1805424426f6e 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1241,6 +1241,48 @@ macro_rules! impl_benchmark_test { /// } /// ``` /// +/// When called inside the `benchmarks` macro of the `pallet_example` as +/// +/// ```rust,ignore +/// benchmarks! { +/// // Benchmarks omitted for brevity +/// +/// impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test); +/// } +/// ``` +/// +/// It expands to the equivalent of: +/// +/// ```rust,ignore +/// #[cfg(test)] +/// mod benchmarking { +/// use super::*; +/// use crate::tests::{new_test_ext, Test}; +/// use frame_support::assert_ok; +/// +/// #[test] +/// fn bench_accumulate_dummy() { +/// new_test_ext().execute_with(|| { +/// assert_ok!(test_benchmark_accumulate_dummy::()); +/// } +/// } +/// +/// #[test] +/// fn bench_set_dummy() { +/// new_test_ext().execute_with(|| { +/// assert_ok!(test_benchmark_set_dummy::()); +/// } +/// } +/// +/// #[test] +/// fn bench_sort_vector() { +/// new_test_ext().execute_with(|| { +/// assert_ok!(test_benchmark_sort_vector::()); +/// } +/// } +/// } +/// ``` +/// /// ## Arguments /// /// The first argument, `module`, must be the path to this crate's module.