Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improved election pallet testing #12327

Merged
merged 22 commits into from
Oct 4, 2022
Merged

Improved election pallet testing #12327

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f978433
Improved election pallet testing
Szegoo Sep 22, 2022
ef0c223
Merge branch 'master' into election-testing
Szegoo Sep 22, 2022
2f2c8b5
fmt
Szegoo Sep 22, 2022
0494a89
remove comment
Szegoo Sep 22, 2022
d64de36
more checks
Szegoo Sep 22, 2022
0d3478f
fixes in logic
Szegoo Sep 22, 2022
be05c37
roll_to_signed
Szegoo Sep 22, 2022
a0e24ca
switch to roll_to_signed
Szegoo Sep 22, 2022
e861d57
Update frame/election-provider-multi-phase/src/mock.rs
Szegoo Sep 24, 2022
97b30c4
remove useless checks
Szegoo Sep 24, 2022
b2d1859
remove warning
Szegoo Sep 25, 2022
818c404
add checks to signed.rs
Szegoo Oct 3, 2022
956de55
add some checks to unsigned.rs
Szegoo Oct 3, 2022
c8a3b4e
Merge branch 'master' into election-testing
Szegoo Oct 3, 2022
b4c162c
fmt
Szegoo Oct 3, 2022
779e300
use roll_to_signed and roll_to_unsigned
Szegoo Oct 3, 2022
b9a008d
remove nonsense
Szegoo Oct 4, 2022
ac84324
remove even more nonsense
Szegoo Oct 4, 2022
bd128f3
fix
Szegoo Oct 4, 2022
93cc43f
fix
Szegoo Oct 4, 2022
a403504
remove useless checks
Szegoo Oct 4, 2022
7787315
Merge remote-tracking branch 'origin/master' into election-testing
Oct 4, 2022
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
161 changes: 133 additions & 28 deletions frame/election-provider-multi-phase/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1842,9 +1842,9 @@ mod tests {
use super::*;
use crate::{
mock::{
multi_phase_events, raw_solution, roll_to, AccountId, ExtBuilder, MockWeightInfo,
MockedWeightInfo, MultiPhase, Runtime, RuntimeOrigin, SignedMaxSubmissions, System,
TargetIndex, Targets,
multi_phase_events, raw_solution, roll_to, roll_to_signed, roll_to_unsigned, AccountId,
ExtBuilder, MockWeightInfo, MockedWeightInfo, MultiPhase, Runtime, RuntimeOrigin,
SignedMaxSubmissions, System, TargetIndex, Targets,
},
Phase,
};
Expand All @@ -1868,7 +1868,7 @@ mod tests {
assert!(MultiPhase::snapshot().is_none());
assert_eq!(MultiPhase::round(), 1);

roll_to(15);
roll_to_signed();
assert_eq!(MultiPhase::current_phase(), Phase::Signed);
assert_eq!(multi_phase_events(), vec![Event::SignedPhaseStarted { round: 1 }]);
assert!(MultiPhase::snapshot().is_some());
Expand All @@ -1879,7 +1879,7 @@ mod tests {
assert!(MultiPhase::snapshot().is_some());
assert_eq!(MultiPhase::round(), 1);

roll_to(25);
roll_to_unsigned();
Copy link
Contributor

@Ank4n Ank4n Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Referring to line 1877.

I think if we are using this pattern then we should not use the exact block numbers explicitly in the code anymore.
What I mean is, the test should read something like this

roll_to_signed();
some_assertions();

roll_by(9); // instead of roll_to(24), go forward by 9 blocks
assertions_phase_still_signed();

roll_to_unsigned();
some_assertions();

My reasoning for this is, if we still have to keep track of block numbers, it defeats the purpose of using the pattern roll_to_event()

Since we are actively moving forward till a certain phase is reached, we probably don't need this assertions at block 24 anyway. If there is a logical assertion needed here, may be it could be done a bit differently. For example, block_unsigned_phase - block_signed_phase == 10

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @kianenigma

Copy link
Contributor

Choose a reason for hiding this comment

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

The goal is to have as much "separation of concerns" as possible. There is one tests somewhere in there that checks roll_to(x) manually (called phase_rotation_works). All other tests should spend the minimum amount of mental effort to deal with phase related stuff, so I would stick to roll_to_signed and roll_to_unsigned for the sake of simplicity.

Copy link
Contributor

@Ank4n Ank4n Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I am agreeing with the pattern. I agree we can keep manual roll over for this test. There are still few more roll_to() checks that I think we can get rid of.

assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25)));
assert_eq!(
multi_phase_events(),
Expand Down Expand Up @@ -1912,11 +1912,29 @@ mod tests {
roll_to(44);
assert!(MultiPhase::current_phase().is_off());

roll_to(45);
roll_to_signed();
assert!(MultiPhase::current_phase().is_signed());

roll_to(55);
assert!(MultiPhase::current_phase().is_unsigned_open_at(55));

assert_eq!(
multi_phase_events(),
vec![
Event::SignedPhaseStarted { round: 1 },
Event::UnsignedPhaseStarted { round: 1 },
Event::ElectionFinalized {
compute: ElectionCompute::Fallback,
score: ElectionScore {
minimal_stake: 0,
sum_stake: 0,
sum_stake_squared: 0
}
},
Event::SignedPhaseStarted { round: 2 },
Event::UnsignedPhaseStarted { round: 2 }
]
);
})
}

Expand All @@ -1940,6 +1958,21 @@ mod tests {

assert!(MultiPhase::current_phase().is_off());
assert!(MultiPhase::snapshot().is_none());

assert_eq!(
multi_phase_events(),
vec![
Event::UnsignedPhaseStarted { round: 1 },
Event::ElectionFinalized {
compute: ElectionCompute::Fallback,
score: ElectionScore {
minimal_stake: 0,
sum_stake: 0,
sum_stake_squared: 0
}
}
]
);
});
}

Expand All @@ -1952,7 +1985,7 @@ mod tests {
roll_to(19);
assert!(MultiPhase::current_phase().is_off());

roll_to(20);
roll_to_signed();
assert!(MultiPhase::current_phase().is_signed());
assert!(MultiPhase::snapshot().is_some());

Expand All @@ -1963,6 +1996,21 @@ mod tests {

assert!(MultiPhase::current_phase().is_off());
assert!(MultiPhase::snapshot().is_none());

assert_eq!(
multi_phase_events(),
vec![
Event::SignedPhaseStarted { round: 1 },
Event::ElectionFinalized {
compute: ElectionCompute::Fallback,
score: ElectionScore {
minimal_stake: 0,
sum_stake: 0,
sum_stake_squared: 0
}
}
]
)
});
}

Expand All @@ -1985,6 +2033,14 @@ mod tests {
assert_ok!(MultiPhase::elect());

assert!(MultiPhase::current_phase().is_off());

assert_eq!(
multi_phase_events(),
vec![Event::ElectionFinalized {
compute: ElectionCompute::Fallback,
score: ElectionScore { minimal_stake: 0, sum_stake: 0, sum_stake_squared: 0 }
}]
);
});
}

Expand All @@ -1993,16 +2049,13 @@ mod tests {
// An early termination in the signed phase, with no queued solution.
ExtBuilder::default().build_and_execute(|| {
// Signed phase started at block 15 and will end at 25.
roll_to(14);
assert_eq!(MultiPhase::current_phase(), Phase::Off);

roll_to(15);
roll_to_signed();
Szegoo marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(multi_phase_events(), vec![Event::SignedPhaseStarted { round: 1 }]);
assert_eq!(MultiPhase::current_phase(), Phase::Signed);
assert_eq!(MultiPhase::round(), 1);

// An unexpected call to elect.
roll_to(20);
assert_ok!(MultiPhase::elect());

// We surely can't have any feasible solutions. This will cause an on-chain election.
Expand Down Expand Up @@ -2031,10 +2084,8 @@ mod tests {
// an early termination in the signed phase, with no queued solution.
ExtBuilder::default().build_and_execute(|| {
// signed phase started at block 15 and will end at 25.
roll_to(14);
assert_eq!(MultiPhase::current_phase(), Phase::Off);

roll_to(15);
roll_to_signed();
assert_eq!(multi_phase_events(), vec![Event::SignedPhaseStarted { round: 1 }]);
assert_eq!(MultiPhase::current_phase(), Phase::Signed);
assert_eq!(MultiPhase::round(), 1);
Expand All @@ -2052,7 +2103,6 @@ mod tests {
}

// an unexpected call to elect.
roll_to(20);
assert_ok!(MultiPhase::elect());

// all storage items must be cleared.
Expand All @@ -2062,16 +2112,38 @@ mod tests {
assert!(MultiPhase::desired_targets().is_none());
assert!(MultiPhase::queued_solution().is_none());
assert!(MultiPhase::signed_submissions().is_empty());

assert_eq!(
multi_phase_events(),
vec![
Event::SignedPhaseStarted { round: 1 },
Event::SolutionStored { compute: ElectionCompute::Signed, prev_ejected: false },
Event::SolutionStored { compute: ElectionCompute::Signed, prev_ejected: false },
Event::SolutionStored { compute: ElectionCompute::Signed, prev_ejected: false },
Event::SolutionStored { compute: ElectionCompute::Signed, prev_ejected: false },
Event::SolutionStored { compute: ElectionCompute::Signed, prev_ejected: false },
Event::Slashed { account: 99, value: 5 },
Event::Slashed { account: 99, value: 5 },
Event::Slashed { account: 99, value: 5 },
Event::Slashed { account: 99, value: 5 },
Event::Slashed { account: 99, value: 5 },
Event::ElectionFinalized {
compute: ElectionCompute::Fallback,
score: ElectionScore {
minimal_stake: 0,
sum_stake: 0,
sum_stake_squared: 0
}
}
]
);
})
}

#[test]
fn check_events_with_compute_signed() {
ExtBuilder::default().build_and_execute(|| {
roll_to(14);
assert_eq!(MultiPhase::current_phase(), Phase::Off);

roll_to(15);
roll_to_signed();
assert!(MultiPhase::current_phase().is_signed());

let solution = raw_solution();
Expand Down Expand Up @@ -2106,7 +2178,7 @@ mod tests {
#[test]
fn check_events_with_compute_unsigned() {
ExtBuilder::default().build_and_execute(|| {
roll_to(25);
roll_to_unsigned();
assert!(MultiPhase::current_phase().is_unsigned());

// ensure we have snapshots in place.
Expand All @@ -2125,7 +2197,6 @@ mod tests {
));
assert!(MultiPhase::queued_solution().is_some());

roll_to(30);
assert_ok!(MultiPhase::elect());

assert_eq!(
Expand Down Expand Up @@ -2153,7 +2224,7 @@ mod tests {
#[test]
fn fallback_strategy_works() {
ExtBuilder::default().onchain_fallback(true).build_and_execute(|| {
roll_to(25);
roll_to_unsigned();
assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25)));

// Zilch solutions thus far, but we get a result.
Expand All @@ -2166,25 +2237,50 @@ mod tests {
(30, Support { total: 40, voters: vec![(2, 5), (4, 5), (30, 30)] }),
(40, Support { total: 60, voters: vec![(2, 5), (3, 10), (4, 5), (40, 40)] })
]
)
);

assert_eq!(
multi_phase_events(),
vec![
Event::SignedPhaseStarted { round: 1 },
Event::UnsignedPhaseStarted { round: 1 },
Event::ElectionFinalized {
compute: ElectionCompute::Fallback,
score: ElectionScore {
minimal_stake: 0,
sum_stake: 0,
sum_stake_squared: 0
}
}
]
);
});

ExtBuilder::default().onchain_fallback(false).build_and_execute(|| {
roll_to(25);
roll_to_unsigned();
assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25)));

// Zilch solutions thus far.
assert!(MultiPhase::queued_solution().is_none());
assert_eq!(MultiPhase::elect().unwrap_err(), ElectionError::Fallback("NoFallback."));
// phase is now emergency.
assert_eq!(MultiPhase::current_phase(), Phase::Emergency);

assert_eq!(
multi_phase_events(),
vec![
Event::SignedPhaseStarted { round: 1 },
Event::UnsignedPhaseStarted { round: 1 },
Event::ElectionFailed
]
);
})
}

#[test]
fn governance_fallback_works() {
ExtBuilder::default().onchain_fallback(false).build_and_execute(|| {
roll_to(25);
roll_to_unsigned();
assert_eq!(MultiPhase::current_phase(), Phase::Unsigned((true, 25)));

// Zilch solutions thus far.
Expand Down Expand Up @@ -2243,9 +2339,16 @@ mod tests {
assert_eq!(MultiPhase::current_phase(), Phase::Off);

// On-chain backup works though.
roll_to(29);
let supports = MultiPhase::elect().unwrap();
assert!(supports.len() > 0);

assert_eq!(
multi_phase_events(),
vec![Event::ElectionFinalized {
compute: ElectionCompute::Fallback,
score: ElectionScore { minimal_stake: 0, sum_stake: 0, sum_stake_squared: 0 }
}]
);
});
}

Expand All @@ -2269,6 +2372,8 @@ mod tests {
let err = MultiPhase::elect().unwrap_err();
assert_eq!(err, ElectionError::Fallback("NoFallback."));
assert_eq!(MultiPhase::current_phase(), Phase::Emergency);

assert_eq!(multi_phase_events(), vec![Event::ElectionFailed]);
});
}

Expand All @@ -2282,7 +2387,7 @@ mod tests {
crate::mock::MaxElectingVoters::set(2);

// Signed phase opens just fine.
roll_to(15);
roll_to_signed();
assert_eq!(MultiPhase::current_phase(), Phase::Signed);

assert_eq!(
Expand All @@ -2295,7 +2400,7 @@ mod tests {
#[test]
fn untrusted_score_verification_is_respected() {
ExtBuilder::default().build_and_execute(|| {
roll_to(15);
roll_to_signed();
assert_eq!(MultiPhase::current_phase(), Phase::Signed);

// set the solution balancing to get the desired score.
Expand Down
11 changes: 11 additions & 0 deletions frame/election-provider-multi-phase/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ pub fn roll_to(n: BlockNumber) {
}
}

pub fn roll_to_unsigned() {
while !matches!(MultiPhase::current_phase(), Phase::Unsigned(_)) {
roll_to(System::block_number() + 1);
}
}
pub fn roll_to_signed() {
while !matches!(MultiPhase::current_phase(), Phase::Signed) {
roll_to(System::block_number() + 1);
}
}

pub fn roll_to_with_ocw(n: BlockNumber) {
let now = System::block_number();
for i in now + 1..=n {
Expand Down
Loading