Bounties e2e test#399
Conversation
|
Thanks for the early feedback. I will incorporate the suggested changes. |
|
I am aware this is a draft PR; whenever it's ready, ping me for a full review. |
…d acceptance test
|
Will take a look later in the week! |
There was a problem hiding this comment.
The ensureSpendPeriodAvailable function is incorrectly called with the client object instead of lastSpendPeriodBlock in two different tests. This will lead to test failures. Otherwise, the changes look good and provide comprehensive test coverage for the bounties pallet.
There was a problem hiding this comment.
The submitted code is well-structured and provides extensive test coverage for the bounties pallet. However, there are a few issues that need to be addressed, including an incorrect null check, a misleading comment, and several typos in comments and snapshot names. Addressing these points will improve the code's correctness and maintainability.
Suggestions that couldn't be attached to a specific line
packages/shared/src/bounties.ts:1568, 1726
There is a typo in the comments on these lines. 'amout' should be 'amount'.
| // verify the transfer event | ||
| await checkSystemEvents(client, { section: 'balances', method: 'Transfer' }) | ||
| .redact({ redactKeys: /from|to/ }) | ||
| .toMatchSnapshot('bounty value transfered to treasury') |
There was a problem hiding this comment.
There is a typo in the snapshot name: 'bounty value transfered to treasury'. 'transfered' should be 'transferred'.
| expect(bountyStatus.status.isActive).toBe(true) | ||
|
|
||
| // Charlie (public user) tries to unassign curator immediately (premature) | ||
| // Using scheduleInlineCallWithOrigin to simulate public call |
There was a problem hiding this comment.
The comment // Using scheduleInlineCallWithOrigin to simulate public call is misleading. The test actually uses sendTransaction on line 2165 to send a transaction from a regular account (charlie), not scheduleInlineCallWithOrigin. Please update the comment to accurately reflect that a public user is attempting the action, for example: // Charlie (a public user) tries to unassign the curator.
rockbmb
left a comment
There was a problem hiding this comment.
Great stuff!
- Every
bountiespallet's extrinsic is exercised at least oncepokeDepositstill not available
- Every event type appears at least once
DepositPokedstill not available, either
- Almost every variant of the
Error<T>enum is checked at least once- 100% coverage is a distraction, but exercising each at least once is desirable
- snapshots redact temporal data, as they should
- Broad selection of scenarios
The PR is already large enough, feel free to address my comments in a future PR.
| await client.dev.newBlock() | ||
|
|
||
| // claim the bounty | ||
| const claimBountyTx = client.api.tx.bounties.claimBounty(bountyIndex) |
There was a problem hiding this comment.
This is the successful case, which is good to cover.
However, the failure code path is not exercised, which would also be good:
https://github.com/paritytech/polkadot-sdk/blob/c3f62bf918ef6879390dc6a2cf9f91caac23f5b5/substrate/frame/bounties/src/lib.rs#L734-L736
if let BountyStatus::PendingPayout { curator, beneficiary, unlock_at } =
bounty.status
{
...
} else {
// This arm of the conditional
Err(Error::<T, I>::UnexpectedStatus.into())
}Can be left to a future PR.
There was a problem hiding this comment.
added the failure code in the latest commits
| * Test: Bounty closure in PendingPayout state (should fail) | ||
| * | ||
| * Verifies: | ||
| * - Bounty closure fails with PendingPayout error when in PendingPayout state (GeneralAdmin must unassign curator first) |
There was a problem hiding this comment.
| * - Bounty closure fails with PendingPayout error when in PendingPayout state (GeneralAdmin must unassign curator first) | |
| * - Bounty closure fails with `PendingPayout` error when in `PendingPayout` state (`GeneralAdmin` must unassign curator first) |
This is pedantry on my part, but backticks (``) when referring to code in comments are preferable, imo.
There was a problem hiding this comment.
You are right, i am updating it.
There was a problem hiding this comment.
added backticks for code in all the opening comments.
There was a problem hiding this comment.
Thanks. Like I said, it's not important, but it makes comments slightly easier to read.
| const updateDueBefore = bountyForExtending.status.asActive.updateDue.toNumber() | ||
|
|
||
| // extend the bounty expiry | ||
| const extendBountyTx = client.api.tx.bounties.extendBountyExpiry(bountyIndex, 'Testing the bounty extension') |
There was a problem hiding this comment.
This is the successful case;
A few others would be interesting to test: https://github.com/paritytech/polkadot-sdk/blob/c3f62bf918ef6879390dc6a2cf9f91caac23f5b5/substrate/frame/bounties/src/lib.rs#L855-L862
match bounty.status {
BountyStatus::Active { ref curator, ref mut update_due } => {
// A check to the caller. Really basic, but the compiler can't catch these things.
ensure!(*curator == signer, Error::<T, I>::RequireCurator);
*update_due = Self::treasury_block_number()
.saturating_add(T::BountyUpdatePeriod::get())
.max(*update_due);
},
// A check that exercises this code path
_ => return Err(Error::<T, I>::UnexpectedStatus.into()),As my other such comments, this can be left to a future PR.
There was a problem hiding this comment.
As my other such comments, this can be left to a future PR.
Sure. Will add in a future PR
| /// ------- | ||
|
|
||
| // initial funding balance for accounts | ||
| const TEST_ACCOUNT_BALANCE = 100000000000000n |
There was a problem hiding this comment.
This test is going to be run on post-AHM KAH (in fact, it already can; remind me to ping you this week to pair on this; will be useful later), and soon, post-migration PAH.
Could you make this a large multiple of const.balances.existentialDeposit?
pokeDeposit and DepositPoked is not available as I was using the bounties code from the branch in the ahm dryrun repo, should have taken the latest code from polkadot-sdk .
Sure, I will address these in a future PR |
|
Thank you very much for all the suggestions, I'm excited to put these best practices into action on my next PR. |
I know, and now I know that you know! I was just commenting for future reference i.e. that at the time this PR was written, these were not available, in other words, that coverage is complete (until those come online) |
Added end-to-end tests to cover the main flows of the pallet_bounties module.
Key scenarios covered:
Closes #292 .