Improved election pallet testing by Szegoo · Pull Request #12327 · paritytech/substrate

Szegoo · 2022-09-22T11:19:43Z

Closes: #12184

Replaced some roll_to(25) with roll_to_unsigned
Added an extra check for events at the end of each test.
Replaced some roll_to(15) with roll_to_signed
Make sure the emitted events are correct.

NOTE: I did go through the event assertions to see if the emitted events are correct and it seems like they are to me.

Polkadot address: 126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA

Szegoo · 2022-09-23T13:29:43Z

@kianenigma PTAL.

frame/election-provider-multi-phase/src/mock.rs

frame/election-provider-multi-phase/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

kianenigma

Looks mostly good, some small changes needed.

kianenigma

Would like to see events in other tests as well, namely at least one that finished with compute: Signed (signed.rs)

Szegoo · 2022-10-03T10:58:38Z

Accidentally removed the review request for @ruseinov and @Ank4n :/

Ank4n · 2022-10-03T11:46:10Z

frame/election-provider-multi-phase/src/lib.rs

 			assert_eq!(MultiPhase::round(), 1);

-			roll_to(25);
+			roll_to_unsigned();


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

cc @kianenigma

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.

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.

frame/election-provider-multi-phase/src/lib.rs

frame/election-provider-multi-phase/src/signed.rs

kianenigma · 2022-10-04T10:58:20Z

/tip medium

substrate-tip-bot · 2022-10-04T10:58:23Z

@kianenigma A medium tip was successfully submitted for Szegoo (126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

kianenigma · 2022-10-04T10:58:37Z

PS: https://github.com/polkadot-fellows/manifesto

frame/election-provider-multi-phase/src/lib.rs

Ank4n

LGTM! Nice work. :)

Ank4n · 2022-10-04T18:34:02Z

bot rebase

paritytech-processbot · 2022-10-04T18:34:09Z

Rebased

Ank4n · 2022-10-04T22:15:59Z

bot merge

@koute

* master: (42 commits) Adapt `pallet-contracts` to WeightV2 (#12421) Improved election pallet testing (#12327) Bump prost to 0.11+ (#12419) Use saturating add for alliance::disband witness data (#12418) [Fix] Rename VoterBagsList -> VoterList to match pdot (#12416) client/beefy: small code improvements (#12414) BEEFY: Simplify hashing for pallet-beefy-mmr (#12393) Add @koute to `docs/CODEOWNERS` and update stale paths (#12408) docs/CODEOWNERS: add @acatangiu as MMR owner (#12406) Remove unnecessary Clone trait bounds on CountedStorageMap (#12402) Fix `Weight::is_zero` (#12396) Beefy on-demand justifications as a custom RequestResponse protocol (#12124) Remove contracts RPCs (#12358) pallet-mmr: generate historical proofs (#12324) unsafe_pruning flag removed (#12385) Carry over where clauses defined in Config to Call and Hook (#12388) Properly set the max proof size weight on defaults and tests (#12383) BEEFY: impl TypeInfo for SignedCommitment (#12382) bounding staking: `BoundedElectionProvider` trait (#12362) New Pallet: Root offences (#11943) ...

* Improved election pallet testing * fmt * remove comment * more checks * fixes in logic * roll_to_signed * switch to roll_to_signed * Update frame/election-provider-multi-phase/src/mock.rs Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> * remove useless checks * remove warning * add checks to signed.rs * add some checks to unsigned.rs * fmt * use roll_to_signed and roll_to_unsigned * remove nonsense * remove even more nonsense * fix * fix * remove useless checks Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Co-authored-by: parity-processbot <>

Szegoo and others added 8 commits September 22, 2022 13:15

Improved election pallet testing

f978433

Merge branch 'master' into election-testing

ef0c223

fmt

2f2c8b5

remove comment

0494a89

more checks

d64de36

fixes in logic

0d3478f

roll_to_signed

be05c37

switch to roll_to_signed

a0e24ca

kianenigma reviewed Sep 24, 2022

View reviewed changes

frame/election-provider-multi-phase/src/mock.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 24, 2022

View reviewed changes

frame/election-provider-multi-phase/src/lib.rs Outdated Show resolved Hide resolved

Update frame/election-provider-multi-phase/src/mock.rs

e861d57

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

kianenigma reviewed Sep 24, 2022

View reviewed changes

Szegoo added 2 commits September 24, 2022 10:37

remove useless checks

97b30c4

remove warning

b2d1859

Szegoo requested a review from kianenigma September 25, 2022 16:18

kianenigma requested review from Ank4n and ruseinov October 3, 2022 07:40

kianenigma suggested changes Oct 3, 2022

View reviewed changes

Szegoo and others added 5 commits October 3, 2022 12:28

add checks to signed.rs

818c404

add some checks to unsigned.rs

956de55

Merge branch 'master' into election-testing

c8a3b4e

fmt

b4c162c

use roll_to_signed and roll_to_unsigned

779e300

Szegoo removed request for Ank4n and ruseinov October 3, 2022 10:57

Szegoo requested a review from kianenigma October 3, 2022 10:57

Ank4n requested review from Ank4n and ruseinov October 3, 2022 11:18

Ank4n reviewed Oct 3, 2022

View reviewed changes

frame/election-provider-multi-phase/src/lib.rs Outdated Show resolved Hide resolved

Szegoo added 3 commits October 4, 2022 09:59

remove nonsense

b9a008d

remove even more nonsense

ac84324

fix

bd128f3

kianenigma reviewed Oct 4, 2022

View reviewed changes

frame/election-provider-multi-phase/src/signed.rs Show resolved Hide resolved

kianenigma approved these changes Oct 4, 2022

View reviewed changes

Ank4n reviewed Oct 4, 2022

View reviewed changes

frame/election-provider-multi-phase/src/lib.rs Show resolved Hide resolved

Szegoo added 2 commits October 4, 2022 19:48

fix

93cc43f

remove useless checks

a403504

Ank4n approved these changes Oct 4, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into election-testing

7787315

paritytech-processbot bot merged commit 241b0d0 into paritytech:master Oct 4, 2022

Szegoo mentioned this pull request Oct 7, 2022

Member Request polkadot-fellows/seeding#36

Merged

aurexav mentioned this pull request Jan 10, 2023

To polkadot-v0.9.33 darwinia-network/darwinia-2.0#194

Closed

Conversation

Szegoo commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Szegoo commented Sep 23, 2022

Uh oh!

Uh oh!

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

Szegoo commented Oct 3, 2022

Uh oh!

Ank4n Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ank4n Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

kianenigma Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

Ank4n Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kianenigma commented Oct 4, 2022

Uh oh!

substrate-tip-bot bot commented Oct 4, 2022

Uh oh!

kianenigma commented Oct 4, 2022

Uh oh!

Uh oh!

Ank4n left a comment

Choose a reason for hiding this comment

Uh oh!

Ank4n commented Oct 4, 2022

Uh oh!

paritytech-processbot bot commented Oct 4, 2022

Uh oh!

Ank4n commented Oct 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Szegoo commented Sep 22, 2022 •

edited

Loading

Ank4n Oct 3, 2022 •

edited

Loading

Ank4n Oct 4, 2022 •

edited

Loading