-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
refactor crisis tests #4918
refactor crisis tests #4918
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4918 +/- ##
==========================================
+ Coverage 53.75% 53.81% +0.06%
==========================================
Files 272 272
Lines 17103 17117 +14
==========================================
+ Hits 9194 9212 +18
+ Misses 7222 7218 -4
Partials 687 687 |
return app | ||
} | ||
|
||
func testPassingInvariant(_ sdk.Context) (string, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are these for? they don't seem to provide any value other than its name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are used in keeper_test.go
. I believe it is just there for readability of a passing/failing invariant function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we delete them then? 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor nit, otherwise LGTM 👍
simapp/test_helpers.go
Outdated
|
||
initCoins := sdk.NewCoins(sdk.NewCoin(app.StakingKeeper.BondDenom(ctx), accAmt)) | ||
totalSupply := sdk.NewCoins(sdk.NewCoin(app.StakingKeeper.BondDenom(ctx), accAmt.MulRaw(int64(len(testAddrs))))) | ||
app.SupplyKeeper.SetSupply(ctx, supply.NewSupply(totalSupply)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think think this should be done outside of the function call? Or maybe just added to the total supply instead of simply reseting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will add it to the total supply. It can be easy to forget to update supply when writing tests so I think keeping this functionality encapsulated in this helper function will prevent useless debugging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK pending my prev comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 🎉
#4875 has merge conflict with #4909
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added a relevant changelog entry to the
Unreleased
section inCHANGELOG.md
Re-reviewed
Files changed
in the github PR explorerFor Admin Use: