-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 supply keeper tests to use simapp #4877
Conversation
Codecov Report
@@ 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 |
&abci.ConsensusParams{ | ||
Validator: &abci.ValidatorParams{ | ||
PubKeyTypes: []string{tmtypes.ABCIPubKeyTypeEd25519}, | ||
app := simapp.NewSimApp(log.NewNopLogger(), db, nil, true, 0) |
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.
👏 👏
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
@@ -1,25 +1,18 @@ | |||
package keeper | |||
package keeper_test |
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.
I think we should be more explicit in the sense that these are integration tests. Can we rename it to integration_test.go
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.
definitely! I like this idea a lot
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.
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.
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.
Sorry, I meant the file lol
@@ -0,0 +1 @@ | |||
#4875 refactored supply keeper integration tests to use SimApp |
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.
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?
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 about creating a multiple line pending entry? Then you add a new line on each PR. Check the supply module PR for reference
simapp/app.go
Outdated
accountKeeper auth.AccountKeeper | ||
bankKeeper bank.Keeper | ||
supplyKeeper supply.Keeper | ||
AccountKeeper auth.AccountKeeper |
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.
Is there a reason certain keepers are public and others are kept private?
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.
no reason, will update the rest
simapp/app.go
Outdated
type SimApp struct { | ||
*bam.BaseApp | ||
cdc *codec.Codec |
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.
We should rather keep this private and provide a Codec() *codec.Codec
method on SimApp
.
simapp/app.go
Outdated
|
||
invCheckPeriod uint | ||
|
||
// keys to access the substores | ||
keys map[string]*sdk.KVStoreKey | ||
Keys map[string]*sdk.KVStoreKey |
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.
ditto; We should provide a keys getter.
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
. "github.com/cosmos/cosmos-sdk/x/supply/internal/keeper" |
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.
afaiu, dot imports are stylistic choice and strictly unnecessary. Why do we do this?
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.
was mostly following what go standard lib did. I can update this
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.
it makes sense to use dot notation here as we're on the same folder. I'd keep 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.
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.
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.
updated to using a "keep" alias instead of dot import
"github.com/cosmos/cosmos-sdk/x/supply/internal/types" | ||
) | ||
|
||
// nolint: deadcode unused |
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.
Why is this dead code?
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.
it's not, will remove
"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" |
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.
ditto
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.
see comment above
Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
I modified return signature for |
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.
LGTM; Left some minor feedback 👍
CHANGELOG.md
Outdated
@@ -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 |
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.
- Modules updated: supply |
simapp/helpers.go
Outdated
) | ||
} | ||
|
||
ctx = app.BaseApp.NewContext(isCheckTx, abci.Header{}) |
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.
Since we have BaseApp
public on the app and we return it, why not just have the caller create the context as needed?
|
||
// nolint: deadcode unused | ||
func createTestApp(isCheckTx bool) (*simapp.SimApp, sdk.Context) { | ||
app, ctx := simapp.Setup(isCheckTx) |
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.
See above.
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.
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?
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 ⚡️
POC for #4875
Removes outside imports into
keeper
package.keeper_test
imports simapp and uses simapp to do integration testing. Had to updateSimApp
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
ormock
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 explorerFor Admin Use: