refactor: remove dependency on simapp from baseapp tests #12543
Conversation
…efactor-baseapp-simapp
baseapp/block_gas_test.go
Outdated
| ) | ||
|
|
||
| appConfig := depinject.Configs(makeTestConfig(), | ||
| depinject.ProvideInModule("bank", |
There was a problem hiding this comment.
| depinject.ProvideInModule("bank", | |
| depinject.ProvideInModule(banktypes.ModuleName, |
baseapp/grpcrouter_test.go
Outdated
| @@ -53,11 +54,13 @@ func TestGRPCQueryRouter(t *testing.T) { | |||
|
|
|||
| func TestRegisterQueryServiceTwice(t *testing.T) { | |||
| // Setup baseapp. | |||
| var ( | |||
There was a problem hiding this comment.
One line declaration is better.
| } | ||
| // GenesisStateWithSingleValidator initializes GenesisState with a single validator and genesis accounts | ||
| // that also act as delegators. | ||
| func GenesisStateWithSingleValidator(t *testing.T, codec codec.Codec, builder *runtime.AppBuilder) map[string]json.RawMessage { |
There was a problem hiding this comment.
Maybe this is what you meant in your comment, but shouldn't this be in /testutil/sims instead? simapp.GenesisStateWithSingleValidator was used in other tests, so this will most likely too after complete decoupling.
There was a problem hiding this comment.
More or less; that function's signature looks like
func GenesisStateWithSingleValidator(t *testing.T, app *SimApp) GenesisState
so I can't use it here. Maybe we can live with a bit of code duplication until it's more clear?
kocubinski
added
the
A:automerge
| authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper" | ||
| xauthsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" | ||
| bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper" | ||
| banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" | ||
| minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" | ||
| paramskeeper "github.com/cosmos/cosmos-sdk/x/params/keeper" | ||
| stakingkeeper "github.com/cosmos/cosmos-sdk/x/staking/keeper" |
There was a problem hiding this comment.
im hesitant introducing these dependencies into baseapp. I think this should be rethought.
There was a problem hiding this comment.
baseapp already depended on these packages transitively through simapp. Now they are specified explicitly instead of implicitly. There is an overall reduction in the size of the dependency graph.
There was a problem hiding this comment.
yes, it was fine before. Im thinking more in the future when things become their own go mods. It could be a pre-optimization though. we can ignore for now
| runtimev1alpha1 "cosmossdk.io/api/cosmos/app/runtime/v1alpha1" | ||
| appv1alpha1 "cosmossdk.io/api/cosmos/app/v1alpha1" | ||
| authmodulev1 "cosmossdk.io/api/cosmos/auth/module/v1" | ||
| bankmodulev1 "cosmossdk.io/api/cosmos/bank/module/v1" | ||
| mintmodulev1 "cosmossdk.io/api/cosmos/mint/module/v1" | ||
| paramsmodulev1 "cosmossdk.io/api/cosmos/params/module/v1" | ||
| stakingmodulev1 "cosmossdk.io/api/cosmos/staking/module/v1" | ||
| txmodulev1 "cosmossdk.io/api/cosmos/tx/module/v1" | ||
| "cosmossdk.io/core/appconfig" | ||
| "cosmossdk.io/depinject" | ||
| "github.com/cosmos/cosmos-sdk/codec" | ||
| "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" | ||
| "github.com/cosmos/cosmos-sdk/runtime" | ||
| "github.com/cosmos/cosmos-sdk/testutil/mock" | ||
| simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| _ "github.com/cosmos/cosmos-sdk/x/auth" | ||
| _ "github.com/cosmos/cosmos-sdk/x/auth/tx/module" | ||
| authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" | ||
| _ "github.com/cosmos/cosmos-sdk/x/bank" | ||
| banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" | ||
| _ "github.com/cosmos/cosmos-sdk/x/mint" | ||
| minttypes "github.com/cosmos/cosmos-sdk/x/mint/types" | ||
| _ "github.com/cosmos/cosmos-sdk/x/params" | ||
| _ "github.com/cosmos/cosmos-sdk/x/staking" | ||
| stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" |
There was a problem hiding this comment.
|
in the future lets wait for two approvals, this was merged too fast. It didnt have enough eyes |
|
@marbar3778 responded to your concerns above. |
Description
Ref: #12535
simapp.MakeTestEncodingConfig()todepinject.simapp.NewSimApptodepinjectwith clear dependencies and explicit modules under test.testutil(yet) which introduces a transitive dependency onsimapp.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