x/feegrant audit: clean up / add test coverage to types package #9193

technicallyty · 2021-04-23T22:25:58Z

Description

-cleans up the x/feegrant/types folder
-adds test cases to cli_test to cover filtered_fee functions
-adds line to msgs.go to cover case where NewMsgRevoke could have msg.granter == msg.grantee
-remove redundant constants from msgs.go

ref: #9115

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.

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.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

commit 58dc50051226a9eeb8d0ebea5bb0908fe5b9637f Author: technicallyty <[email protected]> Date: Fri Apr 23 15:09:27 2021 -0700 remove comments commit a84107e1b3eaa31324cb0f4f097b49f02af79c69 Author: technicallyty <[email protected]> Date: Fri Apr 23 15:02:41 2021 -0700 add tests for msgs.go commit 2ad16869237e9631b402c93cde650c3fc554daf2 Author: technicallyty <[email protected]> Date: Fri Apr 23 13:20:21 2021 -0700 specify test name commit b7121277c9be586a7c80d010ec401e50b510e02a Merge: c0c134d971 3c65c3d Author: technicallyty <[email protected]> Date: Fri Apr 23 12:54:55 2021 -0700 Merge branch 'master' into ty-9115-types_tests commit c0c134d97107194dc4f9d3c501a15d023ae083e5 Author: technicallyty <[email protected]> Date: Thu Apr 22 19:59:11 2021 -0700 -add test case to cli_test.go for filtered fee -clean up identifiers -remove redundant import alias from filtered_fee.go commit f7ab3699da39be3ab886f96962d28d23438d2e8e Author: technicallyty <[email protected]> Date: Thu Apr 22 09:57:31 2021 -0700 remove unecessary constant commit 9db59a82a7337cf5a7a3569c6900a44a6c81e8b4 Merge: a3e75ceb8a e28271b Author: technicallyty <[email protected]> Date: Thu Apr 22 09:19:20 2021 -0700 Merge branch 'master' into ty-9115-types_tests commit a3e75ceb8a510ad9db43dd96073c43b7a8b062b0 Merge: 4d3dafab85 bffcae5 Author: technicallyty <[email protected]> Date: Wed Apr 21 12:04:39 2021 -0700 Merge branch 'master' into ty-9115-types_tests commit 4d3dafab85d85526a7c94b045289605289ee6b0d Author: technicallyty <[email protected]> Date: Wed Apr 21 12:03:41 2021 -0700 cleanup id's / add test case commit e8f6924931ba95e592bfc3057ba167700458da41 Author: technicallyty <[email protected]> Date: Wed Apr 21 10:55:29 2021 -0700 add test for 0 allowance / remove unused field on exp test commit 516b6ef89a4582ad681cc6c5c101cf50a9a54fb5 Author: technicallyty <[email protected]> Date: Tue Apr 20 15:22:48 2021 -0700 make names more clear, remove unused field

codecov · 2021-04-23T22:34:13Z

Codecov Report

Merging #9193 (9ddb406) into master (15edf3c) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 9ddb406 differs from pull request most recent head a92d72a. Consider uploading reports for the commit a92d72a to get more accurate results

@@            Coverage Diff             @@
##           master    #9193      +/-   ##
==========================================
+ Coverage   59.86%   59.95%   +0.09%     
==========================================
  Files         595      595              
  Lines       37355    37358       +3     
==========================================
+ Hits        22361    22398      +37     
+ Misses      13011    12974      -37     
- Partials     1983     1986       +3

Impacted Files	Coverage Δ
x/feegrant/types/filtered_fee.go	`0.00% <ø> (ø)`
x/feegrant/client/testutil/suite.go	`99.66% <100.00%> (+0.50%)`	⬆️
x/feegrant/types/msgs.go	`72.09% <100.00%> (+72.09%)`	⬆️
x/feegrant/types/basic_fee.go	`75.00% <0.00%> (+10.00%)`	⬆️

blushi · 2021-04-26T06:55:19Z

x/feegrant/types/expiration_test.go

@@ -15,32 +15,27 @@ func TestExpiresAt(t *testing.T) {

 	cases := map[string]struct {
 		example types.ExpiresAt


maybe rename it to expiresAt?

ExpiresAt is a proto message, we usually have these exported

I meant rename the struct field name example into expiresAt

ok, changed it to expires to be consistent with the other tests in the file

blushi · 2021-04-26T07:10:24Z

x/feegrant/types/grant_test.go

@@ -49,6 +51,12 @@ func TestGrant(t *testing.T) {
 	})
 	require.NoError(t, err)

+	zeroAllowance, err := types.NewFeeAllowanceGrant(addr, addr2, &types.BasicFeeAllowance{


maybe all these NewFeeAllowanceGrants test could be turned into table tests (using the one below)?

yeah that will be cleaner, uses table tests now

blushi · 2021-04-26T07:15:52Z

x/feegrant/types/msgs_test.go

+		grant 	*types.BasicFeeAllowance
+		valid 	bool
+	}{
+		"valid":{


event though that's already tested elsewhere, could you add test cases for invalid granter and grantee for both messages?
Also I believe having two separate tests for both msgs types would be clearer.

Also I believe having two separate tests for both msgs types would be clearer.

moved out

event though that's already tested elsewhere, could you add test cases for invalid granter and grantee for both messages?

okay added some invalid granter/grantee cases

x/feegrant/types/msgs_test.go

blushi

LGTM

atheeshp

lgtm

fix imports

441a18c

technicallyty requested review from blushi and aleem1314 April 23, 2021 22:52

blushi mentioned this pull request Apr 26, 2021

x/feegrant Updates #9115

Closed

7 tasks

blushi reviewed Apr 26, 2021

View reviewed changes

technicallyty added 2 commits April 26, 2021 12:12

Merge branch 'master' into ty-9115-cleanup_types

434774c

fix tests

3e6b751

technicallyty marked this pull request as ready for review April 26, 2021 23:59

technicallyty requested review from aaronc and alexanderbez as code owners April 26, 2021 23:59

blushi requested review from atheeshp, amaury1093, anilcse, robert-zaremba and cyberbono3 April 27, 2021 07:46

amaury1093 assigned cyberbono3 and atheeshp Apr 27, 2021

technicallyty added 3 commits April 27, 2021 10:57

rename test field

6bde6b7

Merge branch 'master' into ty-9115-cleanup_types

ce77bbd

Merge branch 'master' into ty-9115-cleanup_types

9ddb406

technicallyty requested a review from blushi April 27, 2021 19:11

github-actions bot added C:CLI C:x/feegrant labels Apr 27, 2021

blushi approved these changes Apr 28, 2021

View reviewed changes

atheeshp approved these changes Apr 28, 2021

View reviewed changes

blushi added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 28, 2021

blushi and others added 2 commits April 28, 2021 13:33

Merge branch 'master' into ty-9115-cleanup_types

882c3a5

Merge branch 'master' into ty-9115-cleanup_types

a92d72a

mergify bot merged commit 0cbed20 into master Apr 28, 2021

mergify bot deleted the ty-9115-cleanup_types branch April 28, 2021 14:18

cyberbono3 removed their assignment Apr 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/feegrant audit: clean up / add test coverage to types package #9193

x/feegrant audit: clean up / add test coverage to types package #9193

technicallyty commented Apr 23, 2021 •

edited by blushi

Loading

codecov bot commented Apr 23, 2021 •

edited

Loading

blushi Apr 26, 2021

technicallyty Apr 26, 2021

blushi Apr 27, 2021

technicallyty Apr 27, 2021

blushi Apr 26, 2021

technicallyty Apr 26, 2021

blushi Apr 26, 2021

technicallyty Apr 26, 2021

blushi left a comment

atheeshp left a comment

		@@ -15,32 +15,27 @@ func TestExpiresAt(t *testing.T) {

		cases := map[string]struct {
		example types.ExpiresAt

x/feegrant audit: clean up / add test coverage to types package #9193

x/feegrant audit: clean up / add test coverage to types package #9193

Conversation

technicallyty commented Apr 23, 2021 • edited by blushi Loading

Description

codecov bot commented Apr 23, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

atheeshp left a comment

Choose a reason for hiding this comment

technicallyty commented Apr 23, 2021 •

edited by blushi

Loading

codecov bot commented Apr 23, 2021 •

edited

Loading