Fix benchmarking macro for pallet with instance and where clause (#9485)

* fix benchmarking instance with where clause

* fmt

* add tests

* remove unused import

* fix tests

* doc

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::<Test>());
-		})
-	}
+	frame_benchmarking::impl_benchmark_test_suite!(
+		Pallet,
+		crate::mock::new_test_ext(3),
+		crate::mock::Test,
+	);
 
 	#[test]
 	fn test_generate_equivocation_report_blob() {

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_<benchmark_name>`, 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_<benchmark_name>`, 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::<Test>());
-///     assert_err!(test_benchmark_other_name::<Test>(), "Bad origin");
-///     assert_ok!(test_benchmark_sort_vector::<Test>());
-///     assert_err!(test_benchmark_broken_benchmark::<Test>(), "You forgot to sort!");
+///     assert_ok!(Pallet::<Test>::test_benchmark_dummy());
+///     assert_err!(Pallet::<Test>::test_benchmark_other_name(), "Bad origin");
+///     assert_ok!(Pallet::<Test>::test_benchmark_sort_vector());
+///     assert_err!(Pallet::<Test>::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<T>(name: &[u8]) -> Result<(), &'static str>
-		where
-			T: Config + frame_system::Config, $( $where_clause )*
+		impl<T: Config $(<$instance>, $instance: $instance_bound )? >
+			Pallet<T $(, $instance)? >
+		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 >]::<T>() }
-				} )*
-				_ => 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 [<test_benchmark_ $name>] <T: Config > () -> Result<(), &'static str>
-				where T: frame_system::Config, $( $where_clause )*
+			#[cfg(test)]
+			impl<T: Config $(<$instance>, $instance: $instance_bound )? >
+				Pallet<T $(, $instance)? >
+			where T: frame_system::Config, $( $where_clause )*
 			{
-				let selected_benchmark = SelectedBenchmark::$name;
-				let components = <
-					SelectedBenchmark as $crate::BenchmarkingSetup<T, _>
-				>::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 [<test_benchmark_ $name>] () -> Result<(), &'static str> {
+					let selected_benchmark = SelectedBenchmark::$name;
+					let components = <
 						SelectedBenchmark as $crate::BenchmarkingSetup<T, _>
-					>::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::<T>::block_number()) {
-						frame_system::Pallet::<T>::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<T, _>
+						>::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::<T>::block_number()) {
+							frame_system::Pallet::<T>::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;

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::<Test>());
-			assert_ok!(test_benchmark_other_name::<Test>());
-			assert_ok!(test_benchmark_sort_vector::<Test>());
-			assert_err!(test_benchmark_bad_origin::<Test>(), "Bad origin");
-			assert_err!(test_benchmark_bad_verify::<Test>(), "You forgot to sort!");
-			assert_ok!(test_benchmark_no_components::<Test>());
-			assert_ok!(test_benchmark_variable_components::<Test>());
+			assert_ok!(Pallet::<Test>::test_benchmark_set_value());
+			assert_ok!(Pallet::<Test>::test_benchmark_other_name());
+			assert_ok!(Pallet::<Test>::test_benchmark_sort_vector());
+			assert_err!(Pallet::<Test>::test_benchmark_bad_origin(), "Bad origin");
+			assert_err!(Pallet::<Test>::test_benchmark_bad_verify(), "You forgot to sort!");
+			assert_ok!(Pallet::<Test>::test_benchmark_no_components());
+			assert_ok!(Pallet::<Test>::test_benchmark_variable_components());
 		});
 	}
 }

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<T: Config<I>, I: Instance = DefaultInstance> as Test where
+			<T as OtherConfig>::OtherEvent: Into<<T as Config<I>>::Event>
+		{
+			pub Value get(fn value): Option<u32>;
+		}
+	}
+
+	frame_support::decl_module! {
+		pub struct Module<T: Config<I>, I: Instance = DefaultInstance> for enum Call where
+			origin: T::Origin, <T as OtherConfig>::OtherEvent: Into<<T as Config<I>>::Event>
+		{
+			#[weight = 0]
+			fn set_value(origin, n: u32) -> frame_support::dispatch::DispatchResult {
+				let _sender = frame_system::ensure_signed(origin)?;
+				Value::<I>::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<I: Instance = DefaultInstance>: frame_system::Config + OtherConfig
+	where
+		Self::OtherEvent: Into<<Self as Config<I>>::Event>,
+	{
+		type Event;
+		type LowerBound: Get<u32>;
+		type UpperBound: Get<u32>;
+	}
+}
+
+type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
+type Block = frame_system::mocking::MockBlock<Test>;
+
+frame_support::construct_runtime!(
+	pub enum Test where
+		Block = Block,
+		NodeBlock = Block,
+		UncheckedExtrinsic = UncheckedExtrinsic,
+	{
+		System: frame_system::{Pallet, Call, Config, Storage, Event<T>},
+		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<Self::AccountId>;
+	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
+				<T as pallet_test::OtherConfig>::OtherEvent: Clone
+					+ Into<<T as pallet_test::Config<I>>::Event>,
+				<T as pallet_test::Config<I>>::Event: Clone,
+		}
+
+		set_value {
+			let b in 1 .. 1000;
+			let caller = account::<T::AccountId>("caller", 0, 0);
+		}: _ (RawOrigin::Signed(caller), b.into())
+		verify {
+			assert_eq!(Value::<pallet_test::DefaultInstance>::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::<u32>::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
+	);
+}

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::<Test>());
-			assert_ok!(test_benchmark_note_stalled::<Test>());
-		})
-	}
+	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() {