Change assert(is_err()) to assert_noop to check state consistency on …

…errors (paritytech#8587)

* Change is_err() asserts in tests to assert_noop to check state consistency

fixes paritytech#8545

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Update frame/contracts/src/exec.rs

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Update frame/democracy/src/benchmarking.rs

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Update frame/transaction-payment/src/lib.rs

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* Don't assert no-changing state.

see: paritytech#8587 (comment)

* fix expected error

* Fix non-extrinsic-call asserts

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

frame/contracts/src/exec.rs

@@ -937,14 +937,15 @@ mod tests {
 	use super::*;
 	use crate::{
 		gas::GasMeter, tests::{ExtBuilder, Test, Event as MetaEvent},
-		storage::Storage,
+		storage::{Storage, ContractAbsentError},
 		tests::{
 			ALICE, BOB, CHARLIE,
 			test_utils::{place_contract, set_balance, get_balance},
 		},
 		exec::ExportedFunction::*,
 		Error, Weight, CurrentSchedule,
 	};
+	use frame_support::assert_noop;
 	use sp_runtime::DispatchError;
 	use assert_matches::assert_matches;
 	use std::{cell::RefCell, collections::HashMap, rc::Rc};
@@ -1571,7 +1572,10 @@ mod tests {
 			);
 
 			// Check that the account has not been created.
-			assert!(Storage::<Test>::code_hash(&instantiated_contract_address).is_err());
+			assert_noop!(
+				Storage::<Test>::code_hash(&instantiated_contract_address),
+				ContractAbsentError,
+			);
 			assert!(events().is_empty());
 		});
 	}

frame/democracy/src/benchmarking.rs

@@ -21,8 +21,9 @@ use super::*;
 
 use frame_benchmarking::{benchmarks, account, whitelist_account, impl_benchmark_test_suite};
 use frame_support::{
-	IterableStorageMap,
-	traits::{Currency, Get, EnsureOrigin, OnInitialize, UnfilteredDispatchable, schedule::DispatchTime},
+	assert_noop, assert_ok, IterableStorageMap,
+	traits::{Currency, Get, EnsureOrigin, OnInitialize, UnfilteredDispatchable,
+        schedule::DispatchTime},
 };
 use frame_system::{RawOrigin, Pallet as System, self, EventRecord};
 use sp_runtime::traits::{Bounded, One};
@@ -206,11 +207,14 @@ benchmarks! {
 		let origin = T::CancellationOrigin::successful_origin();
 		let referendum_index = add_referendum::<T>(0)?;
 		let call = Call::<T>::emergency_cancel(referendum_index);
-		assert!(Democracy::<T>::referendum_status(referendum_index).is_ok());
+		assert_ok!(Democracy::<T>::referendum_status(referendum_index));
 	}: { call.dispatch_bypass_filter(origin)? }
 	verify {
 		// Referendum has been canceled
-		assert!(Democracy::<T>::referendum_status(referendum_index).is_err());
+		assert_noop!(
+			Democracy::<T>::referendum_status(referendum_index),
+			Error::<T>::ReferendumInvalid,
+		);
 	}
 
 	blacklist {
@@ -224,18 +228,23 @@ benchmarks! {
 
 		// Place our proposal in the external queue, too.
 		let hash = T::Hashing::hash_of(&0);
-		assert!(Democracy::<T>::external_propose(T::ExternalOrigin::successful_origin(), hash.clone()).is_ok());
+		assert_ok!(
+            Democracy::<T>::external_propose(T::ExternalOrigin::successful_origin(), hash.clone())
+        );
 
 		// Add a referendum of our proposal.
 		let referendum_index = add_referendum::<T>(0)?;
-		assert!(Democracy::<T>::referendum_status(referendum_index).is_ok());
+		assert_ok!(Democracy::<T>::referendum_status(referendum_index));
 
 		let call = Call::<T>::blacklist(hash, Some(referendum_index));
 		let origin = T::BlacklistOrigin::successful_origin();
 	}: { call.dispatch_bypass_filter(origin)? }
 	verify {
 		// Referendum has been canceled
-		assert!(Democracy::<T>::referendum_status(referendum_index).is_err());
+		assert_noop!(
+            Democracy::<T>::referendum_status(referendum_index),
+            Error::<T>::ReferendumInvalid
+        );
 	}
 
 	// Worst case scenario, we external propose a previously blacklisted proposal

frame/democracy/src/tests/decoders.rs

@@ -58,12 +58,12 @@ fn pre_image() {
 		let key = Default::default();
 		let missing = PreimageStatus::Missing(0);
 		Preimages::<Test>::insert(key, missing);
-		assert!(Democracy::pre_image_data_len(key).is_err());
+		assert_noop!(Democracy::pre_image_data_len(key), Error::<Test>::PreimageMissing);
 		assert_eq!(Democracy::check_pre_image_is_missing(key), Ok(()));
 
 		Preimages::<Test>::remove(key);
-		assert!(Democracy::pre_image_data_len(key).is_err());
-		assert!(Democracy::check_pre_image_is_missing(key).is_err());
+		assert_noop!(Democracy::pre_image_data_len(key), Error::<Test>::PreimageMissing);
+		assert_noop!(Democracy::check_pre_image_is_missing(key), Error::<Test>::NotImminent);
 
 		for l in vec![0, 10, 100, 1000u32] {
 			let available = PreimageStatus::Available{
@@ -76,7 +76,8 @@ fn pre_image() {
 
 			Preimages::<Test>::insert(key, available);
 			assert_eq!(Democracy::pre_image_data_len(key), Ok(l));
-			assert!(Democracy::check_pre_image_is_missing(key).is_err());
+			assert_noop!(Democracy::check_pre_image_is_missing(key),
+				Error::<Test>::DuplicatePreimage);
 		}
 	})
 }

frame/democracy/src/tests/external_proposing.rs

@@ -93,7 +93,7 @@ fn external_blacklisting_should_work() {
 		assert_ok!(Democracy::blacklist(Origin::root(), hash, None));
 
 		fast_forward_to(2);
-		assert!(Democracy::referendum_status(0).is_err());
+		assert_noop!(Democracy::referendum_status(0), Error::<Test>::ReferendumInvalid);
 
 		assert_noop!(
 			Democracy::external_propose(

frame/democracy/src/tests/public_proposals.rs

@@ -129,9 +129,9 @@ fn blacklisting_should_work() {
 		fast_forward_to(2);
 
 		let hash = set_balance_proposal_hash(4);
-		assert!(Democracy::referendum_status(0).is_ok());
+		assert_ok!(Democracy::referendum_status(0));
 		assert_ok!(Democracy::blacklist(Origin::root(), hash, Some(0)));
-		assert!(Democracy::referendum_status(0).is_err());
+		assert_noop!(Democracy::referendum_status(0), Error::<Test>::ReferendumInvalid);
 	});
 }
 

frame/democracy/src/tests/voting.rs

@@ -80,12 +80,12 @@ fn single_proposal_should_work() {
 		fast_forward_to(3);
 
 		// referendum still running
-		assert!(Democracy::referendum_status(0).is_ok());
+		assert_ok!(Democracy::referendum_status(0));
 
 		// referendum runs during 2 and 3, ends @ start of 4.
 		fast_forward_to(4);
 
-		assert!(Democracy::referendum_status(0).is_err());
+		assert_noop!(Democracy::referendum_status(0), Error::<Test>::ReferendumInvalid);
 		assert!(pallet_scheduler::Agenda::<Test>::get(6)[0].is_some());
 
 		// referendum passes and wait another two blocks for enactment.

frame/election-provider-multi-phase/src/unsigned.rs

@@ -832,20 +832,20 @@ mod tests {
 			assert!(MultiPhase::try_acquire_offchain_lock(25).is_ok());
 
 			// next block: rejected.
-			assert!(MultiPhase::try_acquire_offchain_lock(26).is_err());
+			assert_noop!(MultiPhase::try_acquire_offchain_lock(26), "recently executed.");
 
 			// allowed after `OFFCHAIN_REPEAT`
 			assert!(MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT).into()).is_ok());
 
 			// a fork like situation: re-execute last 3.
-			assert!(
-				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 3).into()).is_err()
+			assert_noop!(
+				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 3).into()), "fork."
 			);
-			assert!(
-				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 2).into()).is_err()
+			assert_noop!(
+				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 2).into()), "fork."
 			);
-			assert!(
-				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 1).into()).is_err()
+			assert_noop!(
+				MultiPhase::try_acquire_offchain_lock((26 + OFFCHAIN_REPEAT - 1).into()), "fork."
 			);
 		})
 	}

frame/executive/src/lib.rs

@@ -535,7 +535,7 @@ mod tests {
 		},
 	};
 	use frame_support::{
-		parameter_types,
+		assert_err, parameter_types,
 		weights::{Weight, RuntimeDbWeight, IdentityFee, WeightToFeePolynomial},
 		traits::{Currency, LockIdentifier, LockableCurrency, WithdrawReasons},
 	};
@@ -889,7 +889,9 @@ mod tests {
 				[69u8; 32].into(),
 				Digest::default(),
 			));
-			assert!(Executive::apply_extrinsic(xt).is_err());
+			assert_err!(Executive::apply_extrinsic(xt),
+				TransactionValidityError::Invalid(InvalidTransaction::Future)
+			);
 			assert_eq!(<frame_system::Pallet<Runtime>>::extrinsic_index(), Some(0));
 		});
 	}

frame/grandpa/src/tests.rs

@@ -24,7 +24,7 @@ use crate::mock::*;
 use codec::{Decode, Encode};
 use fg_primitives::ScheduledChange;
 use frame_support::{
-	assert_err, assert_ok,
+	assert_err, assert_ok, assert_noop,
 	traits::{Currency, OnFinalize, OneSessionHandler},
 	weights::{GetDispatchInfo, Pays},
 };
@@ -100,21 +100,27 @@ fn cannot_schedule_change_when_one_pending() {
 		initialize_block(1, Default::default());
 		Grandpa::schedule_change(to_authorities(vec![(4, 1), (5, 1), (6, 1)]), 1, None).unwrap();
 		assert!(<PendingChange<Test>>::exists());
-		assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None).is_err());
+		assert_noop!(
+			Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None),
+			Error::<Test>::ChangePending
+		);
 
 		Grandpa::on_finalize(1);
 		let header = System::finalize();
 
 		initialize_block(2, header.hash());
 		assert!(<PendingChange<Test>>::exists());
-		assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None).is_err());
+		assert_noop!(
+			Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None),
+			Error::<Test>::ChangePending
+		);
 
 		Grandpa::on_finalize(2);
 		let header = System::finalize();
 
 		initialize_block(3, header.hash());
 		assert!(!<PendingChange<Test>>::exists());
-		assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None).is_ok());
+		assert_ok!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None));
 
 		Grandpa::on_finalize(3);
 		let _header = System::finalize();
@@ -148,7 +154,10 @@ fn dispatch_forced_change() {
 		).unwrap();
 
 		assert!(<PendingChange<Test>>::exists());
-		assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, Some(0)).is_err());
+		assert_noop!(
+			Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, Some(0)),
+			Error::<Test>::ChangePending
+		);
 
 		Grandpa::on_finalize(1);
 		let mut header = System::finalize();
@@ -157,8 +166,14 @@ fn dispatch_forced_change() {
 			initialize_block(i, header.hash());
 			assert!(<PendingChange<Test>>::get().unwrap().forced.is_some());
 			assert_eq!(Grandpa::next_forced(), Some(11));
-			assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None).is_err());
-			assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, Some(0)).is_err());
+			assert_noop!(
+				Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None),
+				Error::<Test>::ChangePending
+			);
+			assert_noop!(
+				Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, Some(0)),
+				Error::<Test>::ChangePending
+			);
 
 			Grandpa::on_finalize(i);
 			header = System::finalize();
@@ -170,7 +185,7 @@ fn dispatch_forced_change() {
 			initialize_block(7, header.hash());
 			assert!(!<PendingChange<Test>>::exists());
 			assert_eq!(Grandpa::grandpa_authorities(), to_authorities(vec![(4, 1), (5, 1), (6, 1)]));
-			assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None).is_ok());
+			assert_ok!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None));
 			Grandpa::on_finalize(7);
 			header = System::finalize();
 		}
@@ -180,7 +195,10 @@ fn dispatch_forced_change() {
 			initialize_block(8, header.hash());
 			assert!(<PendingChange<Test>>::exists());
 			assert_eq!(Grandpa::grandpa_authorities(), to_authorities(vec![(4, 1), (5, 1), (6, 1)]));
-			assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None).is_err());
+			assert_noop!(
+				Grandpa::schedule_change(to_authorities(vec![(5, 1)]), 1, None),
+				Error::<Test>::ChangePending
+			);
 			Grandpa::on_finalize(8);
 			header = System::finalize();
 		}
@@ -192,15 +210,18 @@ fn dispatch_forced_change() {
 			assert!(!<PendingChange<Test>>::exists());
 			assert_eq!(Grandpa::grandpa_authorities(), to_authorities(vec![(5, 1)]));
 			assert_eq!(Grandpa::next_forced(), Some(11));
-			assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1), (6, 1)]), 5, Some(0)).is_err());
+			assert_noop!(
+				Grandpa::schedule_change(to_authorities(vec![(5, 1), (6, 1)]), 5, Some(0)),
+				Error::<Test>::TooSoon
+			);
 			Grandpa::on_finalize(i);
 			header = System::finalize();
 		}
 
 		{
 			initialize_block(11, header.hash());
 			assert!(!<PendingChange<Test>>::exists());
-			assert!(Grandpa::schedule_change(to_authorities(vec![(5, 1), (6, 1), (7, 1)]), 5, Some(0)).is_ok());
+			assert_ok!(Grandpa::schedule_change(to_authorities(vec![(5, 1), (6, 1), (7, 1)]), 5, Some(0)));
 			assert_eq!(Grandpa::next_forced(), Some(21));
 			Grandpa::on_finalize(11);
 			header = System::finalize();
@@ -231,7 +252,7 @@ fn schedule_pause_only_when_live() {
 		initialize_block(2, Default::default());
 
 		// signaling a pause now should fail
-		assert!(Grandpa::schedule_pause(1).is_err());
+		assert_noop!(Grandpa::schedule_pause(1), Error::<Test>::PauseFailed);
 
 		Grandpa::on_finalize(2);
 		let _ = System::finalize();
@@ -250,7 +271,7 @@ fn schedule_resume_only_when_paused() {
 		initialize_block(1, Default::default());
 
 		// the set is currently live, resuming it is an error
-		assert!(Grandpa::schedule_resume(1).is_err());
+		assert_noop!(Grandpa::schedule_resume(1), Error::<Test>::ResumeFailed);
 
 		assert_eq!(
 			Grandpa::state(),

frame/system/src/extensions/check_nonce.rs

@@ -120,6 +120,7 @@ impl<T: Config> SignedExtension for CheckNonce<T> where
 mod tests {
 	use super::*;
 	use crate::mock::{Test, new_test_ext, CALL};
+	use frame_support::{assert_noop, assert_ok};
 
 	#[test]
 	fn signed_ext_check_nonce_works() {
@@ -134,14 +135,23 @@ mod tests {
 			let info = DispatchInfo::default();
 			let len = 0_usize;
 			// stale
-			assert!(CheckNonce::<Test>(0).validate(&1, CALL, &info, len).is_err());
-			assert!(CheckNonce::<Test>(0).pre_dispatch(&1, CALL, &info, len).is_err());
+			assert_noop!(
+				CheckNonce::<Test>(0).validate(&1, CALL, &info, len),
+				InvalidTransaction::Stale
+			);
+			assert_noop!(
+				CheckNonce::<Test>(0).pre_dispatch(&1, CALL, &info, len),
+				InvalidTransaction::Stale
+			);
 			// correct
-			assert!(CheckNonce::<Test>(1).validate(&1, CALL, &info, len).is_ok());
-			assert!(CheckNonce::<Test>(1).pre_dispatch(&1, CALL, &info, len).is_ok());
+			assert_ok!(CheckNonce::<Test>(1).validate(&1, CALL, &info, len));
+			assert_ok!(CheckNonce::<Test>(1).pre_dispatch(&1, CALL, &info, len));
 			// future
-			assert!(CheckNonce::<Test>(5).validate(&1, CALL, &info, len).is_ok());
-			assert!(CheckNonce::<Test>(5).pre_dispatch(&1, CALL, &info, len).is_err());
+			assert_ok!(CheckNonce::<Test>(5).validate(&1, CALL, &info, len));
+			assert_noop!(
+				CheckNonce::<Test>(5).pre_dispatch(&1, CALL, &info, len),
+				InvalidTransaction::Future
+			);
 		})
 	}
 }