Change assert(is_err()) to assert_noop to check state consistency on errors (#8587)

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

fixes #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: https://github.com/paritytech/substrate/pull/8587#issuecomment-817137906

* 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>
This commit is contained in:
Falco Hirschenberger
2021-04-13 12:44:27 +02:00
committed by GitHub
parent b9ed6e01b3
commit 24311eee3e
13 changed files with 142 additions and 115 deletions
@@ -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
);
})
}
}
@@ -26,7 +26,7 @@ use sp_runtime::{
DispatchResult,
};
use frame_support::{
traits::{Get},
traits::Get,
weights::{PostDispatchInfo, DispatchInfo, DispatchClass, priority::FrameTransactionPriority},
};
@@ -281,8 +281,7 @@ mod tests {
use crate::{BlockWeight, AllExtrinsicsLen};
use crate::mock::{Test, CALL, new_test_ext, System};
use sp_std::marker::PhantomData;
use frame_support::{assert_ok, assert_noop};
use frame_support::weights::{Weight, Pays};
use frame_support::{assert_err, assert_ok, weights::{Weight, Pays}};
fn block_weights() -> crate::limits::BlockWeights {
<Test as crate::Config>::BlockWeights::get()
@@ -335,11 +334,7 @@ mod tests {
..Default::default()
};
let len = 0_usize;
assert_noop!(
CheckWeight::<Test>::do_validate(&max, len),
InvalidTransaction::ExhaustsResources
);
assert_err!(CheckWeight::<Test>::do_validate(&max, len), InvalidTransaction::ExhaustsResources);
});
}
@@ -371,10 +366,7 @@ mod tests {
..Default::default()
})
);
assert_noop!(
CheckWeight::<Test>::do_validate(&max, len),
InvalidTransaction::ExhaustsResources
);
assert_err!(CheckWeight::<Test>::do_validate(&max, len), InvalidTransaction::ExhaustsResources);
});
}
@@ -437,15 +429,13 @@ mod tests {
let dispatch_operational = DispatchInfo { weight: 251, class: DispatchClass::Operational, ..Default::default() };
let len = 0_usize;
assert_noop!(
CheckWeight::<Test>::do_pre_dispatch(&dispatch_normal, len),
assert_err!( CheckWeight::<Test>::do_pre_dispatch(&dispatch_normal, len),
InvalidTransaction::ExhaustsResources
);
// Thank goodness we can still do an operational transaction to possibly save the blockchain.
assert_ok!(CheckWeight::<Test>::do_pre_dispatch(&dispatch_operational, len));
// Not too much though
assert_noop!(
CheckWeight::<Test>::do_pre_dispatch(&dispatch_operational, len),
assert_err!(CheckWeight::<Test>::do_pre_dispatch(&dispatch_operational, len),
InvalidTransaction::ExhaustsResources
);
// Even with full block, validity of single transaction should be correct.
@@ -466,15 +456,19 @@ mod tests {
current_weight.set(normal_limit, DispatchClass::Normal)
});
// will not fit.
assert!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &normal, len).is_err());
assert_err!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &normal, len),
InvalidTransaction::ExhaustsResources
);
// will fit.
assert!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &op, len).is_ok());
assert_ok!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &op, len));
// likewise for length limit.
let len = 100_usize;
AllExtrinsicsLen::<Test>::put(normal_length_limit());
assert!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &normal, len).is_err());
assert!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &op, len).is_ok());
assert_err!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &normal, len),
InvalidTransaction::ExhaustsResources
);
assert_ok!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &op, len));
})
}
@@ -575,10 +569,7 @@ mod tests {
let pre = CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &info, len).unwrap();
assert_eq!(BlockWeight::<Test>::get().total(), info.weight + 256);
assert!(
CheckWeight::<Test>::post_dispatch(pre, &info, &post_info, len, &Ok(()))
.is_ok()
);
assert_ok!( CheckWeight::<Test>::post_dispatch(pre, &info, &post_info, len, &Ok(())));
assert_eq!(
BlockWeight::<Test>::get().total(),
post_info.actual_weight.unwrap() + 256,
@@ -607,10 +598,7 @@ mod tests {
info.weight + 128 + block_weights().get(DispatchClass::Normal).base_extrinsic,
);
assert!(
CheckWeight::<Test>::post_dispatch(pre, &info, &post_info, len, &Ok(()))
.is_ok()
);
assert_ok!(CheckWeight::<Test>::post_dispatch(pre, &info, &post_info, len, &Ok(())));
assert_eq!(
BlockWeight::<Test>::get().total(),
info.weight + 128 + block_weights().get(DispatchClass::Normal).base_extrinsic,
@@ -630,8 +618,7 @@ mod tests {
System::block_weight().total(),
weights.base_block
);
let r = CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &free, len);
assert!(r.is_ok());
assert_ok!(CheckWeight::<Test>(PhantomData).pre_dispatch(&1, CALL, &free, len));
assert_eq!(
System::block_weight().total(),
weights.get(DispatchClass::Normal).base_extrinsic + weights.base_block
@@ -687,15 +674,14 @@ mod tests {
let mandatory2 = DispatchInfo { weight: 6, class: DispatchClass::Mandatory, ..Default::default() };
// when
let result1 = calculate_consumed_weight::<<Test as Config>::Call>(
maximum_weight.clone(), all_weight.clone(), &mandatory1
assert_ok!(
calculate_consumed_weight::<<Test as Config>::Call>(
maximum_weight.clone(), all_weight.clone(), &mandatory1
)
);
let result2 = calculate_consumed_weight::<<Test as Config>::Call>(
maximum_weight, all_weight, &mandatory2
assert_err!(
calculate_consumed_weight::<<Test as Config>::Call>( maximum_weight, all_weight, &mandatory2),
InvalidTransaction::ExhaustsResources
);
// then
assert!(result2.is_err());
assert!(result1.is_ok());
}
}
+10 -8
View File
@@ -19,7 +19,9 @@ use crate::*;
use mock::{*, Origin};
use sp_core::H256;
use sp_runtime::{DispatchError, DispatchErrorWithPostInfo, traits::{Header, BlakeTwo256}};
use frame_support::{assert_noop, weights::WithPostDispatchInfo, dispatch::PostDispatchInfo};
use frame_support::{
assert_noop, assert_ok, weights::WithPostDispatchInfo, dispatch::PostDispatchInfo
};
#[test]
fn origin_works() {
@@ -31,7 +33,7 @@ fn origin_works() {
#[test]
fn stored_map_works() {
new_test_ext().execute_with(|| {
assert!(System::insert(&0, 42).is_ok());
assert_ok!(System::insert(&0, 42));
assert!(!System::is_provider_required(&0));
assert_eq!(Account::<Test>::get(0), AccountInfo {
@@ -42,17 +44,17 @@ fn stored_map_works() {
data: 42,
});
assert!(System::inc_consumers(&0).is_ok());
assert_ok!(System::inc_consumers(&0));
assert!(System::is_provider_required(&0));
assert!(System::insert(&0, 69).is_ok());
assert_ok!(System::insert(&0, 69));
assert!(System::is_provider_required(&0));
System::dec_consumers(&0);
assert!(!System::is_provider_required(&0));
assert!(KILLED.with(|r| r.borrow().is_empty()));
assert!(System::remove(&0).is_ok());
assert_ok!(System::remove(&0));
assert_eq!(KILLED.with(|r| r.borrow().clone()), vec![0u64]);
});
}
@@ -122,7 +124,7 @@ fn sufficient_cannot_support_consumer() {
assert_noop!(System::inc_consumers(&0), IncRefError::NoProviders);
assert_eq!(System::inc_providers(&0), IncRefStatus::Existed);
assert!(System::inc_consumers(&0).is_ok());
assert_ok!(System::inc_consumers(&0));
assert_noop!(System::dec_providers(&0), DecRefError::ConsumerRemaining);
});
}
@@ -140,7 +142,7 @@ fn provider_required_to_support_consumer() {
assert_eq!(System::dec_providers(&0).unwrap(), DecRefStatus::Exists);
assert_eq!(System::account_nonce(&0), 1);
assert!(System::inc_consumers(&0).is_ok());
assert_ok!(System::inc_consumers(&0));
assert_noop!(System::dec_providers(&0), DecRefError::ConsumerRemaining);
System::dec_consumers(&0);
@@ -516,7 +518,7 @@ fn ensure_one_of_works() {
assert_eq!(ensure_root_or_signed(RawOrigin::Root).unwrap(), Either::Left(()));
assert_eq!(ensure_root_or_signed(RawOrigin::Signed(0)).unwrap(), Either::Right(0));
assert!(ensure_root_or_signed(RawOrigin::None).is_err())
assert!(ensure_root_or_signed(RawOrigin::None).is_err());
}
#[test]