Substitute mock-accountant macro by a mock builder pallet #1652
Conversation
lemunozm
added
P2-nice-to-have
lemunozm
force-pushed
the
remove-mock-accountant
branch
from
85a0d86 to
851d435
Compare
|
Requires foss3/runtime-pallet-library#15 to work |
| #[allow(non_snake_case)] | ||
| pub fn mock_InvestmentAccountant_deposit( | ||
| f: impl Fn(&T::AccountId, T::TrancheCurrency, T::Balance) -> DispatchResult + 'static, | ||
| ) { | ||
| register_call!(move |(a, b, c)| f(a, b, c)); | ||
| } | ||
|
|
||
| #[allow(non_snake_case)] | ||
| pub fn mock_InvestmentAccountant_withdraw( | ||
| f: impl Fn(&T::AccountId, T::TrancheCurrency, T::Balance) -> DispatchResult + 'static, | ||
| ) { | ||
| register_call!(move |(a, b, c)| f(a, b, c)); | ||
| } |
There was a problem hiding this comment.
The application of foss3/runtime-pallet-library#16, @wischli
Basically the pallet implements deposit() and withdraw() methods from two different traits. So we need to add mock_<trait>_method() to one of these deposit()/withdraw() to differentiate from the others
In those cases, the T::AccountId used as generic for InvestmentAccountant was wrongly internally coded.
| @@ -1289,11 +1289,11 @@ fn fulfillment_partially_works_low_price() { | |||
| { | |||
| assert_eq!( | |||
| free_balance_of(investment_account(INVESTMENT_0_0), AUSD_CURRENCY_ID), | |||
| 207245508982035928145 | |||
| 207245508982035928140 | |||
There was a problem hiding this comment.
Rounding issues due to modifying Rate (27 decimals) to Quantity (18 decimals).
| @@ -2781,7 +2783,7 @@ fn collecting_fully_works() { | |||
| INVESTMENT_0_0 | |||
| )); | |||
| assert_eq!( | |||
| n_last_event(3), | |||
| n_last_event(4), | |||
There was a problem hiding this comment.
The expected event is behind another produced event of killing an account.
I think the rounding issues put the account under the existential deposit value, and the account is killed.
| #[cfg(test)] | ||
| crate::mock::configure_accountant_mock(); |
There was a problem hiding this comment.
Completely astonished by the needed for this.
Without the above line, the test crashes because the mock closure methods are not defined. Nevertheless, configure_accountant_mock() is already called under TextExternatilitiesBuilder::build(), so I would expect to have already defined the mock methods.
It seems like for benchmarking, somehow, the thread used to build the externality is not the same as the one used to benchmark. And then, the internal thread_local used by the mock builder doesn't correspond to the one used.
There was a problem hiding this comment.
Actually, for testing the 4 benchmarking, just one call to TextExternatilitiesBuilder::build() is done
There was a problem hiding this comment.
Interesting finding.
Description
From the addition of this feature (foss3/runtime-pallet-library#1), the mock accountant macro can be removed, and a mock builder pallet can be used instead.
Changes and Descriptions
RatebyQuantity(both represent the same type, but usingQuantityfor correctness).cfg-test-utils