refactor supply keeper tests to use simapp #4877

colin-axner · 2019-08-08T23:47:11Z

POC for #4875

Removes outside imports into keeper package. keeper_test imports simapp and uses simapp to do integration testing. Had to update SimApp to make some fields exportable, which I think should be fine since it is just for testing.

If merged, I will move forward with refactoring all modules to use simapp instead of their current test_common or mock

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: clog add [section] [-t <tag>] [-m <msg>]
Re-reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

codecov · 2019-08-09T00:11:41Z

Codecov Report

Merging #4877 into master will increase coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #4877      +/-   ##
==========================================
+ Coverage   53.63%   53.68%   +0.04%     
==========================================
  Files         270      271       +1     
  Lines       16982    16997      +15     
==========================================
+ Hits         9109     9125      +16     
+ Misses       7184     7183       -1     
  Partials      689      689

fedekunze · 2019-08-09T11:14:24Z

x/supply/internal/keeper/common_test.go

-		&abci.ConsensusParams{
-			Validator: &abci.ValidatorParams{
-				PubKeyTypes: []string{tmtypes.ABCIPubKeyTypeEd25519},
+	app := simapp.NewSimApp(log.NewNopLogger(), db, nil, true, 0)


fedekunze

ACK

fedekunze · 2019-08-09T19:38:40Z

x/supply/internal/keeper/common_test.go

@@ -1,25 +1,18 @@
-package keeper
+package keeper_test


I think we should be more explicit in the sense that these are integration tests. Can we rename it to integration_test.go

definitely! I like this idea a lot

~~seems like golang doesn't allow this:~~

can't load package: package github.com/cosmos/cosmos-sdk/x/supply/internal/keeper: found packages keeper (account.go) and integration (bank_test.go) in github.com/cosmos/cosmos-sdk/x/supply/internal/keeper

~~from stackoverflow~~

The 2 packages must have the following names: - NameOfDirectory. - NameOfDirectory + _test. The names of the files in the _test package must end with _test.go

~~So I think we should probably leave it as keeper_test. I would suggest adding a folder integration but these tests really are just testing the keeper's integration into the rest of the modules.~~

Sorry, I meant the file lol

colin-axner · 2019-08-09T20:34:34Z

.pending/improvements/modules/_4875-refactored-sup

@@ -0,0 +1 @@
+#4875 refactored supply keeper integration tests to use SimApp


when I continue to refactor tests for different modules, should I link to same issue with different pending logs, link to the prs or update this pending log with bullets for each module refactored?

What about creating a multiple line pending entry? Then you add a new line on each PR. Check the supply module PR for reference

.pending/improvements/modules/_4875-refactor-integ

alexanderbez · 2019-08-12T16:39:20Z

simapp/app.go

-	accountKeeper  auth.AccountKeeper
-	bankKeeper     bank.Keeper
-	supplyKeeper   supply.Keeper
+	AccountKeeper  auth.AccountKeeper


Is there a reason certain keepers are public and others are kept private?

no reason, will update the rest

alexanderbez · 2019-08-12T16:39:45Z

simapp/app.go

 type SimApp struct {
 	*bam.BaseApp
-	cdc *codec.Codec


We should rather keep this private and provide a Codec() *codec.Codec method on SimApp.

alexanderbez · 2019-08-12T16:40:44Z

simapp/app.go


 	invCheckPeriod uint

 	// keys to access the substores
-	keys  map[string]*sdk.KVStoreKey
+	Keys  map[string]*sdk.KVStoreKey


ditto; We should provide a keys getter.

alexanderbez · 2019-08-12T16:43:29Z

x/supply/internal/keeper/bank_test.go


 import (
 	"testing"

 	"github.com/stretchr/testify/require"

 	sdk "github.com/cosmos/cosmos-sdk/types"
+	. "github.com/cosmos/cosmos-sdk/x/supply/internal/keeper"


afaiu, dot imports are stylistic choice and strictly unnecessary. Why do we do this?

was mostly following what go standard lib did. I can update this

it makes sense to use dot notation here as we're on the same folder. I'd keep it

I don't understand. This isn't a requirement and imho is a bad practice; i.e. it's not recommended (by the Go team). In the context of DSLs sure, but we're not using a DSL here.

updated to using a "keep" alias instead of dot import

alexanderbez · 2019-08-12T16:44:36Z

x/supply/internal/keeper/integration_test.go

+	"github.com/cosmos/cosmos-sdk/x/supply/internal/types"
+)
+
+// nolint: deadcode unused


Why is this dead code?

it's not, will remove

alexanderbez · 2019-08-12T16:44:40Z

x/supply/internal/keeper/integration_test.go

+	"github.com/cosmos/cosmos-sdk/codec"
+	"github.com/cosmos/cosmos-sdk/simapp"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	. "github.com/cosmos/cosmos-sdk/x/supply/internal/keeper"


see comment above

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

colin-axner · 2019-08-12T20:03:48Z

I modified return signature for createTestApp to be app, ctx and added a helper function in simapp that creates an application and context. If isCheckTx is set to false, it also runs initChain to prevent a panic since deliverState would be nil

alexanderbez

LGTM; Left some minor feedback 👍

alexanderbez · 2019-08-14T15:09:36Z

CHANGELOG.md

@@ -53,6 +53,8 @@ longer panics if the store to load contains substores that we didn't explicitly
 * (modules) [\#4762](https://github.com/cosmos/cosmos-sdk/issues/4762) Deprecate remove and add permissions in ModuleAccount.
 * (modules) [\#4760](https://github.com/cosmos/cosmos-sdk/issues/4760) update `x/auth` to match module spec.
 * (modules) [\#4814](https://github.com/cosmos/cosmos-sdk/issues/4814) Add security contact to Validator description.
+* (modules) [\#4875](https://github.com/cosmos/cosmos-sdk/issues/4875) refactor integration tests to use SimApp and separate test package
+  - Modules updated: supply


Suggested change

- Modules updated: supply

alexanderbez · 2019-08-14T15:17:27Z

simapp/helpers.go

+		)
+	}
+
+	ctx = app.BaseApp.NewContext(isCheckTx, abci.Header{})


Since we have BaseApp public on the app and we return it, why not just have the caller create the context as needed?

alexanderbez · 2019-08-14T15:18:05Z

x/supply/internal/keeper/integration_test.go

+
+// nolint: deadcode unused
+func createTestApp(isCheckTx bool) (*simapp.SimApp, sdk.Context) {
+	app, ctx := simapp.Setup(isCheckTx)


this function uses the ctx returned, so I don't see a reason not to return the ctx as well. maybe I should update the function signature to setupTestApp or something?

alexanderbez

ACK ⚡️

refactor supply keeper tests to use simapp

0da3fcf

colin-axner added the WIP label Aug 8, 2019

fedekunze reviewed Aug 9, 2019

View reviewed changes

add pending log

41192b0

colin-axner marked this pull request as ready for review August 9, 2019 18:59

colin-axner requested review from alessio, alexanderbez, jackzampolin and rigelrozanski as code owners August 9, 2019 18:59

colin-axner requested a review from fedekunze August 9, 2019 19:00

fix merge conflict

323e821

colin-axner added R4R T: Tests and removed WIP labels Aug 9, 2019

fedekunze approved these changes Aug 9, 2019

View reviewed changes

colin-axner commented Aug 9, 2019

View reviewed changes

colin-axner added 2 commits August 9, 2019 14:04

rename common_test to integration_test

214ea45

update pending log to multi line

2dc2de5

colin-axner mentioned this pull request Aug 9, 2019

refactor mint tests #4882

Merged

5 tasks

alexanderbez suggested changes Aug 12, 2019

View reviewed changes

colin-axner and others added 3 commits August 12, 2019 10:33

Update .pending/improvements/modules/_4875-refactor-integ

9646c04

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

add simapp helper function

1ea5fe0

remove testing from integration helper func

15f85f0

colin-axner added the S:blocked Status: Blocked label Aug 12, 2019

colin-axner added 3 commits August 13, 2019 15:28

fix merge conflict

5e2a30b

update to latest version of simapp

b133a0f

remove pending log; add to changelog

36b0b2c

remove usage of dot import

54b1d0a

colin-axner removed the S:blocked Status: Blocked label Aug 13, 2019

update querier test cdc usage

73a6a86

alexanderbez suggested changes Aug 14, 2019

View reviewed changes

colin-axner added 2 commits August 14, 2019 11:01

remove ctx creation from simapp

3bda62f

update CHANGELOG

f367145

alexanderbez approved these changes Aug 14, 2019

View reviewed changes

alexanderbez merged commit 1cf016c into master Aug 14, 2019

alexanderbez deleted the colin/4875-refactor-supply-testing branch August 14, 2019 21:10

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

Merge PR cosmos#4877: refactor supply keeper tests to use simapp

6400364

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor supply keeper tests to use simapp #4877

refactor supply keeper tests to use simapp #4877

colin-axner commented Aug 8, 2019 •

edited

Loading

codecov bot commented Aug 9, 2019 •

edited

Loading

fedekunze Aug 9, 2019

fedekunze left a comment

fedekunze Aug 9, 2019

colin-axner Aug 9, 2019

colin-axner Aug 9, 2019 •

edited

Loading

fedekunze Aug 9, 2019

colin-axner Aug 9, 2019

fedekunze Aug 9, 2019

alexanderbez Aug 12, 2019

colin-axner Aug 12, 2019

alexanderbez Aug 12, 2019

alexanderbez Aug 12, 2019

alexanderbez Aug 12, 2019

colin-axner Aug 12, 2019

fedekunze Aug 12, 2019

alexanderbez Aug 13, 2019

colin-axner Aug 13, 2019 •

edited

Loading

alexanderbez Aug 12, 2019

colin-axner Aug 12, 2019

alexanderbez Aug 12, 2019

fedekunze Aug 12, 2019

colin-axner commented Aug 12, 2019

alexanderbez left a comment

alexanderbez Aug 14, 2019

alexanderbez Aug 14, 2019

alexanderbez Aug 14, 2019

colin-axner Aug 14, 2019

alexanderbez left a comment

		@@ -0,0 +1 @@
		#4875 refactored supply keeper integration tests to use SimApp

refactor supply keeper tests to use simapp #4877

refactor supply keeper tests to use simapp #4877

Conversation

colin-axner commented Aug 8, 2019 • edited Loading

codecov bot commented Aug 9, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner Aug 9, 2019 • edited Loading

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

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

colin-axner Aug 13, 2019 • edited Loading

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

colin-axner commented Aug 12, 2019

alexanderbez left a 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

alexanderbez left a comment

Choose a reason for hiding this comment

colin-axner commented Aug 8, 2019 •

edited

Loading

codecov bot commented Aug 9, 2019 •

edited

Loading

colin-axner Aug 9, 2019 •

edited

Loading

colin-axner Aug 13, 2019 •

edited

Loading