Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Better runtime tests
  • Loading branch information
kianenigma committed Oct 18, 2019
commit 734bfd8454e59ab8e302d4939a4e0197fd7f850c
6 changes: 6 additions & 0 deletions core/sr-primitives/src/sr_arithmetic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,12 @@ impl Fixed64 {
Self(int.saturating_mul(DIV))
}

/// Return the inner value. Only for testing.
#[cfg(any(feature = "std", test))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is only for testing, then why not ?

Suggested change
#[cfg(any(feature = "std", test))]
#[cfg(test)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point and I (naively) used the gating feature that I used here https://github.com/paritytech/substrate/pull/3823/files#diff-24bd3cac4cae1c620f82f7aa79ef28ceR707

Declaring the function with just #[cfg(test)] was not enough and it cannot be found in tests though 🤔 and I had to make it test or std. Do you know the reason of the difference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that logically it should be just cfg[test]. Something is wrong maybe in cargo files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh actually this is because the test configure is only set for the tested crate.
if you test the runtime then sr-arithmetic isn't compiled with test.

I don't know what is the prefered way to solve this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(won't work as discussed on off-band. will leave the comment open)

pub fn into_inner(self) -> i64 {
self.0
}

/// Return the accuracy of the type. Given that this function returns the value `X`, it means
/// that an instance composed of `X` parts (`Fixed64::from_parts(X)`) is equal to `1`.
pub fn accuracy() -> i64 {
Expand Down
2 changes: 0 additions & 2 deletions node/executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -480,8 +480,6 @@ mod tests {
).0.unwrap();

t.execute_with(|| {
// NOTE: fees differ slightly in tests that execute more than one block due to the
// weight update. Hence, using `assert_eq_error_rate`.
assert_eq!(
Balances::total_balance(&alice()),
alice_last_known_balance - 10 * DOLLARS - transfer_fee(&xt(), fm),
Expand Down
45 changes: 24 additions & 21 deletions node/runtime/src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use sr_primitives::{Fixed64, Perbill};
use support::traits::{OnUnbalanced, Currency, Get};
use crate::{
Balances, System, Authorship, MaximumBlockWeight, NegativeImbalance, WeightToFee,
TargetedFeeAdjustment, TransactionPayment,
TargetedFeeAdjustment,
};

pub struct Author;
Expand Down Expand Up @@ -58,7 +58,7 @@ impl Convert<Weight, Balance> for WeightToFee {

/// Update the given multiplier based on the following formula
///
/// diff = (target_weight - current_block_weight)
/// diff = (target_weight - previous_block_weight)
/// v = 0.00004
/// next_weight = weight * (1 + (v . diff) + (v . diff)^2 / 2)
///
Expand Down Expand Up @@ -112,8 +112,9 @@ impl Convert<Fixed64, Fixed64> for TargetedFeeAdjustment {
mod tests {
use super::*;
use sr_primitives::weights::Weight;
use sr_primitives::assert_eq_error_rate;
use crate::{MaximumBlockWeight, AvailableBlockRatio, Runtime};
use crate::constants::currency::*;
use crate::{constants::currency::*, TransactionPayment};

fn max() -> Weight {
MaximumBlockWeight::get()
Expand All @@ -135,8 +136,7 @@ mod tests {
// Current saturation in terms of weight
let s = block_weight;

let fm = (v * (s/m - ss/m)) + (v.powi(2) * (s/m - ss/m).powi(2)) / 2.0;
dbg!(fm * 1_000_000_000_f32);
let fm = v * (s/m - ss/m) + v.powi(2) * (s/m - ss/m).powi(2) / 2.0;
let addition_fm = Fixed64::from_parts((fm * 1_000_000_000_f32).round() as i64);
previous.saturating_add(addition_fm)
}
Expand All @@ -158,21 +158,18 @@ mod tests {
fn fee_multiplier_update_poc_works() {
let fm = Fixed64::from_rational(0, 1);
let test_set = vec![
// TODO: this has a rounding error and fails.
// (0, fm.clone()),
(0, fm.clone()),
(100, fm.clone()),
(target(), fm.clone()),
(max() / 2, fm.clone()),
(max(), fm.clone()),
];
test_set.into_iter().for_each(|(w, fm)| {
run_with_system_weight(w, || {
assert_eq!(
fee_multiplier_update(w, fm),
TargetedFeeAdjustment::convert(fm),
"failed for weight {} and prev fm {:?}",
w,
fm,
assert_eq_error_rate!(
fee_multiplier_update(w, fm).into_inner(),
TargetedFeeAdjustment::convert(fm).into_inner(),
5,
);
})
})
Expand All @@ -181,7 +178,7 @@ mod tests {
#[test]
fn empty_chain_simulation() {
// just a few txs per_block.
let block_weight = 1000;
let block_weight = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it on purpose that block weight changed from 1000 to 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah just makes a bit more sense: the test is supposed to simulate an empty chain. Nonetheless the test is not that important as you see and just runs a simulation.

run_with_system_weight(block_weight, || {
let mut fm = Fixed64::default();
let mut iterations: u64 = 0;
Expand All @@ -192,6 +189,13 @@ mod tests {
iterations += 1;
}
println!("iteration {}, new fm = {:?}. Weight fee is now zero", iterations, fm);
assert!(iterations > 50_000, "This assertion is just a warning; Don't panic. \
Current substrate/polkadot node are configured with a _slow adjusting fee_ \
mechanism. Hence, it is really unlikely that fees collapse to zero even on an empty chain \
in less than at least of couple of thousands of empty blocks. But this simulation
indicates that fees collapsed to zero after {} almost-empty blocks. Check it",
iterations,
);
})
}

Expand All @@ -201,12 +205,10 @@ mod tests {
// `cargo test congested_chain_simulation -- --nocapture` to get some insight.

// almost full. The entire quota of normal transactions is taken.
let block_weight = AvailableBlockRatio::get() * max();
let block_weight = AvailableBlockRatio::get() * max() - 100;

// default minimum substrate weight
let tx_weight = 10_000u32;
let tx_per_block = 100;
let block_weight = tx_weight * tx_per_block;
// Default substrate minimum.
let tx_weight = 10_000;

run_with_system_weight(block_weight, || {
// initial value configured on module
Expand All @@ -216,7 +218,8 @@ mod tests {
let mut iterations: u64 = 0;
loop {
let next = TargetedFeeAdjustment::convert(fm);
if fm == next { break; }
// if no change, panic. This should never happen in this case.
if fm == next { panic!("The fee should ever increase"); }
fm = next;
iterations += 1;
let fee = <Runtime as transaction_payment::Trait>::WeightToFee::convert(tx_weight);
Expand Down Expand Up @@ -337,7 +340,7 @@ mod tests {
run_with_system_weight(i, || {
let next = TargetedFeeAdjustment::convert(Fixed64::default());
let truth = fee_multiplier_update(i, Fixed64::default());
assert_eq!(next, truth);
assert_eq_error_rate!(truth.into_inner(), next.into_inner(), 5);
});
});

Expand Down