Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes all tests and removes a few nits #6595

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Fixes all tests and removes a few nits #6595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 0 additions & 4 deletions substrate/frame/election-provider-multi-block/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,6 @@ pub use types::*;

pub use crate::{unsigned::miner, verifier::Verifier, weights::WeightInfo};

/// Internal crate re-exports to use across benchmarking and tests.
#[cfg(any(test, feature = "runtime-benchmarks"))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really needed anymore.

use crate::verifier::Pallet as PalletVerifier;

/// Log target for this the core EPM-MB pallet.
const LOG_TARGET: &'static str = "runtime::multiblock-election";

Expand Down
3 changes: 2 additions & 1 deletion substrate/frame/election-provider-multi-block/src/mock/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ pub struct ExtBuilder {

impl Default for ExtBuilder {
fn default() -> Self {
Self { core_try_state: true, signed_try_state: true, minimum_score: Some(Default::default()) }
Self { core_try_state: true, signed_try_state: true, minimum_score: None }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that the default Election score passes, simplifies tests

}
}

Expand Down Expand Up @@ -438,6 +438,7 @@ impl ExtBuilder {

pub(crate) fn build_and_execute(self, test: impl FnOnce() -> ()) {
let mut ext = self.build();
ext.execute_with(|| roll_one());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

starts by rolling one block so that the System events are initialized. Otherwise they won't be catched and don't show up in the tests.

ext.execute_with(test);

ext.execute_with(|| {
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/election-provider-multi-block/src/signed/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ type BalanceOf<T> = <<T as Config>::Currency as FnInspect<AccountIdOf<T>>>::Bala
pub type CreditOf<T> = Credit<AccountIdOf<T>, <T as Config>::Currency>;

/// Release strategy for currency held by this pallet.
#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, PartialEq)]
#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, PartialEq, Clone)]
pub(crate) enum ReleaseStrategy {
/// Releases all held deposit.
All,
Expand All @@ -165,7 +165,7 @@ impl Default for ReleaseStrategy {
}

/// Metadata of a registered submission.
#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Default, RuntimeDebugNoBound)]
#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Default, RuntimeDebugNoBound, Clone)]
#[cfg_attr(test, derive(frame_support::PartialEqNoBound, frame_support::EqNoBound))]
#[codec(mel_bound(T: Config))]
#[scale_info(skip_type_params(T))]
Expand Down
103 changes: 18 additions & 85 deletions substrate/frame/election-provider-multi-block/src/signed/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,7 @@
// limitations under the License.

use super::*;
use crate::{
mock::*,
signed::Error::{CannotClear, NotAcceptingSubmissions, SubmissionNotRegistered},
verifier::SolutionDataProvider,
CurrentPhase, Phase, Verifier,
};
use crate::{mock::*, verifier::SolutionDataProvider, Phase, Verifier};
use frame_support::{assert_noop, assert_ok, testing_prelude::*};
use sp_npos_elections::ElectionScore;
use sp_runtime::traits::Convert;
Expand All @@ -34,7 +29,6 @@ fn clear_submission_of_works() {
mod calls {
use super::*;
use sp_core::bounded_vec;
use sp_runtime::traits::BadOrigin;

#[test]
fn register_works() {
Expand Down Expand Up @@ -263,11 +257,12 @@ mod calls {
}

#[test]
fn register_and_submit_page_and_bail_prohibitted_in_phase_other_than_signed() {
fn calls_in_phase_other_than_signed() {
ExtBuilder::default().build_and_execute(|| {
let account_id = 99;

let phases = vec![
Phase::Halted,
Phase::Off,
Phase::SignedValidation(1),
Phase::Unsigned(1),
Expand All @@ -281,7 +276,7 @@ mod calls {

assert_err!(
SignedPallet::register(RuntimeOrigin::signed(account_id), Default::default()),
NotAcceptingSubmissions::<Runtime>,
Error::<T>::NotAcceptingSubmissions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

easier to read

);

assert_err!(
Expand All @@ -290,12 +285,12 @@ mod calls {
0,
Some(Default::default())
),
NotAcceptingSubmissions::<Runtime>,
Error::<T>::NotAcceptingSubmissions
);

assert_err!(
SignedPallet::bail(RuntimeOrigin::signed(account_id)),
NotAcceptingSubmissions::<Runtime>,
Error::<T>::CannotBail
);
}
})
Expand Down Expand Up @@ -345,33 +340,20 @@ mod calls {
// account 1 submitted nothing, so bail should have no effect and return error
assert_noop!(
SignedPallet::bail(RuntimeOrigin::signed(bailing_account_id)),
SubmissionNotRegistered::<Runtime>
);
})
}

#[test]
fn force_clear_submission_fails_if_called_by_account_none() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really relevant.

ExtBuilder::default().build_and_execute(|| {
assert_err!(
SignedPallet::force_clear_submission(RuntimeOrigin::none(), 0, 99),
BadOrigin
Error::<T>::SubmissionNotRegistered
);
})
}

#[test]
fn force_clear_submission_fails_if_called_in_phase_other_than_off() {
ExtBuilder::default().build_and_execute(|| {
let some_bn = 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.

doesn't need to be a separate variable, otherwise the developer will think this is important value to the tests.

let some_page_index = 0;

let phases = vec![
Phase::Signed,
Phase::Snapshot(some_page_index),
Phase::SignedValidation(some_bn),
Phase::Unsigned(some_bn),
Phase::Export(some_bn),
Phase::Snapshot(0),
Phase::SignedValidation(0),
Phase::Unsigned(0),
Phase::Export(0),
Phase::Emergency,
];

Expand All @@ -381,7 +363,7 @@ mod calls {

assert_err!(
SignedPallet::force_clear_submission(RuntimeOrigin::root(), 0, account_id),
CannotClear::<Runtime>,
Error::<T>::CannotClear,
);
}
})
Expand All @@ -395,14 +377,13 @@ mod calls {

assert_err!(
SignedPallet::force_clear_submission(RuntimeOrigin::root(), 0, account_id),
CannotClear::<Runtime>
Error::<T>::NoSubmission,
);
})
}

#[test]
fn force_clear_submission_fails_if_submitter_done_submissions_for_another_round_than_requested()
{
fn force_clear_submission_fails_if_submitter_if_different_round() {
ExtBuilder::default().build_and_execute(|| {
let account_id = 99;
let current_round = MultiPhase::current_round();
Expand All @@ -415,42 +396,7 @@ mod calls {
current_round + 1,
account_id
),
CannotClear::<Runtime>
);
})
}

#[test]
fn force_clear_submission_removes_both_metadata_and_submission_pages() {
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 added a test that covers all these cases, so we can drop this one.

ExtBuilder::default().build_and_execute(|| {
let account_id = 99;
let current_round = MultiPhase::current_round();

// do_register and try_mutate_page used directly so as not to switch phases in the test
assert_ok!(Pallet::<Runtime>::do_register(
&account_id,
Default::default(),
current_round
));

assert_ok!(Submissions::<Runtime>::try_mutate_page(
&account_id,
current_round,
0,
Some(Default::default())
));

roll_to_phase(Phase::Off);

assert_ok!(SignedPallet::force_clear_submission(
RuntimeOrigin::root(),
current_round,
account_id
));

assert!(Submissions::<Runtime>::metadata_for(current_round, &account_id).is_none());
assert!(
Submissions::<Runtime>::page_submission_for(current_round, account_id, 0).is_none()
Error::<T>::NoSubmission,
);
})
}
Expand Down Expand Up @@ -513,7 +459,7 @@ mod calls {
);

// force phase Off.
CurrentPhase::<T>::set(Phase::Off);
set_phase_to(Phase::Off);
assert_ok!(SignedPallet::force_clear_submission(RuntimeOrigin::signed(99), round, 99));

// 3 pages submitted have been cleared from storage.
Expand Down Expand Up @@ -543,14 +489,14 @@ mod calls {
Phase::Export(0),
Phase::Emergency,
] {
CurrentPhase::<T>::set(disabled_phase);
set_phase_to(disabled_phase);
assert_noop!(
SignedPallet::force_clear_submission(RuntimeOrigin::signed(99), round, 99),
Error::<T>::CannotClear
);
}

CurrentPhase::<T>::set(Phase::Off);
set_phase_to(Phase::Off);

// request force clear of a non existing submission.
assert_noop!(
Expand Down Expand Up @@ -704,19 +650,6 @@ mod solution_data_provider {
})
}

#[test]
fn returns_none_given_invalid_page_index() {
ExtBuilder::default().build_and_execute(|| {
let origin = RuntimeOrigin::signed(99);
roll_to_phase(Phase::Signed);

assert_ok!(SignedPallet::register(origin.clone(), Default::default()));
assert_ok!(SignedPallet::submit_page(origin, 0, Some(Default::default())));

assert_eq!(<SignedPallet as SolutionDataProvider>::get_paged_solution(12345), None)
})
}

#[test]
fn returns_none_if_there_are_no_submissions() {
ExtBuilder::default().build_and_execute(|| {
Expand Down
8 changes: 4 additions & 4 deletions substrate/frame/election-provider-multi-block/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,10 +466,10 @@ mod election_provider {
.unwrap();

let supports_page_zero =
PalletVerifier::<T>::feasibility_check(results.solution_pages[0].clone(), 0)
VerifierPallet::feasibility_check(results.solution_pages[0].clone(), 0)
.unwrap();
let supports_page_one =
PalletVerifier::<T>::feasibility_check(results.solution_pages[1].clone(), 1)
VerifierPallet::feasibility_check(results.solution_pages[1].clone(), 1)
.unwrap();

let s0: Supports<AccountId> = vec![
Expand Down Expand Up @@ -520,10 +520,10 @@ mod election_provider {
.unwrap();

let supports_page_zero =
PalletVerifier::<T>::feasibility_check(results.solution_pages[0].clone(), 0)
VerifierPallet::feasibility_check(results.solution_pages[0].clone(), 0)
.unwrap();
let supports_page_one =
PalletVerifier::<T>::feasibility_check(results.solution_pages[1].clone(), 1)
VerifierPallet::feasibility_check(results.solution_pages[1].clone(), 1)
.unwrap();

// consume supports and checks they fit within the max backers per winner bounds.
Expand Down
Loading
Loading