refactor(ecocredit): `GetParamSet` -> `Get` #1064

technicallyty · 2022-04-27T19:37:42Z

Description

Closes: #996

refactors GetParamSet calls to use the single Get

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

codecov · 2022-04-28T22:17:01Z

Codecov Report

Merging #1064 (cc99100) into master (f653b63) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head cc99100 differs from pull request most recent head 7527dda. Consider uploading reports for the commit 7527dda to get more accurate results

@@            Coverage Diff             @@
##           master    #1064      +/-   ##
==========================================
- Coverage   68.10%   68.00%   -0.10%     
==========================================
  Files         210      207       -3     
  Lines       20968    20855     -113     
==========================================
- Hits        14280    14183      -97     
+ Misses       5375     5367       -8     
+ Partials     1313     1305       -8

Flag	Coverage Δ
experimental-codecov	`67.93% <100.00%> (-0.03%)`	⬇️
stable-codecov	`61.72% <100.00%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

ryanchristo

A couple concerns, one with using panic and another with abstracting the expect calls.

x/ecocredit/expected_keepers.go

ryanchristo · 2022-04-29T16:05:13Z

x/ecocredit/server/basket/create_test.go

-	minFee := sdk.NewCoins(sdk.NewCoin("regen", sdk.NewInt(100)))
-
-	// no fee specified should fail
-	gmAny := gomock.Any()
-	s.paramsKeeper.EXPECT().GetParamSet(gmAny, gmAny).Do(func(any interface{}, p *core.Params) {
-		p.BasketFee = minFee
-	}).Times(2)


I'm not sure we should abstract the expect call. If anything, we should be more specific with the values passed and avoid using gomock.Any() whenever possible. This looks like a nice improvement but I think we lose some of the value of using mocks. I realize the abstracted method currently provides the same assurance as what was before but if we want to be more specific, we should use the expect call directly.

ryanchristo · 2022-04-29T16:14:58Z

x/ecocredit/server/utils/test_utils.go

+// ExpectParamGet is a helper function that sets up an expected mock call for the provided type.
+// Once we switch to Go 1.18+ we can switch this impl to be generic:
+// func ExpectParamGet[T any](obj *T, paramKeeper *mocks.MockParamKeeper, times int) {
+//	gmAny := gomock.Any()
+//	paramKeeper.EXPECT().Get(gmAny, gmAny, gmAny).Do(func(_, _ any, param *T) {
+//		*param = *obj
+//	}).Times(times)
+// }
+func ExpectParamGet(obj interface{}, paramKeeper *mocks.MockParamKeeper, times int) {
+	gmAny := gomock.Any()
+	switch obj.(type) {
+	case *[]string:
+		s := obj.(*[]string)
+		paramKeeper.EXPECT().Get(gmAny, gmAny, gmAny).Do(func(_, _ interface{}, param *[]string) {
+			*param = *s
+		}).Times(times)
+	case *sdk.Coins:
+		coins := obj.(*sdk.Coins)
+		paramKeeper.EXPECT().Get(gmAny, gmAny, gmAny).Do(func(_, _ interface{}, param *sdk.Coins) {
+			*param = *coins
+		}).Times(times)
+	case *bool:
+		b := obj.(*bool)
+		paramKeeper.EXPECT().Get(gmAny, gmAny, gmAny).Do(func(_, _ interface{}, param *bool) {
+			*param = *b
+		}).Times(times)
+	case *[]*core.AskDenom:
+		askDenoms := obj.(*[]*core.AskDenom)
+		paramKeeper.EXPECT().Get(gmAny, gmAny, gmAny).Do(func(_, _ interface{}, param *[]*core.AskDenom) {
+			*param = *askDenoms
+		}).Times(times)
+	}
+}


I don't think we should abstract the expect call. We should try to avoid using gomock.Any() and be more specific with the values passed whenever possible. This may be convenient but gomock.Any() should be used sparingly and we should be as specific as we can when we can when writing tests.

For example, I started using specific values with expect calls in the acceptance tests I'm currently drafting:

regen-ledger/x/ecocredit/server/basket/msg_put_test.go

Lines 335 to 341 in bf21ecc

s.bankKeeper.EXPECT().

MintCoins(s.sdkCtx, basket.BasketSubModuleName, coins).

Return(nil).AnyTimes()

s.bankKeeper.EXPECT().

SendCoinsFromModuleToAccount(s.sdkCtx, basket.BasketSubModuleName, s.addr, coins).

Return(nil).AnyTimes()

this method is specific to params (ExpectParamGet). the only more specific i can really get is the param key ( and maybe the context, but incorrect contexts would be caught by compiler anyway). i can go ahead and add key []byte as an argument

@ryanchristo i think you may have missed this reply 👋🏻

the only more specific i can really get is the param key

Nope. I saw it. We don't need to use gomock.Any(). See #1064 (comment).

I don't think we should be adding more parameters. Might as well use the expect call directly if we add any more.

Might as well use the expect call directly if we add any more.

it's only 4 params, i'd much prefer that over the 4+ lines from direct expect call. the only thing thats different at this point is the expected context, but i dont really see that as necessary or valuable, as its not used, and the app wont even compile if we pass anything other than sdk.Context to the acutal params.Get call in the keeper methods.

technicallyty · 2022-05-04T16:58:20Z

@aleem1314 could i get a review here 🙏🏻

ryanchristo

I'm still not sure about abstracting the expect call and think we should be as specific as possible when writing tests. I included a working example without using gomock.Any().

x/ecocredit/server/core/create_class.go

x/ecocredit/server/utils/test_utils.go

ryanchristo · 2022-05-04T18:58:25Z

x/ecocredit/server/basket/create_test.go

-	s.paramsKeeper.EXPECT().GetParamSet(gmAny, gmAny).Do(func(any interface{}, p *core.Params) {
-		p.BasketFee = fee
-	}).Times(1)
+	utils.ExpectParamGet(&basketFees, s.paramsKeeper, core.KeyBasketCreationFee, 1)


Here's a working example without using gomock.Any() and using a specific time:

Suggested change

utils.ExpectParamGet(&basketFees, s.paramsKeeper, core.KeyBasketCreationFee, 1)

var coins sdk.Coins

s.paramsKeeper.EXPECT().Get(s.sdkCtx, core.KeyBasketCreationFee, &coins).

Do(func(ctx sdk.Context, key []byte, ptr *sdk.Coins) {

ptr = &basketFees

}).

Times(1)

Not sure if setting the ptr is necessary because it works without it and regardless of what it gets set too, in which case it could be:

Suggested change

utils.ExpectParamGet(&basketFees, s.paramsKeeper, core.KeyBasketCreationFee, 1)

var coins sdk.Coins

s.paramsKeeper.EXPECT().Get(s.sdkCtx, core.KeyBasketCreationFee, &coins).

Do(func(sdk.Context, []byte, *sdk.Coins) {}).

Times(1)

aleem1314

lgtm

technicallyty added 3 commits April 27, 2022 08:28

feat: add get to createclass

502fe65

refactor: GetParamSet -> Get

5fbe259

chore: cleanup documentation

1c61092

technicallyty closed this Apr 27, 2022

technicallyty reopened this Apr 27, 2022

This was referenced Apr 27, 2022

perf(ecocredit/core+marketplace): GetParamSet -> Get #997

Closed

perf(ecocredit/basket): GetParamSet -> Get #999

Closed

technicallyty added 2 commits April 27, 2022 17:35

chore: cleanup func

aee2ac0

chore: 1.18 -> 1.17

4ff86a6

technicallyty marked this pull request as ready for review April 28, 2022 20:50

chore: any -> interface{}

de3855d

ryanchristo reviewed Apr 29, 2022

View reviewed changes

chore: add param key to util

b8cf4b9

technicallyty requested a review from aleem1314 May 3, 2022 19:33

technicallyty added 2 commits May 3, 2022 13:10

Merge branch 'master' into ty/996-param_get

f74fade

Merge branch 'master' into ty/996-param_get

b34b471

ryanchristo reviewed May 4, 2022

View reviewed changes

technicallyty added 4 commits May 4, 2022 12:20

fix: revert times removal

867198f

chore: add context to Do function

a6fe23c

Merge branch 'master' into ty/996-param_get

44ff537

refactor: use generics in ExpectParamGet

cc99100

ryanchristo approved these changes May 5, 2022

View reviewed changes

aleem1314 approved these changes May 10, 2022

View reviewed changes

Merge branch 'master' into ty/996-param_get

7527dda

technicallyty enabled auto-merge (squash) May 11, 2022 16:22

technicallyty merged commit 3f66709 into master May 11, 2022

technicallyty deleted the ty/996-param_get branch May 11, 2022 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ecocredit): `GetParamSet` -> `Get` #1064

refactor(ecocredit): `GetParamSet` -> `Get` #1064

technicallyty commented Apr 27, 2022 •

edited by zube bot

Loading

codecov bot commented Apr 28, 2022 •

edited

Loading

ryanchristo left a comment

ryanchristo Apr 29, 2022

ryanchristo Apr 29, 2022

technicallyty Apr 29, 2022

technicallyty May 4, 2022

ryanchristo May 4, 2022

ryanchristo May 4, 2022

technicallyty May 4, 2022 •

edited

Loading

technicallyty commented May 4, 2022

ryanchristo left a comment

ryanchristo May 4, 2022 •

edited

Loading

ryanchristo May 4, 2022 •

edited

Loading

aleem1314 left a comment

	s.bankKeeper.EXPECT().
	MintCoins(s.sdkCtx, basket.BasketSubModuleName, coins).
	Return(nil).AnyTimes()

	s.bankKeeper.EXPECT().
	SendCoinsFromModuleToAccount(s.sdkCtx, basket.BasketSubModuleName, s.addr, coins).
	Return(nil).AnyTimes()

-	utils.ExpectParamGet(&basketFees, s.paramsKeeper, core.KeyBasketCreationFee, 1)
+	var coins sdk.Coins
+	s.paramsKeeper.EXPECT().Get(s.sdkCtx, core.KeyBasketCreationFee, &coins).
+		Do(func(ctx sdk.Context, key []byte, ptr *sdk.Coins) {
+			ptr = &basketFees
+		}).
+		Times(1)

refactor(ecocredit): GetParamSet -> Get #1064

refactor(ecocredit): GetParamSet -> Get #1064

Conversation

technicallyty commented Apr 27, 2022 • edited by zube bot Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Apr 28, 2022 • edited Loading

Codecov Report

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo Apr 29, 2022

Choose a reason for hiding this comment

ryanchristo Apr 29, 2022

Choose a reason for hiding this comment

technicallyty Apr 29, 2022

Choose a reason for hiding this comment

technicallyty May 4, 2022

Choose a reason for hiding this comment

ryanchristo May 4, 2022

Choose a reason for hiding this comment

ryanchristo May 4, 2022

Choose a reason for hiding this comment

technicallyty May 4, 2022 • edited Loading

Choose a reason for hiding this comment

technicallyty commented May 4, 2022

ryanchristo left a comment

Choose a reason for hiding this comment

ryanchristo May 4, 2022 • edited Loading

Choose a reason for hiding this comment

ryanchristo May 4, 2022 • edited Loading

Choose a reason for hiding this comment

aleem1314 left a comment

Choose a reason for hiding this comment

refactor(ecocredit): `GetParamSet` -> `Get` #1064

refactor(ecocredit): `GetParamSet` -> `Get` #1064

technicallyty commented Apr 27, 2022 •

edited by zube bot

Loading

codecov bot commented Apr 28, 2022 •

edited

Loading

technicallyty May 4, 2022 •

edited

Loading

ryanchristo May 4, 2022 •

edited

Loading

ryanchristo May 4, 2022 •

edited

Loading