Unit tests for pallet EPM-MB #6332
Conversation
| for phase in phases { | ||
| roll_to_phase(phase); | ||
|
|
||
| assert_err!( |
There was a problem hiding this comment.
Try to register and submit a solution as well.
| ExtBuilder::default().build_and_execute(|| { | ||
| // TODO | ||
| // let some_bn = 0; |
There was a problem hiding this comment.
No need to define the BN, using fn roll_to_phase is enough I think.
| // let some_bn = 0; | ||
| // let some_page_index = 0; | ||
|
|
||
| let account_id = 99; |
There was a problem hiding this comment.
nit but since the account_id is not so important for the tests, no need to define it as a separate var. Just use 99 directly in the calls.
There was a problem hiding this comment.
I prefer to use a variable than a number that looks a little magical. This way wherever this variable is used, one can see what it actually represents.
| }) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
To simplify these tests, I now think it would be better to not use the roll to phase helper but rather enforce the phase by changing directly the current phase storage state, e.g.
#[test]
fn register_and_submit_page_and_bail_prohibitted_in_phase_signed_validation() {
ExtBuilder::default().build_and_execute(|| {
set_phase_to(Phase::SignedValidation(100));
assert_err!(
SignedPallet::register(RuntimeOrigin::signed(99), Default::default()),
NotAcceptingSubmissions::<Runtime>,
);
assert_err!(
SignedPallet::submit_page(
RuntimeOrigin::signed(99),
0,
Some(Default::default())
),
NotAcceptingSubmissions::<Runtime>,
);
assert_err!(
SignedPallet::bail(RuntimeOrigin::signed(99)),
NotAcceptingSubmissions::<Runtime>,
);
})
}where fn set_phase_to is defined in the mock.rs and should be something along the lines of
pub fn set_phase_to(phase: Phase<BlockNumber>) {
CurrentPhase::<Runtime>::set(phase);
}This way, we don't need to complicate too much the test cases with the roll to and ensuring all the invariants are present at every phase (snapshot is in place, a solution is in place, etce etc), which are not part of what we want to test here.
There was a problem hiding this comment.
Basically with this setup, you can test all the phase cases for this failure much more easily.
| } | ||
|
|
||
| #[test] | ||
| fn register_and_submit_page_and_bail_prohibitted_in_phase_export() { |
There was a problem hiding this comment.
same as my comment above, and for all test cases.
| } | ||
|
|
||
| #[test] | ||
| fn force_clear_submission_fails_if_called_by_account_none() { |
There was a problem hiding this comment.
I would re-factor this test to check that all origins are correct when calling bail. I.e.
- Origin none fails
- Origin signed fails if there's no registration
- Origin signed is OK if there's a registration
- etc
This would cover all the cases and no need for the test above (bail_while_having_no_submissions_does_not_modify_balances)
There was a problem hiding this comment.
This is a test for force_clear but I guess you mean the one for bail.
Anyway, if I understand correctly you would put all the test cases for "bail" function in one test function.
If yes, then I would be against it. It would surely provide the coverage we need and verify all we need but it would result in overloaded tests, checking lots of things, instead of many tests, each checking very specific outcome.
So let me understand your reasoning here.
| } | ||
|
|
||
| #[test] | ||
| fn force_clear_submission_fails_if_called_in_phase_other_than_off() { |
There was a problem hiding this comment.
As above, I'd probably force set the phase storage instead of using roll_to which is simpler. In any case, testing all the cases using a vec is a good approach but instead of roll_to_phase you can set_phase_to.
| } | ||
|
|
||
| #[test] | ||
| fn force_clear_submission_fails_if_submitter_done_no_submissions_at_all() { |
There was a problem hiding this comment.
This test is useful but I'd try to integrate more test cases to cover all the possible edge cases in one test alone. This will help to keep the number of tests low and cover everything.
There was a problem hiding this comment.
Is there a specific reason why you're suggesting the approach of having few tests with lots of checks inside each one?
My preferred approach is exactly the opposite one, so I'd like to understand your point of view.
| } | ||
|
|
||
| #[test] | ||
| fn get_score_returns_entry_with_lowest_sum_stake_squared_given_entries_with_equal_minimal_stakes_and_sum_stakes( |
There was a problem hiding this comment.
I would probably simplify the test names :) this is too long and too descriptive. Again I think this test could be merged in the above test, where all the permutations of scores and ordering are tested.
There was a problem hiding this comment.
Yeah, I got carried away with these names a little :)
I am very much for desciptive but true that this is too much.
I'll work on it.
| }) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
This test could be merged with the above test. Calling it get_wrong_solution_page_works and then testing all the cases where the None is returned due to bad page index is a good call !
There was a problem hiding this comment.
As above. I'm not in favour for merged scenarios so let's discuss that bigger topic first
No description provided.