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

Diff denom keeper tests #96

Merged
merged 4 commits into from
Jul 12, 2021
Merged

Diff denom keeper tests #96

merged 4 commits into from
Jul 12, 2021

Conversation

Josumner
Copy link
Contributor

@Josumner Josumner commented Jul 9, 2021

Added test fund with afet
Added test drip with stake and afet

  • Just wondering what variable naming etiquette you use normally? I know the snake case + camel case combo doesn't really look very nice

@Josumner Josumner requested review from ejfitzgerald and daeMOn63 July 9, 2021 09:12
@@ -164,6 +166,84 @@ func (s *KeeperTestSuite) TestFeeDrip() {
s.Require().Equal(feeCollectorBalance, sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(4040)))
}

func (s *KeeperTestSuite) TestAddNewFundWithDiffDenom() {
amount := sdk.NewInt64Coin("afet", 4000)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for using another denom that afet here. This is test and can be anything really, no need to create any confusion with prod tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good call - let me know what you think of "denom" as a placeholder

}

func (s *KeeperTestSuite) TestMultiDenomFeeDrip() {
amount_stake := sdk.NewInt64Coin(sdk.DefaultBondDenom, 4000)
Copy link
Contributor

Choose a reason for hiding this comment

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

on variable naming, go uses camelCase only.

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2021

Codecov Report

Merging #96 (1c84c0b) into master (a59a3d4) will increase coverage by 1.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   61.99%   63.01%   +1.01%     
==========================================
  Files         622      624       +2     
  Lines       35762    41382    +5620     
==========================================
+ Hits        22170    26076    +3906     
- Misses      11239    12910    +1671     
- Partials     2353     2396      +43     
Impacted Files Coverage Δ
x/ibc/core/03-connection/types/connection.go 72.88% <0.00%> (-8.94%) ⬇️
types/handler.go 77.77% <0.00%> (-7.94%) ⬇️
x/staking/types/delegation.go 50.00% <0.00%> (-7.15%) ⬇️
x/staking/legacy/v040/migrate.go 55.31% <0.00%> (-6.22%) ⬇️
version/version.go 65.00% <0.00%> (-5.84%) ⬇️
x/ibc/core/04-channel/types/msgs.go 65.18% <0.00%> (-5.76%) ⬇️
x/ibc/core/03-connection/types/msgs.go 76.13% <0.00%> (-5.57%) ⬇️
x/ibc/core/04-channel/types/keys.go 83.33% <0.00%> (-5.56%) ⬇️
x/ibc/applications/transfer/types/packet.go 87.50% <0.00%> (-5.36%) ⬇️
x/gov/client/utils/query.go 19.73% <0.00%> (-5.27%) ⬇️
... and 610 more

daeMOn63
daeMOn63 previously approved these changes Jul 9, 2021

fund2 := types.Fund{
Amount: &amountStake,
DripAmount: sdk.NewInt(4000), // will only last one block
Copy link
Member

Choose a reason for hiding this comment

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

It would be a slightly better test if we didn't use the same 4000 40 combination but mixed it up a little

ejfitzgerald
ejfitzgerald previously approved these changes Jul 9, 2021
@Josumner
Copy link
Contributor Author

Sorry guys - my mistake! Accidentally pushed to this before merging and pushing to a new branch

@Josumner Josumner force-pushed the diff_denom_keeper_tests branch from 1c84c0b to 9e1c039 Compare July 12, 2021 09:37
@Josumner Josumner requested a review from daeMOn63 July 12, 2021 09:38
@Josumner Josumner closed this Jul 12, 2021
@Josumner Josumner reopened this Jul 12, 2021
@Josumner Josumner merged commit 73de69d into master Jul 12, 2021
@Josumner Josumner deleted the diff_denom_keeper_tests branch July 12, 2021 09:47
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.

4 participants