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

replace TestHandleSlashPacketDistribution w/ TestSlashUndelegation #633

Merged
merged 4 commits into from
Jan 6, 2023

Conversation

mpoke
Copy link
Contributor

@mpoke mpoke commented Jan 3, 2023

Description

TestHandleSlashPacketDistribution is replaced with TestSlashUndelegation, which tests

the slashing of an undelegation balance in various scenarios

Linked issues

Related to #629

Type of change

  • Testing: Adds testing

@mpoke mpoke requested a review from MSalopek January 3, 2023 17:26
}{
// infraction - delegate - undelegate - slash - mature consumer - mature provider
// TODO: this behavior is unexpected
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 behavior is unexpected according to my understanding of slashing in Cosmos. Need to discuss with TM team.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this already tracked as an issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sainoe
Copy link
Contributor

sainoe commented Jan 4, 2023

Great work @mpoke. Just a few suggestions to shorten the test length.

@sainoe sainoe self-requested a review January 4, 2023 10:37
err := so.fn(suite)
suite.Require().NoError(err)
for i, tc := range testCases {
providerKeeper := suite.providerApp.GetProviderKeeper()
Copy link
Contributor

Choose a reason for hiding this comment

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

To fully reset the state, we need to call SetupTest on e2e tests that have multiple cases. Otherwise I believe we're using the same store between cases

Suggested change
providerKeeper := suite.providerApp.GetProviderKeeper()
suite.SetupTest()
providerKeeper := suite.providerApp.GetProviderKeeper()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already doing it at the end of the for loop.

}{
// infraction - delegate - undelegate - slash - mature consumer - mature provider
// TODO: this behavior is unexpected
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this already tracked as an issue?

@shaspitz shaspitz mentioned this pull request Jan 5, 2023
1 task
Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

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

LGTM once the comments are addressed. Great stuff, this test is definitely more intentioned than the previous one

* checkDelegatorBalance refactored out

* fix comments
@mpoke
Copy link
Contributor Author

mpoke commented Jan 6, 2023

make test passes locally

@mpoke mpoke merged commit fe44b35 into main Jan 6, 2023
@mpoke mpoke deleted the marius/slash_undeleg_test branch January 6, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants