test(x/staking): write integration tests #15890
Conversation
…ita/staking-integration-tests
…ita/staking-integration-tests
…ita/staking-integration-tests
…ita/staking-integration-tests
…ita/staking-integration-tests
…ita/staking-integration-tests
…ita/staking-integration-tests
…ita/staking-integration-tests
…ita/staking-integration-tests
…ita/staking-integration-tests
There was a problem hiding this comment.
lgtm! one nit. This does seem more like a mechanical rewrite, but given how big staking is, I'd say it is fine for this PR. Eventually, we do need to think about what should stay an integration test and what can be simplified to a unit test, and re-thinking about our test cases. Let's do that immediately for smaller modules.
There was a problem hiding this comment.
Why can this not be a unit test?
…ita/staking-integration-tests
| func initFixture(t testing.TB) *fixture { | ||
| keys := storetypes.NewKVStoreKeys( | ||
| authtypes.StoreKey, banktypes.StoreKey, types.StoreKey, | ||
| ) | ||
| cdc := moduletestutil.MakeTestEncodingConfig(auth.AppModuleBasic{}, distribution.AppModuleBasic{}).Codec | ||
|
|
||
| logger := log.NewTestLogger(t) | ||
| cms := integration.CreateMultiStore(keys, logger) | ||
|
|
||
| newCtx := sdk.NewContext(cms, cmtprototypes.Header{}, true, logger) | ||
|
|
||
| authority := authtypes.NewModuleAddress("gov") | ||
|
|
||
| maccPerms := map[string][]string{ | ||
| minttypes.ModuleName: {authtypes.Minter}, | ||
| types.ModuleName: {authtypes.Minter}, | ||
| types.BondedPoolName: {authtypes.Burner, authtypes.Staking}, | ||
| types.NotBondedPoolName: {authtypes.Burner, authtypes.Staking}, | ||
| } | ||
|
|
||
| accountKeeper := authkeeper.NewAccountKeeper( | ||
| cdc, | ||
| runtime.NewKVStoreService(keys[authtypes.StoreKey]), | ||
| authtypes.ProtoBaseAccount, | ||
| maccPerms, | ||
| sdk.Bech32MainPrefix, | ||
| authority.String(), | ||
| ) | ||
|
|
||
| blockedAddresses := map[string]bool{ | ||
| accountKeeper.GetAuthority(): false, | ||
| } | ||
| bankKeeper := bankkeeper.NewBaseKeeper( | ||
| cdc, | ||
| runtime.NewKVStoreService(keys[banktypes.StoreKey]), | ||
| accountKeeper, | ||
| blockedAddresses, | ||
| authority.String(), | ||
| log.NewNopLogger(), | ||
| ) | ||
|
|
||
| stakingKeeper := stakingkeeper.NewKeeper(cdc, keys[types.StoreKey], accountKeeper, bankKeeper, authority.String()) | ||
|
|
||
| authModule := auth.NewAppModule(cdc, accountKeeper, authsims.RandomGenesisAccounts, nil) | ||
| bankModule := bank.NewAppModule(cdc, bankKeeper, accountKeeper, nil) | ||
| stakingModule := staking.NewAppModule(cdc, stakingKeeper, accountKeeper, bankKeeper, nil) | ||
|
|
||
| integrationApp := integration.NewIntegrationApp(newCtx, logger, keys, cdc, authModule, bankModule, stakingModule) | ||
|
|
||
| sdkCtx := sdk.UnwrapSDKContext(integrationApp.Context()) | ||
|
|
||
| // Register MsgServer and QueryServer | ||
| types.RegisterMsgServer(integrationApp.MsgServiceRouter(), stakingkeeper.NewMsgServerImpl(stakingKeeper)) | ||
| types.RegisterQueryServer(integrationApp.QueryHelper(), stakingkeeper.NewQuerier(stakingKeeper)) | ||
|
|
||
| // set default staking params | ||
| stakingKeeper.SetParams(sdkCtx, types.DefaultParams()) | ||
|
|
||
| f := fixture{ | ||
| app: integrationApp, | ||
| sdkCtx: sdkCtx, | ||
| cdc: cdc, | ||
| keys: keys, | ||
| accountKeeper: accountKeeper, | ||
| bankKeeper: bankKeeper, | ||
| stakingKeeper: stakingKeeper, | ||
| } | ||
|
|
||
| return &f | ||
| } |
There was a problem hiding this comment.
nit: A lot of initFixture code is looking redundant in other modules' tests too, can this code pulled out to a helper method?
cc: @julienrbrt
There was a problem hiding this comment.
Its true that initFixture code seems redundant wherever used in integration tests, but every module has different requirements. The StoreKeys, AppModule, module account permissions, etc. whichever are required to create a new integration app differs from every module.
I think that's the downside of this new integration tests, everything needed should be manually given.
There was a problem hiding this comment.
Agreed. I think still eventually we can use depinject for the quick setup. We just do not need to inject an app.
…ita/staking-integration-tests
…ita/staking-integration-tests
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
Description
Closes: #14677
ref: #14145
This PR refactors the staking integration tests to use new testing framework setup
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
!in the type prefix if API or client breaking change