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.

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

ACP-77: Update P-chain state staker tests #3494

Merged
merged 5 commits into from
Oct 24, 2024

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

Factored out of #3487.

  1. Rewrites TestPersistStakers as TestState_writeStakers which uses test tables rather than extensive callbacks.
  2. Fixes TestState_writeStakers to not break the invariants around subnet validators being a sub-set of the primary network validator set.
  3. Expands TestStateAddRemoveValidator as TestState_ApplyValidatorDiffs which includes additional cases, such as adding an removing validators in the same diff.

How this works

Generally cleans up existing tests and improves coverage.

How this was tested

Only tests are modified.

@StephenButtolph StephenButtolph added testing This primarily focuses on testing cleanup Code quality improvement acp77 labels Oct 24, 2024
@StephenButtolph StephenButtolph added this to the v1.11.13 milestone Oct 24, 2024
@StephenButtolph StephenButtolph self-assigned this Oct 24, 2024
valOut := valsMap[staker.NodeID]
r.Equal(valOut.NodeID, staker.NodeID)
r.Equal(valOut.Weight, val.Weight+staker.Weight)
expectedPublicKeyDiff: maybe.Some[*bls.PublicKey](nil),
Copy link
Contributor

Choose a reason for hiding this comment

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

why would adding a validator not update the publicKeyDiff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is specifying a key diff (providing a maybe.Some means there is a key diff present). Adding a validator results in a key diff where the prior value was nil.

vms/platformvm/state/state_test.go Outdated Show resolved Hide resolved
},
expectedWeightDiff: &ValidatorWeightDiff{
Decrease: false,
Amount: primaryNetworkCurrentDelegatorStaker.Weight,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

for these tests why not also sanity check the expectedPublicKeyDiff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are verifying that there is no public key diff.

Weight: uint64(i + 1),
StartTime: primaryStaker.StartTime,
EndTime: primaryStaker.EndTime,
PotentialReward: uint64(i + 1),
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't seem like we use PotentialReward anyway, but if start time and end time are the same, then shouldn't the potential reward be 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.

if start time and end time are the same

endTime is 24h after startTime throughout this test, but you're right these values don't really matter for this test.

@StephenButtolph StephenButtolph added this pull request to the merge queue Oct 24, 2024
Merged via the queue into master with commit 9a0a079 Oct 24, 2024
23 checks passed
@StephenButtolph StephenButtolph deleted the update-state-staker-tests branch October 24, 2024 18:41
yacovm pushed a commit to yacovm/avalanchego that referenced this pull request Oct 25, 2024
yacovm pushed a commit to yacovm/avalanchego that referenced this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acp77 cleanup Code quality improvement testing This primarily focuses on testing
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants