Unit tests for pallet EPM-MB #6332

Zebedeusz · 2024-11-01T13:35:37Z

No description provided.

…ssion

gpestana

A few comments and nits, especially around merging test cases that are similar and simplifying the test code.

Other than that, this is great, thanks for the help!

substrate/frame/election-provider-multi-block/src/signed/tests.rs

gpestana · 2024-11-06T06:36:36Z

substrate/frame/election-provider-multi-block/src/signed/tests.rs

+			// let some_bn = 0;
+			// let some_page_index = 0;
+
+			let account_id = 99;


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.

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.

substrate/frame/election-provider-multi-block/src/signed/tests.rs

gpestana · 2024-11-06T06:56:24Z

substrate/frame/election-provider-multi-block/src/signed/tests.rs

+	}
+
+	#[test]
+	fn force_clear_submission_fails_if_called_by_account_none() {


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)

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.

Quick 2-cents from me: in this case, testing different origins is a simple procedure, so I get @Zebedeusz's point of having different #[test]s

If testing different origins required a (more or less) similar setup everytime, doing it all in the same test would make more sense as @gpestana suggests.

Example: a verbose integration test for Westend's people parachain checking for different wrong origins.

substrate/frame/election-provider-multi-block/src/signed/tests.rs

gpestana · 2024-11-06T06:58:40Z

substrate/frame/election-provider-multi-block/src/signed/tests.rs

+	}
+
+	#[test]
+	fn force_clear_submission_fails_if_submitter_done_no_submissions_at_all() {


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.

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.

The reasons why I'd merge these small tests cases are:

Simplify - we can re-use setup code across tests cases;

Context - the tests are basically testing the same unit (the force_clear) behaviour

Follow-along/readability - it's easier to undestand all the invariants of force_clear etc if they are all in the same test, we can use comments to explain what is being tested and why errors happen, etc

Low verbosity - especially due to 1.

See, for example, this test case in staking: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/staking/src/tests.rs#L1283

We test within a test all the flow of Call::unbound. We could split it in different tests but the setup/boilerplate code would be repeated all over the individual tests. Also through that test you can read and understand the whole unbound flow, requirements, invariants, etc

https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/staking/src/tests.rs#L1495 being another good example, where assert_noop! is used too to ensure that errors do not change the storage root (and state).

substrate/frame/election-provider-multi-block/src/signed/tests.rs

gpestana · 2024-11-06T07:01:49Z

substrate/frame/election-provider-multi-block/src/signed/tests.rs

+		})
+	}
+
+	#[test]


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 !

As above. I'm not in favour for merged scenarios so let's discuss that bigger topic first

…e nits

gpestana · 2024-11-18T12:57:59Z

substrate/frame/election-provider-multi-block/src/mock/mod.rs

@@ -66,6 +67,14 @@ pub type T = Runtime;
 pub type Block = frame_system::mocking::MockBlock<Runtime>;
 pub(crate) type Solver = SequentialPhragmen<AccountId, sp_runtime::PerU16, ()>;

+pub struct Weighter;
+
+impl Get<RuntimeDbWeight> for Weighter {


Do we really need this? The type DbWeight can be set to unit in the configs in test mocks, ie.

type DbWeight = ();

gpestana · 2024-11-18T12:58:21Z

substrate/frame/election-provider-multi-block/src/mock/mod.rs

@@ -80,6 +89,7 @@ frame_election_provider_support::generate_solution_type!(
 impl frame_system::Config for Runtime {
 	type Block = Block;
 	type AccountData = pallet_balances::AccountData<Balance>;
+	type DbWeight = Weighter;


Suggested change

type DbWeight = Weighter;

type DbWeight = ();

gpestana · 2024-11-18T12:59:26Z

substrate/frame/election-provider-multi-block/src/mock/mod.rs

+		self
+	}
+
+	pub(crate) fn no_desired_targets(self) -> Self {


why do we need this? I think we should remove this, also setting desired targets to Err("none") doesn't seem correct to me.

gpestana · 2024-11-18T13:06:31Z

substrate/frame/election-provider-multi-block/src/signed/tests.rs

+		})
+	}
+
+	#[test]


so: given there are existing submissions, if someone bails from a submission that they didn't do, then the balances of all the participants, the one that had a submission and especially the one that bailed but didn't have one will remain unchanged.

This can be tested using the assert_noop! macro. Basically, when assert_noop!(call(), Error), the macro checks a couple things:

The call() will return the expected `Error

The Ext storage was not changed after calling call(). this means that the balances, and other states remain the same.

I'd just remove the whole set up of the registration etc and call an assert_noop!(bail_for(1), Err). This tests that the bailing does not work in this case and the storage root remained the same, so no state has changed (+ simplify the tests considerably).

gpestana · 2024-11-18T13:07:05Z

substrate/frame/election-provider-multi-block/src/signed/tests.rs

+	}
+
+	#[test]
+	fn bail_while_having_no_submissions_does_not_modify_balances() {


Same as above, assert_noop!() will simplify the test case, make it more comprehensive and simpler.

gpestana · 2024-11-18T13:12:25Z

substrate/frame/election-provider-multi-block/src/signed/tests.rs

+	}
+
+	#[test]
+	fn force_clear_submission_fails_if_submitter_done_no_submissions_at_all() {


The reasons why I'd merge these small tests cases are:

Simplify - we can re-use setup code across tests cases;

Context - the tests are basically testing the same unit (the force_clear) behaviour

Follow-along/readability - it's easier to undestand all the invariants of force_clear etc if they are all in the same test, we can use comments to explain what is being tested and why errors happen, etc

Low verbosity - especially due to 1.

See, for example, this test case in staking: https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/staking/src/tests.rs#L1283

We test within a test all the flow of Call::unbound. We could split it in different tests but the setup/boilerplate code would be repeated all over the individual tests. Also through that test you can read and understand the whole unbound flow, requirements, invariants, etc

gpestana · 2024-11-18T13:13:38Z

substrate/frame/election-provider-multi-block/src/signed/tests.rs

+	}
+
+	#[test]
+	fn force_clear_submission_fails_if_submitter_done_no_submissions_at_all() {


https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/staking/src/tests.rs#L1495 being another good example, where assert_noop! is used too to ensure that errors do not change the storage root (and state).

Zebedeusz added 3 commits November 1, 2024 14:35

some nits and unit tests for signed part - bail and force_clear_submi…

9b41a95

…ssion

Bail tests restructured

1010c1a

SolutionDataProvider tests

3c02986

gpestana reviewed Nov 6, 2024

View reviewed changes

Zebedeusz requested a review from gpestana November 6, 2024 08:19

Zebedeusz and others added 9 commits November 6, 2024 10:29

some verifier tests

72f4f92

signed pallet - tests rearangements, phase changed done with set, som…

28607d4

…e nits

verify_synchronous tests

ff72aad

verifier tests

4552f6b

unsigned tests

70a5e94

Test improvement ideas (#6487)

d0fccad

commented test removed, some nits

ec12271

test fix

09884c2

unsigned on_initialize hook tests

11ccb86

gpestana reviewed Nov 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests for pallet EPM-MB #6332

Unit tests for pallet EPM-MB #6332

Zebedeusz commented Nov 1, 2024

gpestana left a comment

gpestana Nov 6, 2024

Zebedeusz Nov 6, 2024

gpestana Nov 6, 2024

Zebedeusz Nov 6, 2024

rockbmb Nov 15, 2024

gpestana Nov 6, 2024

Zebedeusz Nov 6, 2024

gpestana Nov 18, 2024

gpestana Nov 18, 2024

gpestana Nov 6, 2024

Zebedeusz Nov 6, 2024

gpestana Nov 18, 2024

gpestana Nov 18, 2024

gpestana Nov 18, 2024

gpestana Nov 18, 2024

gpestana Nov 18, 2024

gpestana Nov 18, 2024

gpestana Nov 18, 2024

+              		})
+              	}
+              	#[test]

Unit tests for pallet EPM-MB #6332

Are you sure you want to change the base?

Unit tests for pallet EPM-MB #6332

Conversation

Zebedeusz commented Nov 1, 2024

gpestana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment