From 307d6eaa8bcd5e51c5e1e7ab6152eb21c233c5c2 Mon Sep 17 00:00:00 2001
From: Shawn Tabrizi <shawntabrizi@gmail.com>
Date: Fri, 15 Jan 2021 10:44:26 -0400
Subject: [PATCH] Add Test for Variable Components in Benchmarking (#7902)

* Adds a test for variable components

* Clean up traces of common parameters which are removed now
---
 substrate/frame/benchmarking/src/lib.rs   | 54 +----------------------
 substrate/frame/benchmarking/src/tests.rs | 17 ++++++-
 2 files changed, 17 insertions(+), 54 deletions(-)

diff --git a/substrate/frame/benchmarking/src/lib.rs b/substrate/frame/benchmarking/src/lib.rs
index 0657aea5d6..d2cba9cc70 100644
--- a/substrate/frame/benchmarking/src/lib.rs
+++ b/substrate/frame/benchmarking/src/lib.rs
@@ -68,10 +68,6 @@ pub use sp_storage::TrackedStorageKey;
 /// for arbitrary expresions to be evaluated in a benchmark (including for example,
 /// `on_initialize`).
 ///
-/// The macro allows for common parameters whose ranges and instancing expressions may be drawn upon
-/// (or not) by each arm. Syntax is available to allow for only the range to be drawn upon if
-/// desired, allowing an alternative instancing expression to be given.
-///
 /// Note that the ranges are *inclusive* on both sides. This is in contrast to ranges in Rust which
 /// are left-inclusive right-exclusive.
 ///
@@ -80,9 +76,6 @@ pub use sp_storage::TrackedStorageKey;
 /// at any time. Local variables are shared between the two pre- and post- code blocks, but do not
 /// leak from the interior of any instancing expressions.
 ///
-/// Any common parameters that are unused in an arm do not have their instancing expressions
-/// evaluated.
-///
 /// Example:
 /// ```ignore
 /// benchmarks! {
@@ -105,8 +98,7 @@ pub use sp_storage::TrackedStorageKey;
 ///   // third dispatchable: baz; this is a user dispatchable. It isn't dependent on length like the
 ///   // other two but has its own complexity `c` that needs setting up. It uses `caller` (in the
 ///   // pre-instancing block) within the code block. This is only allowed in the param instancers
-///   // of arms. Instancers of common params cannot optimistically draw upon hypothetical variables
-///   // that the arm's pre-instancing code block might have declared.
+///   // of arms.
 ///   baz1 {
 ///     let caller = account::<T>(b"caller", 0, benchmarks_seed);
 ///     let c = 0 .. 10 => setup_c(&caller, c);
@@ -450,50 +442,6 @@ macro_rules! benchmark_backend {
 			$postcode
 		}
 	};
-	// mutation arm to look after defaulting to a common param
-	(
-		{ $( $instance:ident )? }
-		$name:ident
-		{ $( $where_clause:tt )* }
-		{ $( $parsed:tt )* }
-		{ $eval:block }
-		{
-			let $param:ident in ...;
-			$( $rest:tt )*
-		}
-		$postcode:block
-	) => {
-		$crate::benchmark_backend! {
-			{ $( $instance)? }
-			$name
-			{ $( $where_clause )* }
-			{ $( $parsed )* }
-			{ $eval }
-			$postcode
-		}
-	};
-	// mutation arm to look after defaulting only the range to common param
-	(
-		{ $( $instance:ident )? }
-		$name:ident
-		{ $( $where_clause:tt )* }
-		{ $( $parsed:tt )* }
-		{ $eval:block }
-		{
-			let $param:ident in _ .. _ => $param_instancer:expr ;
-			$( $rest:tt )*
-		}
-		$postcode:block
-	) => {
-		$crate::benchmark_backend! {
-			{ $( $instance)? }
-			$name
-			{ $( $where_clause )* }
-			{ $( $parsed )* }
-			{ $eval }
-			$postcode
-		}
-	};
 	// mutation arm to look after a single tt for param_from.
 	(
 		{ $( $instance:ident )? }
diff --git a/substrate/frame/benchmarking/src/tests.rs b/substrate/frame/benchmarking/src/tests.rs
index 2cbf4b9d19..2a2daaffba 100644
--- a/substrate/frame/benchmarking/src/tests.rs
+++ b/substrate/frame/benchmarking/src/tests.rs
@@ -24,7 +24,8 @@ use sp_std::prelude::*;
 use sp_runtime::{traits::{BlakeTwo256, IdentityLookup}, testing::{H256, Header}};
 use frame_support::{
 	dispatch::DispatchResult,
-	decl_module, decl_storage, impl_outer_origin, assert_ok, assert_err, ensure
+	decl_module, decl_storage, impl_outer_origin, assert_ok, assert_err, ensure,
+	parameter_types, pallet_prelude::Get,
 };
 use frame_system::{RawOrigin, ensure_signed, ensure_none};
 
@@ -67,6 +68,8 @@ pub trait Config: frame_system::Config + OtherConfig
 	where Self::OtherEvent: Into<<Self as Config>::Event>
 {
 	type Event;
+	type LowerBound: Get<u32>;
+	type UpperBound: Get<u32>;
 }
 
 #[derive(Clone, Eq, PartialEq)]
@@ -97,8 +100,15 @@ impl frame_system::Config for Test {
 	type SS58Prefix = ();
 }
 
+parameter_types!{
+	pub const LowerBound: u32 = 1;
+	pub const UpperBound: u32 = 100;
+}
+
 impl Config for Test {
 	type Event = ();
+	type LowerBound = LowerBound;
+	type UpperBound = UpperBound;
 }
 
 impl OtherConfig for Test {
@@ -155,6 +165,10 @@ benchmarks!{
 	no_components {
 		let caller = account::<T::AccountId>("caller", 0, 0);
 	}: set_value(RawOrigin::Signed(caller), 0)
+
+	variable_components {
+		let b in ( T::LowerBound::get() ) .. T::UpperBound::get();
+	}: dummy (RawOrigin::None, b.into())
 }
 
 #[test]
@@ -248,5 +262,6 @@ fn benchmarks_generate_unit_tests() {
 		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>());
 	});
 }