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

Jump to bottom

test: add tests for gov proposal #216

Merged
merged 12 commits into from
Nov 10, 2021
Merged

test: add tests for gov proposal #216

merged 12 commits into from
Nov 10, 2021

Conversation

hallazzang
Copy link
Contributor

@hallazzang hallazzang commented Nov 8, 2021 •
edited
Loading

Description

Add tests for PublicPlanProposal.

closes: #174

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Appropriate labels applied
  • Targeted PR against correct branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@hallazzang
test: add govHandler in KeeperTestSuite
f638554
@hallazzang
chore: Merge branch 'main' of github.com:tendermint/farming into 174-…
28e8d6c 
…proposal-tests
@hallazzang
test: add tests for gov proposal
fc8a199 
also make `KeeperTestSuite.Create*Plan` to actually use
`Keeper.Create*Plan`, not `Keeper.SetPlan`.
also fix some typos.
@hallazzang hallazzang self-assigned this Nov 8, 2021
@dongsam
Copy link
Contributor

dongsam commented Nov 8, 2021

It might be a little annoying, but it would be better if the test logics were added.

  1. Verification that existing rewards are withdrawn without problems and no additional rewards are accumulated even after delete proposal passed or termination due to endtime modify proposal passed.
  2. When the ratio and amount are changed in the existing staking object, whether the reward corresponding to the changed value is accumulated well.

@hallazzang
Copy link
Contributor Author

Alright. I'll add more tests to cover that cases.

jaybxyz
jaybxyz suggested changes Nov 9, 2021
View reviewed changes
x/farming/keeper/proposal_handler_test.go Outdated Show resolved Hide resolved
x/farming/keeper/keeper_test.go Show resolved Hide resolved
dongsam
dongsam approved these changes Nov 10, 2021
View reviewed changes
Copy link
Contributor

@dongsam dongsam left a comment

Choose a reason for hiding this comment

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

LGTM

@dongsam dongsam requested a review from jaybxyz November 10, 2021 10:35
@dongsam
Copy link
Contributor

dongsam commented Nov 10, 2021

requested changes of @kogisin applied well, and he is on retreat, let me merge with skip approval

@dongsam dongsam merged commit e50fbab into tendermint:main Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dongsam dongsam dongsam approved these changes

@jaybxyz jaybxyz Awaiting requested review from jaybxyz

Assignees

@hallazzang hallazzang

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test: Add testcode the plan works as expected after modified
3 participants
@hallazzang @dongsam @jaybxyz