diff --git a/substrate/frame/babe/src/benchmarking.rs b/substrate/frame/babe/src/benchmarking.rs index b8a85daf6e..372dfa532a 100644 --- a/substrate/frame/babe/src/benchmarking.rs +++ b/substrate/frame/babe/src/benchmarking.rs @@ -69,14 +69,12 @@ benchmarks! { mod tests { use super::*; use crate::mock::*; - use frame_support::assert_ok; - #[test] - fn test_benchmarks() { - new_test_ext(3).execute_with(|| { - assert_ok!(test_benchmark_check_equivocation_proof::()); - }) - } + frame_benchmarking::impl_benchmark_test_suite!( + Pallet, + crate::mock::new_test_ext(3), + crate::mock::Test, + ); #[test] fn test_generate_equivocation_report_blob() { diff --git a/substrate/frame/benchmarking/src/lib.rs b/substrate/frame/benchmarking/src/lib.rs index 7149ddc82f..26ef4873c2 100644 --- a/substrate/frame/benchmarking/src/lib.rs +++ b/substrate/frame/benchmarking/src/lib.rs @@ -21,7 +21,10 @@ #[cfg(feature = "std")] mod analysis; +#[cfg(test)] mod tests; +#[cfg(test)] +mod tests_instance; mod utils; #[cfg(feature = "std")] @@ -140,8 +143,8 @@ macro_rules! whitelist { /// ``` /// /// Test functions are automatically generated for each benchmark and are accessible to you when you -/// run `cargo test`. All tests are named `test_benchmark_`, expect you to pass them -/// the Runtime Config, and run them in a test externalities environment. The test function runs your +/// run `cargo test`. All tests are named `test_benchmark_`, implemented on the +/// Pallet struct, and run them in a test externalities environment. The test function runs your /// 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. /// @@ -170,10 +173,10 @@ macro_rules! whitelist { /// #[test] /// fn test_benchmarks() { /// 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!(Pallet::::test_benchmark_dummy()); +/// assert_err!(Pallet::::test_benchmark_other_name(), "Bad origin"); +/// assert_ok!(Pallet::::test_benchmark_sort_vector()); +/// assert_err!(Pallet::::test_benchmark_broken_benchmark(), "You forgot to sort!"); /// }); /// } /// ``` @@ -879,28 +882,30 @@ macro_rules! impl_benchmark { } } - /// Test a particular benchmark by name. - /// - /// This isn't called `test_benchmark_by_name` just in case some end-user eventually - /// writes a benchmark, itself called `by_name`; the function would be shadowed in - /// that case. - /// - /// This is generally intended to be used by child test modules such as those created - /// by the `impl_benchmark_test_suite` macro. However, it is not an error if a pallet - /// author chooses not to implement benchmarks. #[cfg(test)] - #[allow(unused)] - fn test_bench_by_name(name: &[u8]) -> Result<(), &'static str> - where - T: Config + frame_system::Config, $( $where_clause )* + impl, $instance: $instance_bound )? > + Pallet + where T: frame_system::Config, $( $where_clause )* { - let name = $crate::sp_std::str::from_utf8(name) - .map_err(|_| "`name` is not a valid utf8 string!")?; - match name { - $( stringify!($name) => { - $crate::paste::paste! { [< test_benchmark_ $name >]::() } - } )* - _ => Err("Could not find test for requested benchmark."), + /// Test a particular benchmark by name. + /// + /// This isn't called `test_benchmark_by_name` just in case some end-user eventually + /// writes a benchmark, itself called `by_name`; the function would be shadowed in + /// that case. + /// + /// This is generally intended to be used by child test modules such as those created + /// by the `impl_benchmark_test_suite` macro. However, it is not an error if a pallet + /// author chooses not to implement benchmarks. + #[allow(unused)] + fn test_bench_by_name(name: &[u8]) -> Result<(), &'static str> { + let name = $crate::sp_std::str::from_utf8(name) + .map_err(|_| "`name` is not a valid utf8 string!")?; + match name { + $( stringify!($name) => { + $crate::paste::paste! { Self::[< test_benchmark_ $name >]() } + } )* + _ => Err("Could not find test for requested benchmark."), + } } } }; @@ -918,59 +923,66 @@ macro_rules! impl_benchmark_test { $name:ident ) => { $crate::paste::item! { - fn [] () -> Result<(), &'static str> - where T: frame_system::Config, $( $where_clause )* + #[cfg(test)] + impl, $instance: $instance_bound )? > + Pallet + where T: frame_system::Config, $( $where_clause )* { - let selected_benchmark = SelectedBenchmark::$name; - let components = < - SelectedBenchmark as $crate::BenchmarkingSetup - >::components(&selected_benchmark); - - let execute_benchmark = | - c: $crate::Vec<($crate::BenchmarkParameter, u32)> - | -> Result<(), &'static str> { - // Set up the benchmark, return execution + verification function. - let closure_to_verify = < + #[allow(unused)] + fn [] () -> Result<(), &'static str> { + let selected_benchmark = SelectedBenchmark::$name; + let components = < SelectedBenchmark as $crate::BenchmarkingSetup - >::instance(&selected_benchmark, &c, true)?; + >::components(&selected_benchmark); - // Set the block number to at least 1 so events are deposited. - if $crate::Zero::is_zero(&frame_system::Pallet::::block_number()) { - frame_system::Pallet::::set_block_number(1u32.into()); - } + let execute_benchmark = | + c: $crate::Vec<($crate::BenchmarkParameter, u32)> + | -> Result<(), &'static str> { + // Set up the benchmark, return execution + verification function. + let closure_to_verify = < + SelectedBenchmark as $crate::BenchmarkingSetup + >::instance(&selected_benchmark, &c, true)?; - // Run execution + verification - closure_to_verify()?; + // Set the block number to at least 1 so events are deposited. + if $crate::Zero::is_zero(&frame_system::Pallet::::block_number()) { + frame_system::Pallet::::set_block_number(1u32.into()); + } - // Reset the state - $crate::benchmarking::wipe_db(); + // Run execution + verification + closure_to_verify()?; - Ok(()) - }; + // Reset the state + $crate::benchmarking::wipe_db(); - if components.is_empty() { - execute_benchmark(Default::default())?; - } else { - 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 $crate::vec![low, high] { - // Select the max value for all the other components. - let c: $crate::Vec<($crate::BenchmarkParameter, u32)> = components.iter() - .enumerate() - .map(|(_, (n, _, h))| - if n == name { - (*n, *component_value) - } else { - (*n, *h) - } - ) - .collect(); + Ok(()) + }; - execute_benchmark(c)?; + if components.is_empty() { + execute_benchmark(Default::default())?; + } else { + 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 $crate::vec![low, high] { + // Select the max value for all the other components. + let c: $crate::Vec<($crate::BenchmarkParameter, u32)> = components + .iter() + .enumerate() + .map(|(_, (n, _, h))| + if n == name { + (*n, *component_value) + } else { + (*n, *h) + } + ) + .collect(); + + execute_benchmark(c)?; + } } } + Ok(()) } - Ok(()) } } }; @@ -1084,7 +1096,7 @@ macro_rules! impl_benchmark_test_suite { $test:path $(, $( $rest:tt )* )? ) => { - impl_benchmark_test_suite!( + $crate::impl_benchmark_test_suite!( @selected: $bench_module, $new_test_ext, @@ -1109,7 +1121,7 @@ macro_rules! impl_benchmark_test_suite { benchmarks_path = $benchmarks_path:ident $(, $( $rest:tt )* )? ) => { - impl_benchmark_test_suite!( + $crate::impl_benchmark_test_suite!( @selected: $bench_module, $new_test_ext, @@ -1134,7 +1146,7 @@ macro_rules! impl_benchmark_test_suite { extra = $extra:expr $(, $( $rest:tt )* )? ) => { - impl_benchmark_test_suite!( + $crate::impl_benchmark_test_suite!( @selected: $bench_module, $new_test_ext, @@ -1159,7 +1171,7 @@ macro_rules! impl_benchmark_test_suite { exec_name = $exec_name:ident $(, $( $rest:tt )* )? ) => { - impl_benchmark_test_suite!( + $crate::impl_benchmark_test_suite!( @selected: $bench_module, $new_test_ext, @@ -1185,7 +1197,6 @@ macro_rules! impl_benchmark_test_suite { ) => { #[cfg(test)] mod benchmark_tests { - use $path_to_benchmarks_invocation::test_bench_by_name; use super::$bench_module; #[test] @@ -1196,7 +1207,9 @@ macro_rules! impl_benchmark_test_suite { let mut anything_failed = false; println!("failing benchmark tests:"); for benchmark_name in $bench_module::<$test>::benchmarks($extra) { - match std::panic::catch_unwind(|| test_bench_by_name::<$test>(benchmark_name)) { + match std::panic::catch_unwind(|| { + $bench_module::<$test>::test_bench_by_name(benchmark_name) + }) { Err(err) => { println!("{}: {:?}", String::from_utf8_lossy(benchmark_name), err); anything_failed = true; diff --git a/substrate/frame/benchmarking/src/tests.rs b/substrate/frame/benchmarking/src/tests.rs index 7bb1f9d7d6..aabdb7815c 100644 --- a/substrate/frame/benchmarking/src/tests.rs +++ b/substrate/frame/benchmarking/src/tests.rs @@ -291,13 +291,13 @@ mod benchmarks { #[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!"); - assert_ok!(test_benchmark_no_components::()); - assert_ok!(test_benchmark_variable_components::()); + assert_ok!(Pallet::::test_benchmark_set_value()); + assert_ok!(Pallet::::test_benchmark_other_name()); + assert_ok!(Pallet::::test_benchmark_sort_vector()); + assert_err!(Pallet::::test_benchmark_bad_origin(), "Bad origin"); + assert_err!(Pallet::::test_benchmark_bad_verify(), "You forgot to sort!"); + assert_ok!(Pallet::::test_benchmark_no_components()); + assert_ok!(Pallet::::test_benchmark_variable_components()); }); } } diff --git a/substrate/frame/benchmarking/src/tests_instance.rs b/substrate/frame/benchmarking/src/tests_instance.rs new file mode 100644 index 0000000000..221e9faa5c --- /dev/null +++ b/substrate/frame/benchmarking/src/tests_instance.rs @@ -0,0 +1,183 @@ +// This file is part of Substrate. + +// Copyright (C) 2020-2021 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Tests for the benchmark macro for instantiable modules + +#![cfg(test)] + +use super::*; +use frame_support::parameter_types; +use sp_runtime::{ + testing::{Header, H256}, + traits::{BlakeTwo256, IdentityLookup}, + BuildStorage, +}; +use sp_std::prelude::*; + +mod pallet_test { + use frame_support::pallet_prelude::Get; + + frame_support::decl_storage! { + trait Store for Module, I: Instance = DefaultInstance> as Test where + ::OtherEvent: Into<>::Event> + { + pub Value get(fn value): Option; + } + } + + frame_support::decl_module! { + pub struct Module, I: Instance = DefaultInstance> for enum Call where + origin: T::Origin, ::OtherEvent: Into<>::Event> + { + #[weight = 0] + fn set_value(origin, n: u32) -> frame_support::dispatch::DispatchResult { + let _sender = frame_system::ensure_signed(origin)?; + Value::::put(n); + Ok(()) + } + + #[weight = 0] + fn dummy(origin, _n: u32) -> frame_support::dispatch::DispatchResult { + let _sender = frame_system::ensure_none(origin)?; + Ok(()) + } + } + } + + pub trait OtherConfig { + type OtherEvent; + } + + pub trait Config: frame_system::Config + OtherConfig + where + Self::OtherEvent: Into<>::Event>, + { + type Event; + type LowerBound: Get; + type UpperBound: Get; + } +} + +type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; +type Block = frame_system::mocking::MockBlock; + +frame_support::construct_runtime!( + pub enum Test where + Block = Block, + NodeBlock = Block, + UncheckedExtrinsic = UncheckedExtrinsic, + { + System: frame_system::{Pallet, Call, Config, Storage, Event}, + TestPallet: pallet_test::{Pallet, Call, Storage}, + } +); + +impl frame_system::Config for Test { + type BaseCallFilter = frame_support::traits::AllowAll; + type BlockWeights = (); + type BlockLength = (); + type DbWeight = (); + type Origin = Origin; + type Index = u64; + type BlockNumber = u64; + type Hash = H256; + type Call = Call; + type Hashing = BlakeTwo256; + type AccountId = u64; + type Lookup = IdentityLookup; + type Header = Header; + type Event = Event; + type BlockHashCount = (); + type Version = (); + type PalletInfo = PalletInfo; + type AccountData = (); + type OnNewAccount = (); + type OnKilledAccount = (); + type SystemWeightInfo = (); + type SS58Prefix = (); + type OnSetCode = (); +} + +parameter_types! { + pub const LowerBound: u32 = 1; + pub const UpperBound: u32 = 100; +} + +impl pallet_test::Config for Test { + type Event = Event; + type LowerBound = LowerBound; + type UpperBound = UpperBound; +} + +impl pallet_test::OtherConfig for Test { + type OtherEvent = Event; +} + +fn new_test_ext() -> sp_io::TestExternalities { + GenesisConfig::default().build_storage().unwrap().into() +} + +mod benchmarks { + use super::pallet_test::{self, Value}; + use crate::account; + use frame_support::{ensure, StorageValue}; + use frame_system::RawOrigin; + use sp_std::prelude::*; + + // Additional used internally by the benchmark macro. + use super::pallet_test::{Call, Config, Pallet}; + use frame_support::traits::Instance; + + crate::benchmarks_instance! { + where_clause { + where + ::OtherEvent: Clone + + Into<>::Event>, + >::Event: Clone, + } + + set_value { + let b in 1 .. 1000; + let caller = account::("caller", 0, 0); + }: _ (RawOrigin::Signed(caller), b.into()) + verify { + assert_eq!(Value::::get(), Some(b)); + } + + other_name { + let b in 1 .. 1000; + }: dummy (RawOrigin::None, b.into()) + + 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!") + } + } + + crate::impl_benchmark_test_suite!( + Pallet, + crate::tests_instance::new_test_ext(), + crate::tests_instance::Test + ); +} diff --git a/substrate/frame/grandpa/src/benchmarking.rs b/substrate/frame/grandpa/src/benchmarking.rs index d5372c5687..b0f70adb60 100644 --- a/substrate/frame/grandpa/src/benchmarking.rs +++ b/substrate/frame/grandpa/src/benchmarking.rs @@ -76,15 +76,12 @@ benchmarks! { mod tests { use super::*; use crate::mock::*; - use frame_support::assert_ok; - #[test] - fn test_benchmarks() { - new_test_ext(vec![(1, 1), (2, 1), (3, 1)]).execute_with(|| { - assert_ok!(test_benchmark_check_equivocation_proof::()); - assert_ok!(test_benchmark_note_stalled::()); - }) - } + 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() {