Repair and extend some benchmarks (#5648)

substrate/bin/node/runtime/src/lib.rs

@@ -903,13 +903,13 @@ impl_runtime_apis! {
 			add_benchmark!(params, batches, b"democracy", Democracy);
 			add_benchmark!(params, batches, b"identity", Identity);
 			add_benchmark!(params, batches, b"im-online", ImOnline);
+			add_benchmark!(params, batches, b"offences", OffencesBench::<Runtime>);
 			add_benchmark!(params, batches, b"session", SessionBench::<Runtime>);
 			add_benchmark!(params, batches, b"staking", Staking);
 			add_benchmark!(params, batches, b"timestamp", Timestamp);
 			add_benchmark!(params, batches, b"treasury", Treasury);
 			add_benchmark!(params, batches, b"utility", Utility);
 			add_benchmark!(params, batches, b"vesting", Vesting);
-			add_benchmark!(params, batches, b"offences", OffencesBench::<Runtime>);
 
 			if batches.is_empty() { return Err("Benchmark not found for this pallet.".into()) }
 			Ok(batches)

substrate/frame/benchmarking/src/lib.rs

@@ -723,6 +723,9 @@ macro_rules! impl_benchmark {
 					let steps = steps.get(idx).cloned().unwrap_or(prev_steps);
 					prev_steps = steps;
 
+					// Skip this loop if steps is zero
+					if steps == 0 { continue }
+
 					let lowest = lowest_range_values.get(idx).cloned().unwrap_or(*low);
 					let highest = highest_range_values.get(idx).cloned().unwrap_or(*high);
 
@@ -828,6 +831,9 @@ macro_rules! impl_benchmark {
 					let steps = steps.get(idx).cloned().unwrap_or(prev_steps);
 					prev_steps = steps;
 
+					// Skip this loop if steps is zero
+					if steps == 0 { continue }
+
 					let lowest = lowest_range_values.get(idx).cloned().unwrap_or(*low);
 					let highest = highest_range_values.get(idx).cloned().unwrap_or(*high);
 
@@ -1036,7 +1042,7 @@ macro_rules! add_benchmark {
 							&steps[..],
 							repeat,
 						)?,
-						pallet: pallet.to_vec(),
+						pallet: $name.to_vec(),
 						benchmark: benchmark.to_vec(),
 					});
 				}
@@ -1049,7 +1055,7 @@ macro_rules! add_benchmark {
 						&steps[..],
 						repeat,
 					)?,
-					pallet: pallet.to_vec(),
+					pallet: $name.to_vec(),
 					benchmark: benchmark.clone(),
 				});
 			}

substrate/frame/collective/src/benchmarking.rs

@@ -19,173 +19,398 @@
 use super::*;
 
 use frame_system::RawOrigin as SystemOrigin;
+use frame_system::EventRecord;
 use frame_benchmarking::{benchmarks_instance, account};
+use sp_runtime::traits::Bounded;
 
 use frame_system::Module as System;
 use crate::Module as Collective;
 
 const SEED: u32 = 0;
 
+const MAX_MEMBERS: u32 = 1000;
+const MAX_PROPOSALS: u32 = 100;
+const MAX_BYTES: u32 = 1_024;
+
+fn assert_last_event<T: Trait<I>, I: Instance>(generic_event: <T as Trait<I>>::Event) {
+	let events = System::<T>::events();
+	let system_event: <T as frame_system::Trait>::Event = generic_event.into();
+	// compare to the last event record
+	let EventRecord { event, .. } = &events[events.len() - 1];
+	assert_eq!(event, &system_event);
+}
+
 benchmarks_instance! {
-	_{
-		// User account seed.
-		let u in 1 .. 1000 => ();
-		// Old members.
-		let n in 1 .. 1000 => ();
-		// New members.
-		let m in 1 .. 1000 => ();
-		// Existing proposals.
-		let p in 1 .. 100 => ();
-	}
+	_{ }
 
 	set_members {
-		let m in ...;
-		let n in ...;
-
-		// Construct `new_members`.
-		// It should influence timing since it will sort this vector.
-		let mut new_members = vec![];
-		for i in 0 .. m {
-			let member = account("member", i, SEED);
-			new_members.push(member);
-		}
+		let m in 1 .. MAX_MEMBERS;
+		let n in 1 .. MAX_MEMBERS;
 
 		// Set old members.
 		// We compute the difference of old and new members, so it should influence timing.
 		let mut old_members = vec![];
+		let mut last_old_member = T::AccountId::default();
 		for i in 0 .. n {
-			let old_member = account("old member", i, SEED);
-			old_members.push(old_member);
+			last_old_member = account("old member", i, SEED);
+			old_members.push(last_old_member.clone());
 		}
 
-		let prime = Some(account("prime", 0, SEED));
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), old_members, Some(last_old_member))?;
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), old_members, prime.clone())?;
+		// Construct `new_members`.
+		// It should influence timing since it will sort this vector.
+		let mut new_members = vec![];
+		let mut last_member = T::AccountId::default();
+		for i in 0 .. m {
+			last_member = account("member", i, SEED);
+			new_members.push(last_member.clone());
+		}
 
-	}: _(SystemOrigin::Root, new_members.clone(), prime)
+	}: _(SystemOrigin::Root, new_members.clone(), Some(last_member))
 	verify {
 		new_members.sort();
 		assert_eq!(Collective::<T, _>::members(), new_members);
 	}
 
 	execute {
-		let u in ...;
+		let m in 1 .. MAX_MEMBERS;
+		let b in 1 .. MAX_BYTES;
 
-		let caller: T::AccountId = account("caller", u, SEED);
-		let proposal: T::Proposal = Call::<T, I>::close(Default::default(), Default::default()).into();
+		// Construct `members`.
+		let mut members = vec![];
+		for i in 0 .. m {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), vec![caller.clone()], None)?;
+		let caller: T::AccountId = account("caller", 0, SEED);
+		members.push(caller.clone());
 
-	}: _(SystemOrigin::Signed(caller), Box::new(proposal))
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None)?;
 
-	propose {
-		let u in ...;
+		let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![1; b as usize]).into();
 
-		let caller: T::AccountId = account("caller", u, SEED);
-		let proposal: T::Proposal = Call::<T, I>::close(Default::default(), Default::default()).into();
+	}: _(SystemOrigin::Signed(caller), Box::new(proposal.clone()))
+	verify {
+		let proposal_hash = T::Hashing::hash_of(&proposal);
+		// Note that execution fails due to mis-matched origin
+		assert_last_event::<T, I>(RawEvent::MemberExecuted(proposal_hash, false).into());
+	}
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), vec![caller.clone()], None)?;
+	// This tests when execution would happen immediately after proposal
+	propose_execute {
+		let m in 1 .. MAX_MEMBERS;
+		let b in 1 .. MAX_BYTES;
 
-		let member_count = 0;
+		// Construct `members`.
+		let mut members = vec![];
+		for i in 0 .. m {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
 
-	}: _(SystemOrigin::Signed(caller), member_count, Box::new(proposal.into()))
+		let caller: T::AccountId = account("caller", 0, SEED);
+		members.push(caller.clone());
 
-	propose_else_branch {
-		let u in ...;
-		let p in ...;
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None)?;
 
-		let caller: T::AccountId = account("caller", u, SEED);
-		let proposal: T::Proposal = Call::<T, I>::close(Default::default(), Default::default()).into();
+		let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![1; b as usize]).into();
+		let threshold = 1;
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), vec![caller.clone()], None)?;
+	}: propose(SystemOrigin::Signed(caller), threshold, Box::new(proposal.clone()))
+	verify {
+		let proposal_hash = T::Hashing::hash_of(&proposal);
+		// Note that execution fails due to mis-matched origin
+		assert_last_event::<T, I>(RawEvent::Executed(proposal_hash, false).into());
+	}
 
-		let member_count = 3;
+	// This tests when proposal is created and queued as "proposed"
+	propose_proposed {
+		let m in 1 .. MAX_MEMBERS;
+		let p in 0 .. MAX_PROPOSALS;
+		let b in 1 .. MAX_BYTES;
+
+		// Construct `members`.
+		let mut members = vec![];
+		for i in 0 .. m {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
+
+		let caller: T::AccountId = account("caller", 0, SEED);
+		members.push(caller.clone());
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members, None)?;
+
+		let threshold = m.max(2);
 
 		// Add previous proposals.
 		for i in 0 .. p {
-			let proposal: T::Proposal = Call::<T, I>::close(Default::default(), (i + 1).into()).into();
-			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), member_count.clone(), Box::new(proposal.into()))?;
+			// Proposals should be different so that different proposal hashes are generated
+			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal))?;
 		}
 
-	}: propose(SystemOrigin::Signed(caller), member_count, Box::new(proposal.into()))
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
 
-	vote {
-		let u in ...;
+		let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![p as u8; b as usize]).into();
 
-		let caller1: T::AccountId = account("caller1", u, SEED);
-		let caller2: T::AccountId = account("caller2", u, SEED);
-
-		let proposal: Box<T::Proposal> = Box::new(Call::<T, I>::close(Default::default(), Default::default()).into());
+	}: propose(SystemOrigin::Signed(caller.clone()), threshold, Box::new(proposal.clone()))
+	verify {
+		// New proposal is recorded
+		assert_eq!(Collective::<T, _>::proposals().len(), (p + 1) as usize);
 		let proposal_hash = T::Hashing::hash_of(&proposal);
+		assert_last_event::<T, I>(RawEvent::Proposed(caller, p, proposal_hash, threshold).into());
+	}
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), vec![caller1.clone(), caller2.clone()], None)?;
+	vote_insert {
+		let m in 2 .. MAX_MEMBERS;
+		let p in 1 .. MAX_PROPOSALS;
+		let b in 1 .. MAX_BYTES;
 
-		let member_count = 3;
-		Collective::<T, _>::propose(SystemOrigin::Signed(caller1.clone()).into(), member_count, proposal)?;
+		// Construct `members`.
+		let mut members = vec![];
+		for i in 0 .. m {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
 
-		let index = 0;
-		let approve = true;
+		let caller: T::AccountId = account("caller", 0, SEED);
+		members.push(caller.clone());
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None)?;
 
-	}: _(SystemOrigin::Signed(caller2), proposal_hash, index, approve)
+		// Threshold is 1 less than the number of members so that one person can vote nay
+		let threshold = m;
 
-	vote_not_approve {
-		let u in ...;
+		// Add previous proposals
+		let mut last_hash = T::Hash::default();
+		for i in 0 .. p {
+			// Proposals should be different so that different proposal hashes are generated
+			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			last_hash = T::Hashing::hash_of(&proposal);
+		}
 
-		let caller1: T::AccountId = account("caller1", u, SEED);
-		let caller2: T::AccountId = account("caller2", u, SEED);
+		// Have everyone vote aye on last proposal, while keeping it from passing
+		for j in 2 .. m {
+			let voter = &members[j as usize];
+			let approve = true;
+			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+		}
 
-		let proposal: Box<T::Proposal> = Box::new(Call::<T, I>::close(Default::default(), Default::default()).into());
-		let proposal_hash = T::Hashing::hash_of(&proposal);
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), vec![caller1.clone(), caller2.clone()], None)?;
-
-		let member_count = 3;
-		Collective::<T, _>::propose(SystemOrigin::Signed(caller1.clone()).into(), member_count, proposal)?;
-
-		let index = 0;
+		// Caller switches vote to nay, but does not kill the vote, just updates + inserts
+		let index = p - 1;
 		let approve = false;
 
-	}: vote(SystemOrigin::Signed(caller2), proposal_hash, index, approve)
+	}: vote(SystemOrigin::Signed(caller), last_hash.clone(), index, approve)
+	verify {
+		// All proposals exist and the last proposal has just been updated.
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
+		let voting = Collective::<T, _>::voting(&last_hash).ok_or(Error::<T, I>::ProposalMissing)?;
+		assert_eq!(voting.ayes.len(), (m - 2) as usize);
+		assert_eq!(voting.nays.len(), 1);
+	}
+
+	vote_disapproved {
+		let m in 2 .. MAX_MEMBERS;
+		let p in 1 .. MAX_PROPOSALS;
+		let b in 1 .. MAX_BYTES;
+
+		// Construct `members`.
+		let mut members = vec![];
+		for i in 0 .. m {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
+
+		let caller: T::AccountId = account("caller", 0, SEED);
+		members.push(caller.clone());
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None)?;
+
+		// Threshold is total members so that one nay will disapprove the vote
+		let threshold = m + 1;
+
+		// Add previous proposals
+		let mut last_hash = T::Hash::default();
+		for i in 0 .. p {
+			// Proposals should be different so that different proposal hashes are generated
+			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			last_hash = T::Hashing::hash_of(&proposal);
+		}
+
+		// Have everyone vote aye on last proposal, while keeping it from passing
+		for j in 1 .. m {
+			let voter = &members[j as usize];
+			let approve = true;
+			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+		}
+
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
+
+		// Caller switches vote to nay, which kills the vote
+		let index = p - 1;
+		let approve = false;
+
+	}: vote(SystemOrigin::Signed(caller), last_hash.clone(), index, approve)
+	verify {
+		// The last proposal is removed.
+		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
+		assert_last_event::<T, I>(RawEvent::Disapproved(last_hash).into());
+	}
 
 	vote_approved {
-		let u in ...;
+		let m in 2 .. MAX_MEMBERS;
+		let p in 1 .. MAX_PROPOSALS;
+		let b in 1 .. MAX_BYTES;
 
-		let caller1: T::AccountId = account("caller1", u, SEED);
-		let caller2: T::AccountId = account("caller2", u, SEED);
+		// Construct `members`.
+		let mut members = vec![];
+		for i in 0 .. m {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
 
-		let proposal: Box<T::Proposal> = Box::new(Call::<T, I>::close(Default::default(), Default::default()).into());
-		let proposal_hash = T::Hashing::hash_of(&proposal);
+		let caller: T::AccountId = account("caller", 0, SEED);
+		members.push(caller.clone());
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), None)?;
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), vec![caller1.clone(), caller2.clone()], None)?;
+		// Threshold is 2 so any two ayes will approve the vote
+		let threshold = 2;
 
-		let member_count = 2;
-		Collective::<T, _>::propose(SystemOrigin::Signed(caller1.clone()).into(), member_count, proposal)?;
+		// Add previous proposals
+		let mut last_hash = T::Hash::default();
+		for i in 0 .. p {
+			// Proposals should be different so that different proposal hashes are generated
+			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			last_hash = T::Hashing::hash_of(&proposal);
+		}
 
-		let index = 0;
+		// Caller switches vote to nay on their own proposal, allowing them to be the deciding approval vote
+		Collective::<T, _>::vote(SystemOrigin::Signed(caller.clone()).into(), last_hash.clone(), p - 1, false)?;
+
+		// Have everyone vote nay on last proposal, while keeping it from failing
+		for j in 2 .. m {
+			let voter = &members[j as usize];
+			let approve = false;
+			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+		}
+
+		// Member zero is the first aye
+		Collective::<T, _>::vote(SystemOrigin::Signed(members[0].clone()).into(), last_hash.clone(), p - 1, true)?;
+
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
+
+		// Caller switches vote to aye, which passes the vote
+		let index = p - 1;
 		let approve = true;
 
-	}: vote(SystemOrigin::Signed(caller2), proposal_hash, index, approve)
+	}: vote(SystemOrigin::Signed(caller), last_hash.clone(), index, approve)
+	verify {
+		// The last proposal is removed.
+		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
+		assert_last_event::<T, I>(RawEvent::Executed(last_hash, false).into());
+	}
 
-	close {
-		let u in ...;
+	close_disapproved {
+		let m in 2 .. MAX_MEMBERS;
+		let p in 1 .. MAX_PROPOSALS;
+		let b in 1 .. MAX_BYTES;
 
-		let caller1: T::AccountId = account("caller1", u, SEED);
-		let caller2: T::AccountId = account("caller2", u, SEED);
+		// Construct `members`.
+		let mut members = vec![];
+		for i in 0 .. m {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
+		let caller: T::AccountId = account("caller", 0, SEED);
+		members.push(caller.clone());
 
-		let proposal: Box<T::Proposal> = Box::new(Call::<T, I>::close(Default::default(), Default::default()).into());
-		let proposal_hash = T::Hashing::hash_of(&proposal);
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), Some(caller.clone()))?;
 
-		Collective::<T, _>::set_members(SystemOrigin::Root.into(), vec![caller1.clone(), caller2.clone()], None)?;
-		let member_count = 2;
-		Collective::<T, _>::propose(SystemOrigin::Signed(caller1.clone()).into(), member_count, proposal)?;
+		// Threshold is one less than total members so that two nays will disapprove the vote
+		let threshold = m;
 
-		let index = 0;
-		let approve = true;
+		// Add proposals
+		let mut last_hash = T::Hash::default();
+		for i in 0 .. p {
+			// Proposals should be different so that different proposal hashes are generated
+			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			last_hash = T::Hashing::hash_of(&proposal);
+		}
 
-		let vote_end = T::MotionDuration::get() + 1u32.into();
-		System::<T>::set_block_number(vote_end);
+		// Have everyone vote aye on last proposal, while keeping it from passing
+		// A few abstainers will be the nay votes needed to fail the vote
+		for j in 2 .. m {
+			let voter = &members[j as usize];
+			let approve = true;
+			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+		}
 
-	}: _(SystemOrigin::Signed(caller2), proposal_hash, index)
+		// caller is prime, prime votes nay
+		Collective::<T, _>::vote(SystemOrigin::Signed(caller.clone()).into(), last_hash.clone(), p - 1, false)?;
+
+		System::<T>::set_block_number(T::BlockNumber::max_value());
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
+
+		// Prime nay will close it as disapproved
+	}: close(SystemOrigin::Signed(caller), last_hash, p - 1)
+	verify {
+		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
+		assert_last_event::<T, I>(RawEvent::Disapproved(last_hash).into());
+	}
+
+
+	close_approved {
+		let m in 2 .. MAX_MEMBERS;
+		let p in 1 .. MAX_PROPOSALS;
+		let b in 1 .. MAX_BYTES;
+
+		// Construct `members`.
+		let mut members = vec![];
+		for i in 0 .. m {
+			let member = account("member", i, SEED);
+			members.push(member);
+		}
+		let caller: T::AccountId = account("caller", 0, SEED);
+		members.push(caller.clone());
+
+		Collective::<T, _>::set_members(SystemOrigin::Root.into(), members.clone(), Some(caller.clone()))?;
+
+		// Threshold is two, so any two ayes will pass the vote
+		let threshold = 2;
+
+		// Add proposals
+		let mut last_hash = T::Hash::default();
+		for i in 0 .. p {
+			// Proposals should be different so that different proposal hashes are generated
+			let proposal: T::Proposal = frame_system::Call::<T>::remark(vec![i as u8; b as usize]).into();
+			Collective::<T, _>::propose(SystemOrigin::Signed(caller.clone()).into(), threshold, Box::new(proposal.clone()))?;
+			last_hash = T::Hashing::hash_of(&proposal);
+		}
+
+		// Have everyone vote nay on last proposal, while keeping it from failing
+		// A few abstainers will be the aye votes needed to pass the vote
+		for j in 2 .. m {
+			let voter = &members[j as usize];
+			let approve = false;
+			Collective::<T, _>::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?;
+		}
+
+		// caller is prime, prime already votes aye by creating the proposal
+		System::<T>::set_block_number(T::BlockNumber::max_value());
+		assert_eq!(Collective::<T, _>::proposals().len(), p as usize);
+
+		// Prime aye will close it as approved
+	}: close(SystemOrigin::Signed(caller), last_hash, p - 1)
+	verify {
+		assert_eq!(Collective::<T, _>::proposals().len(), (p - 1) as usize);
+		assert_last_event::<T, I>(RawEvent::Executed(last_hash, false).into());
+	}
 }
 
 #[cfg(test)]
@@ -199,12 +424,13 @@ mod tests {
 		new_test_ext().execute_with(|| {
 			assert_ok!(test_benchmark_set_members::<Test>());
 			assert_ok!(test_benchmark_execute::<Test>());
-			assert_ok!(test_benchmark_propose::<Test>());
-			assert_ok!(test_benchmark_propose_else_branch::<Test>());
-			assert_ok!(test_benchmark_vote::<Test>());
-			assert_ok!(test_benchmark_vote_not_approve::<Test>());
+			assert_ok!(test_benchmark_propose_execute::<Test>());
+			assert_ok!(test_benchmark_propose_proposed::<Test>());
+			assert_ok!(test_benchmark_vote_insert::<Test>());
+			assert_ok!(test_benchmark_vote_disapproved::<Test>());
 			assert_ok!(test_benchmark_vote_approved::<Test>());
-			assert_ok!(test_benchmark_close::<Test>());
+			assert_ok!(test_benchmark_close_disapproved::<Test>());
+			assert_ok!(test_benchmark_close_approved::<Test>());
 		});
 	}
 }

substrate/frame/collective/src/lib.rs

@@ -65,7 +65,7 @@ pub trait Trait<I: Instance=DefaultInstance>: frame_system::Trait {
 	type Origin: From<RawOrigin<Self::AccountId, I>>;
 
 	/// The outer call dispatch type.
-	type Proposal: Parameter + Dispatchable<Origin=<Self as Trait<I>>::Origin> + From<Call<Self, I>>;
+	type Proposal: Parameter + Dispatchable<Origin=<Self as Trait<I>>::Origin> + From<frame_system::Call<Self>>;
 
 	/// The outer event type.
 	type Event: From<Event<Self, I>> + Into<<Self as frame_system::Trait>::Event>;

substrate/frame/democracy/src/tests.rs

@@ -21,7 +21,7 @@ use std::cell::RefCell;
 use codec::Encode;
 use frame_support::{
 	impl_outer_origin, impl_outer_dispatch, assert_noop, assert_ok, parameter_types,
-	ord_parameter_types, traits::{Contains, OnInitialize}, weights::Weight,
+	impl_outer_event, ord_parameter_types, traits::{Contains, OnInitialize}, weights::Weight,
 };
 use sp_core::H256;
 use sp_runtime::{
@@ -53,11 +53,25 @@ impl_outer_origin! {
 
 impl_outer_dispatch! {
 	pub enum Call for Test where origin: Origin {
+		frame_system::System,
 		pallet_balances::Balances,
 		democracy::Democracy,
 	}
 }
 
+mod democracy {
+	pub use crate::Event;
+}
+
+impl_outer_event! {
+	pub enum Event for Test {
+		system<T>,
+		pallet_balances<T>,
+		pallet_scheduler<T>,
+		democracy<T>,
+	}
+}
+
 // Workaround for https://github.com/rust-lang/rust/issues/26925 . Remove when sorted.
 #[derive(Clone, Eq, PartialEq, Debug)]
 pub struct Test;
@@ -77,7 +91,7 @@ impl frame_system::Trait for Test {
 	type AccountId = u64;
 	type Lookup = IdentityLookup<Self::AccountId>;
 	type Header = Header;
-	type Event = ();
+	type Event = Event;
 	type BlockHashCount = BlockHashCount;
 	type MaximumBlockWeight = MaximumBlockWeight;
 	type DbWeight = ();
@@ -94,14 +108,14 @@ parameter_types! {
 	pub const MaximumWeight: u32 = 1000000;
 }
 impl pallet_scheduler::Trait for Test {
-	type Event = ();
+	type Event = Event;
 	type Origin = Origin;
 	type Call = Call;
 	type MaximumWeight = MaximumWeight;
 }
 impl pallet_balances::Trait for Test {
 	type Balance = u64;
-	type Event = ();
+	type Event = Event;
 	type DustRemoval = ();
 	type ExistentialDeposit = ExistentialDeposit;
 	type AccountStore = System;
@@ -144,7 +158,7 @@ impl Get<bool> for InstantAllowed {
 }
 impl super::Trait for Test {
 	type Proposal = Call;
-	type Event = ();
+	type Event = Event;
 	type Currency = pallet_balances::Module<Self>;
 	type EnactmentPeriod = EnactmentPeriod;
 	type LaunchPeriod = LaunchPeriod;

substrate/frame/support/src/traits.rs

@@ -208,7 +208,7 @@ pub trait Contains<T: Ord> {
 	///
 	/// **Should be used for benchmarking only!!!**
 	#[cfg(feature = "runtime-benchmarks")]
-	fn add(t: &T) { unimplemented!() }
+	fn add(_t: &T) { unimplemented!() }
 }
 
 /// Determiner to say whether a given account is unused.

substrate/frame/timestamp/src/benchmarking.rs

@@ -21,18 +21,31 @@
 use super::*;
 use sp_std::prelude::*;
 use frame_system::RawOrigin;
+use frame_support::{ensure, traits::OnFinalize};
 use frame_benchmarking::benchmarks;
 
+use crate::Module as Timestamp;
+
 const MAX_TIME: u32 = 100;
 
 benchmarks! {
-	_ {
-		let n in 1 .. MAX_TIME => ();
-	}
+	_ { }
 
 	set {
-		let n in ...;
-	}: _(RawOrigin::None, n.into())
+		let t in 1 .. MAX_TIME;
+	}: _(RawOrigin::None, t.into())
+	verify {
+		ensure!(Timestamp::<T>::now() == t.into(), "Time was not set.");
+	}
+
+	on_finalize {
+		let t in 1 .. MAX_TIME;
+		Timestamp::<T>::set(RawOrigin::None.into(), t.into())?;
+		ensure!(DidUpdate::exists(), "Time was not set.");
+	}: { Timestamp::<T>::on_finalize(t.into()); }
+	verify {
+		ensure!(!DidUpdate::exists(), "Time was not removed.");
+	}
 }
 
 #[cfg(test)]
@@ -45,6 +58,7 @@ mod tests {
 	fn test_benchmarks() {
 		new_test_ext().execute_with(|| {
 			assert_ok!(test_benchmark_set::<Test>());
+			assert_ok!(test_benchmark_on_finalize::<Test>());
 		});
 	}
 }

substrate/frame/vesting/src/benchmarking.rs

@@ -21,108 +21,177 @@
 use super::*;
 
 use frame_system::{RawOrigin, Module as System};
-use sp_io::hashing::blake2_256;
 use frame_benchmarking::{benchmarks, account};
+use sp_runtime::traits::Bounded;
 
 use crate::Module as Vesting;
 
 const SEED: u32 = 0;
 const MAX_LOCKS: u32 = 20;
 
-fn add_locks<T: Trait>(l: u32) {
-	for id in 0..l {
-		let lock_id = <[u8; 8]>::decode(&mut &id.using_encoded(blake2_256)[..])
-			.unwrap_or_default();
-		let locker = account("locker", 0, SEED);
-		let locked = 1;
+type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::Balance;
+
+fn add_locks<T: Trait>(who: &T::AccountId, n: u8) {
+	for id in 0..n {
+		let lock_id = [id; 8];
+		let locked = 100;
 		let reasons = WithdrawReason::Transfer | WithdrawReason::Reserve;
-		T::Currency::set_lock(lock_id, &locker, locked.into(), reasons);
+		T::Currency::set_lock(lock_id, who, locked.into(), reasons);
 	}
 }
 
-fn setup<T: Trait>(b: u32) -> T::AccountId {
-		let locked = 1;
-		let per_block = 1;
-		let starting_block = 0;
+fn add_vesting_schedule<T: Trait>(who: &T::AccountId) -> Result<(), &'static str> {
+	let locked = 100;
+	let per_block = 10;
+	let starting_block = 1;
 
-		let caller = account("caller", 0, SEED);
-		System::<T>::set_block_number(0.into());
+	System::<T>::set_block_number(0.into());
 
-		// Add schedule to avoid `NotVesting` error.
-		let _ = Vesting::<T>::add_vesting_schedule(
-			&caller,
-			locked.into(),
-			per_block.into(),
-			starting_block.into(),
-		);
-
-		// Set lock and block number to take different code paths.
-		let reasons = WithdrawReason::Transfer | WithdrawReason::Reserve;
-		T::Currency::set_lock(VESTING_ID, &caller, locked.into(), reasons);
-		System::<T>::set_block_number(b.into());
-
-		caller
+	// Add schedule to avoid `NotVesting` error.
+	Vesting::<T>::add_vesting_schedule(
+		&who,
+		locked.into(),
+		per_block.into(),
+		starting_block.into(),
+	)?;
+	Ok(())
 }
 
 benchmarks! {
-	_ {
-		// Number of previous locks.
-		// It doesn't seems to influence the timings for lower values.
-		let l in 0 .. MAX_LOCKS => add_locks::<T>(l);
-	}
+	_ { }
 
 	vest_locked {
-		let l in ...;
+		let l in 0 .. MAX_LOCKS;
 
-		let caller = setup::<T>(0u32);
+		let caller = account("caller", 0, SEED);
+		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+		add_locks::<T>(&caller, l as u8);
+		add_vesting_schedule::<T>(&caller)?;
+		// At block zero, everything is vested.
+		System::<T>::set_block_number(T::BlockNumber::zero());
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&caller),
+			Some(100.into()),
+			"Vesting schedule not added",
+		);
+	}: vest(RawOrigin::Signed(caller.clone()))
+	verify {
+		// Nothing happened since everything is still vested.
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&caller),
+			Some(100.into()),
+			"Vesting schedule was removed",
+		);
+	}
 
-	}: vest(RawOrigin::Signed(caller))
+	vest_unlocked {
+		let l in 0 .. MAX_LOCKS;
 
-	vest_not_locked {
-		let l in ...;
-
-		let caller = setup::<T>(1u32);
-
-	}: vest(RawOrigin::Signed(caller))
+		let caller = account("caller", 0, SEED);
+		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+		add_locks::<T>(&caller, l as u8);
+		add_vesting_schedule::<T>(&caller)?;
+		// At block 20, everything is unvested.
+		System::<T>::set_block_number(20.into());
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&caller),
+			Some(BalanceOf::<T>::zero()),
+			"Vesting schedule still active",
+		);
+	}: vest(RawOrigin::Signed(caller.clone()))
+	verify {
+		// Vesting schedule is removed!
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&caller),
+			None,
+			"Vesting schedule was not removed",
+		);
+	}
 
 	vest_other_locked {
-		let l in ...;
+		let l in 0 .. MAX_LOCKS;
 
-		let other: T::AccountId = setup::<T>(0u32);
+		let other: T::AccountId = account("other", 0, SEED);
 		let other_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(other.clone());
+		T::Currency::make_free_balance_be(&other, BalanceOf::<T>::max_value());
+		add_locks::<T>(&other, l as u8);
+		add_vesting_schedule::<T>(&other)?;
+		// At block zero, everything is vested.
+		System::<T>::set_block_number(T::BlockNumber::zero());
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&other),
+			Some(100.into()),
+			"Vesting schedule not added",
+		);
 
-		let caller = account("caller", 0, SEED);
+		let caller: T::AccountId = account("caller", 0, SEED);
+	}: vest_other(RawOrigin::Signed(caller.clone()), other_lookup)
+	verify {
+		// Nothing happened since everything is still vested.
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&other),
+			Some(100.into()),
+			"Vesting schedule was removed",
+		);
+	}
 
-	}: vest_other(RawOrigin::Signed(caller), other_lookup)
+	vest_other_unlocked {
+		let l in 0 .. MAX_LOCKS;
 
-	vest_other_not_locked {
-		let l in ...;
-
-		let other: T::AccountId = setup::<T>(1u32);
+		let other: T::AccountId = account("other", 0, SEED);
 		let other_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(other.clone());
+		T::Currency::make_free_balance_be(&other, BalanceOf::<T>::max_value());
+		add_locks::<T>(&other, l as u8);
+		add_vesting_schedule::<T>(&other)?;
+		// At block 20, everything is unvested.
+		System::<T>::set_block_number(20.into());
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&other),
+			Some(BalanceOf::<T>::zero()),
+			"Vesting schedule still active",
+		);
 
-		let caller = account("caller", 0, SEED);
-
-	}: vest_other(RawOrigin::Signed(caller), other_lookup)
+		let caller: T::AccountId = account("caller", 0, SEED);
+	}: vest_other(RawOrigin::Signed(caller.clone()), other_lookup)
+	verify {
+		// Vesting schedule is removed!
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&other),
+			None,
+			"Vesting schedule was not removed",
+		);
+	}
 
 	vested_transfer {
-		let u in 0 .. 1000;
+		let l in 0 .. MAX_LOCKS;
 
-		let from = account("from", u, SEED);
-		let to = account("to", u, SEED);
-		let to_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(to);
+		let caller: T::AccountId = account("caller", 0, SEED);
+		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+		let target: T::AccountId = account("target", 0, SEED);
+		let target_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(target.clone());
+		// Give target existing locks
+		add_locks::<T>(&target, l as u8);
 
 		let transfer_amount = T::MinVestedTransfer::get();
 
 		let vesting_schedule = VestingInfo {
 			locked: transfer_amount,
-			per_block: 1.into(),
-			starting_block: 0.into(),
+			per_block: 10.into(),
+			starting_block: 1.into(),
 		};
-
-		let _ = T::Currency::make_free_balance_be(&from, transfer_amount * 10.into());
-
-	}: _(RawOrigin::Signed(from), to_lookup, vesting_schedule)
+	}: _(RawOrigin::Signed(caller), target_lookup, vesting_schedule)
+	verify {
+		assert_eq!(
+			T::MinVestedTransfer::get(),
+			T::Currency::free_balance(&target),
+			"Transfer didn't happen",
+		);
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&target),
+			Some(T::MinVestedTransfer::get()),
+			"Lock not created",
+		);
+	}
 }
 
 #[cfg(test)]
@@ -135,9 +204,9 @@ mod tests {
 	fn test_benchmarks() {
 		ExtBuilder::default().existential_deposit(256).build().execute_with(|| {
 			assert_ok!(test_benchmark_vest_locked::<Test>());
-			assert_ok!(test_benchmark_vest_not_locked::<Test>());
+			assert_ok!(test_benchmark_vest_unlocked::<Test>());
 			assert_ok!(test_benchmark_vest_other_locked::<Test>());
-			assert_ok!(test_benchmark_vest_other_not_locked::<Test>());
+			assert_ok!(test_benchmark_vest_other_unlocked::<Test>());
 			assert_ok!(test_benchmark_vested_transfer::<Test>());
 		});
 	}

substrate/utils/frame/benchmarking-cli/src/command.rs

@@ -92,6 +92,9 @@ impl BenchmarkCmd {
 					self.repeat,
 				);
 
+				// Skip raw data + analysis if there are no results
+				if batch.results.len() == 0 { continue }
+
 				if self.raw_data {
 					// Print the table header
 					batch.results[0].0.iter().for_each(|param| print!("{:?},", param.0));