This repository was archived by the owner on Nov 15, 2023. It is now read-only.
add negative tests#11995
Merged
muharem merged 4 commits intopallet-alliance-retirement-noticefrom Aug 26, 2022
Merged
Conversation
muharem
reviewed
Aug 11, 2022
frame/alliance/src/tests.rs
Outdated
Comment on lines
+474
to
+478
| assert_noop!(Alliance::add_unscrupulous_items(user.clone(), vec![]), BadOrigin); | ||
|
|
||
| assert_noop!(Alliance::remove_unscrupulous_items(user.clone(), vec![]), BadOrigin); | ||
|
|
||
| assert_noop!(Alliance::announce(user, cid.clone()), Error::<Test, ()>::NotAlly); |
Contributor
There was a problem hiding this comment.
there three checked by AnnouncementOrigin, which is configurable by user, and for these tests its mocked to return true for user 3
muharem
reviewed
Aug 11, 2022
| }); | ||
| } | ||
|
|
||
| fn assert_powerless(user: Origin) { |
Contributor
There was a problem hiding this comment.
Nice!
I think right not it make sense to check if method supposed to be executed by a member.
For add_unscrupulous_items for example, its not really a case, because its checked by AnnouncementOrigin provided by client of pallet.
If you think it should be different (should check if the caller is a member for example), I think we need a different issue for it.
Contributor
There was a problem hiding this comment.
Otherwise in this context we are not really checking than the retirement made the use powerless, but more that announcement functionality is allowed only for AnnouncementOrigin
muharem
reviewed
Aug 11, 2022
| // Move time on: | ||
| System::set_block_number(System::block_number() + RetirementPeriod::get()); | ||
|
|
||
| assert_powerless(Origin::signed(3)); |
bkontur
reviewed
Aug 26, 2022
frame/alliance/src/tests.rs
Outdated
|
|
||
| assert_noop!(Alliance::nominate_ally(user.clone(), 4), Error::<Test, ()>::NoVotingRights); | ||
|
|
||
| assert_noop!(Alliance::propose(user.clone(), 5, Box::new(proposal), 1000), Error::<Test, ()>::NoVotingRights); |
Contributor
There was a problem hiding this comment.
@gilescope
the last one user.clone() could be just user
muharem
approved these changes
Aug 26, 2022
paritytech-processbot bot
pushed a commit
that referenced
this pull request
Aug 29, 2022
* Alliance pallet: retirement notice * add alliance pallet to benchmark list for dev chain * fix type * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * link weight generated by bench for retirement_notice method * migration to clear UpForKicking storage prefix * rename migration from v1 to v0_to_v1 * Apply suggestions from code review Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * rename `retirement-notice to give-retirement-notice * Apply suggestions from code review Co-authored-by: Squirrel <gilescope@gmail.com> * review fixes: update doc, saturating add, BlockNumber type alias * add suffix to duratin consts *_IN_BLOCKS * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * add negative tests (#11995) * add negative tests * remove tests powerless asserts checking against announcment origin * assert bad origin from announcement origin checks Co-authored-by: muharem <ismailov.m.h@gmail.com> Co-authored-by: command-bot <> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: Squirrel <gilescope@gmail.com>
ark0f
pushed a commit
to gear-tech/substrate
that referenced
this pull request
Feb 27, 2023
* Alliance pallet: retirement notice * add alliance pallet to benchmark list for dev chain * fix type * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * link weight generated by bench for retirement_notice method * migration to clear UpForKicking storage prefix * rename migration from v1 to v0_to_v1 * Apply suggestions from code review Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * rename `retirement-notice to give-retirement-notice * Apply suggestions from code review Co-authored-by: Squirrel <gilescope@gmail.com> * review fixes: update doc, saturating add, BlockNumber type alias * add suffix to duratin consts *_IN_BLOCKS * ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance * add negative tests (paritytech#11995) * add negative tests * remove tests powerless asserts checking against announcment origin * assert bad origin from announcement origin checks Co-authored-by: muharem <ismailov.m.h@gmail.com> Co-authored-by: command-bot <> Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: Squirrel <gilescope@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added some more negative tests to make sure a retired person has no special privileges.
The last 3 asserts currently return Ok(()) and thus fail the test.
We're using 4 and the announcement origin is defined as the following in the mock:
Maybe I have got the test wrong in some way?