From ed6429f99fba8dd2604f33b413f63696002352fd Mon Sep 17 00:00:00 2001 From: Lukasz Cwik <126621805+lcwik@users.noreply.github.com> Date: Tue, 24 Oct 2023 15:20:41 -0700 Subject: [PATCH] [CLOB-930] Remove Reset method and swap to appOptions/baseAppOptions being explicitly set instead of an appCreatorFn (#697) This will simplify creating e2e test framework that better tests non-determinism/flags/... since the e2e test framework will want to control how the App instance is created. --- protocol/app/ante/gas_test.go | 8 ++-- .../full_node_prepare_proposal_test.go | 2 +- protocol/testutil/app/app.go | 45 +++++-------------- protocol/x/clob/e2e/app_test.go | 6 +-- protocol/x/clob/e2e/long_term_orders_test.go | 2 +- protocol/x/clob/e2e/short_term_orders_test.go | 17 ++++--- protocol/x/rewards/keeper/keeper_test.go | 9 +--- protocol/x/sending/app_test.go | 4 +- 8 files changed, 33 insertions(+), 60 deletions(-) diff --git a/protocol/app/ante/gas_test.go b/protocol/app/ante/gas_test.go index d0f4c7510a..5352e1be3b 100644 --- a/protocol/app/ante/gas_test.go +++ b/protocol/app/ante/gas_test.go @@ -181,10 +181,10 @@ func TestSubmitTxnWithGas(t *testing.T) { } tApp := testapp.NewTestAppBuilder(t). - WithAppCreatorFn( - testapp.DefaultTestAppCreatorFn(map[string]interface{}{}, - baseapp.SetMinGasPrices(cmd.MinGasPrice), - )). + WithAppOptions( + map[string]interface{}{}, + baseapp.SetMinGasPrices(cmd.MinGasPrice), + ). Build() ctx := tApp.InitChain() diff --git a/protocol/app/prepare/full_node_prepare_proposal_test.go b/protocol/app/prepare/full_node_prepare_proposal_test.go index bf74dce709..8380fb3e33 100644 --- a/protocol/app/prepare/full_node_prepare_proposal_test.go +++ b/protocol/app/prepare/full_node_prepare_proposal_test.go @@ -28,7 +28,7 @@ func TestFullNodePrepareProposalHandler(t *testing.T) { flags.NonValidatingFullNodeFlag: true, testlog.LoggerInstanceForTest: logger, } - tApp := testApp.NewTestAppBuilder(t).WithAppCreatorFn(testApp.DefaultTestAppCreatorFn(appOpts)).Build() + tApp := testApp.NewTestAppBuilder(t).WithAppOptions(appOpts).Build() found := false tApp.AdvanceToBlock(2, testApp.AdvanceToBlockOptions{ diff --git a/protocol/testutil/app/app.go b/protocol/testutil/app/app.go index 2c1e5da83d..db30882c1b 100644 --- a/protocol/testutil/app/app.go +++ b/protocol/testutil/app/app.go @@ -123,17 +123,6 @@ func DefaultTestApp(customFlags map[string]interface{}, baseAppOptions ...func(* return dydxApp } -// DefaultTestAppCreatorFn is a wrapper function around DefaultTestApp using the specified custom flags, and allowing -// for optional base app options. -func DefaultTestAppCreatorFn( - customFlags map[string]interface{}, - baseAppOptions ...func(*baseapp.BaseApp), -) AppCreatorFn { - return func() *app.App { - return DefaultTestApp(customFlags, baseAppOptions...) - } -} - // DefaultGenesis returns a genesis doc using configuration from the local net with a genesis time // equivalent to unix epoch + 1 nanosecond. We specifically use non-zero because stateful orders // validate that block time is non-zero (https://github.com/dydxprotocol/v4-chain/protocol/blob/ @@ -257,8 +246,8 @@ func NewTestAppBuilder(t testing.TB) TestAppBuilder { } return TestAppBuilder{ genesisDocFn: DefaultGenesis, - appCreatorFn: DefaultTestAppCreatorFn(nil), usesDefaultAppConfig: true, + appOptions: make(map[string]interface{}), executeCheckTxs: func(ctx sdk.Context, app *app.App) (stop bool) { return true }, @@ -272,8 +261,9 @@ func NewTestAppBuilder(t testing.TB) TestAppBuilder { // immutable. type TestAppBuilder struct { genesisDocFn GenesisDocCreatorFn - appCreatorFn func() *app.App usesDefaultAppConfig bool + appOptions map[string]interface{} + baseAppOptions []func(*baseapp.BaseApp) executeCheckTxs ExecuteCheckTxs t testing.TB } @@ -285,10 +275,13 @@ func (tApp TestAppBuilder) WithGenesisDocFn(fn GenesisDocCreatorFn) TestAppBuild return tApp } -// WithAppCreatorFn returns a builder like this one with the specified function that will be used to create -// the application. -func (tApp TestAppBuilder) WithAppCreatorFn(fn AppCreatorFn) TestAppBuilder { - tApp.appCreatorFn = fn +// WithAppOptions returns a builder like this one with the specified app options. +func (tApp TestAppBuilder) WithAppOptions( + appOptions map[string]interface{}, + baseAppOptions ...func(*baseapp.BaseApp), +) TestAppBuilder { + tApp.appOptions = appOptions + tApp.baseAppOptions = baseAppOptions tApp.usesDefaultAppConfig = false return tApp } @@ -334,7 +327,7 @@ func (tApp *TestApp) Builder() TestAppBuilder { // InitChain initializes the chain. Will panic if initialized more than once. func (tApp *TestApp) InitChain() sdk.Context { if tApp.App != nil { - panic(errors.New("Cannot initialize chain that has been initialized already. Missing a Reset()?")) + panic(errors.New("Cannot initialize chain that has been initialized already.")) } tApp.initChainIfNeeded() return tApp.App.NewContext(true, tApp.header) @@ -347,7 +340,7 @@ func (tApp *TestApp) initChainIfNeeded() { // Get the initial genesis state and initialize the chain and commit the results of the initialization. tApp.genesis = tApp.builder.genesisDocFn() - tApp.App = tApp.builder.appCreatorFn() + tApp.App = DefaultTestApp(tApp.builder.appOptions, tApp.builder.baseAppOptions...) if tApp.builder.usesDefaultAppConfig { tApp.App.Server.DisableUpdateMonitoringForTesting() } @@ -544,20 +537,6 @@ func (tApp *TestApp) AdvanceToBlock( return tApp.App.NewContext(true, tApp.header) } -// Reset resets the chain such that it can be initialized and executed again. -func (tApp *TestApp) Reset() { - if tApp.App != nil { - if err := tApp.App.Close(); err != nil { - tApp.builder.t.Fatal(err) - } - } - tApp.App = nil - tApp.genesis = types.GenesisDoc{} - tApp.header = tmproto.Header{} - tApp.passingCheckTxs = nil - tApp.halted = false -} - // GetHeader fetches the current header of the test app. func (tApp *TestApp) GetHeader() tmproto.Header { return tApp.header diff --git a/protocol/x/clob/e2e/app_test.go b/protocol/x/clob/e2e/app_test.go index 939850a495..d2de9e50b4 100644 --- a/protocol/x/clob/e2e/app_test.go +++ b/protocol/x/clob/e2e/app_test.go @@ -401,7 +401,7 @@ func TestFailsDeliverTxWithIncorrectlySignedPlaceOrderTx(t *testing.T) { appOpts := map[string]interface{}{ indexer.MsgSenderInstanceForTest: msgSender, } - tApp := testapp.NewTestAppBuilder(t).WithAppCreatorFn(testapp.DefaultTestAppCreatorFn(appOpts)).Build() + tApp := testapp.NewTestAppBuilder(t).WithAppOptions(appOpts).Build() tApp.InitChain() ctx := tApp.AdvanceToBlock(2, testapp.AdvanceToBlockOptions{}) @@ -474,7 +474,7 @@ func TestFailsDeliverTxWithUnsignedTransactions(t *testing.T) { appOpts := map[string]interface{}{ indexer.MsgSenderInstanceForTest: msgSender, } - tApp := testapp.NewTestAppBuilder(t).WithAppCreatorFn(testapp.DefaultTestAppCreatorFn(appOpts)).Build() + tApp := testapp.NewTestAppBuilder(t).WithAppOptions(appOpts).Build() tApp.InitChain() tApp.AdvanceToBlock(2, testapp.AdvanceToBlockOptions{}) @@ -506,7 +506,7 @@ func TestStats(t *testing.T) { appOpts := map[string]interface{}{ indexer.MsgSenderInstanceForTest: msgSender, } - tApp := testapp.NewTestAppBuilder(t).WithAppCreatorFn(testapp.DefaultTestAppCreatorFn(appOpts)).Build() + tApp := testapp.NewTestAppBuilder(t).WithAppOptions(appOpts).Build() // Epochs start at block height 2. startTime := time.Unix(10, 0).UTC() diff --git a/protocol/x/clob/e2e/long_term_orders_test.go b/protocol/x/clob/e2e/long_term_orders_test.go index e020da5030..ae630a4799 100644 --- a/protocol/x/clob/e2e/long_term_orders_test.go +++ b/protocol/x/clob/e2e/long_term_orders_test.go @@ -1221,7 +1221,7 @@ func TestPlaceLongTermOrder(t *testing.T) { appOpts := map[string]interface{}{ indexer.MsgSenderInstanceForTest: msgSender, } - tApp := testapp.NewTestAppBuilder(t).WithAppCreatorFn(testapp.DefaultTestAppCreatorFn(appOpts)).Build() + tApp := testapp.NewTestAppBuilder(t).WithAppOptions(appOpts).Build() ctx := tApp.InitChain() for _, ordersAndExpectations := range tc.ordersAndExpectationsPerBlock { diff --git a/protocol/x/clob/e2e/short_term_orders_test.go b/protocol/x/clob/e2e/short_term_orders_test.go index 3f09dd6373..d5b473454d 100644 --- a/protocol/x/clob/e2e/short_term_orders_test.go +++ b/protocol/x/clob/e2e/short_term_orders_test.go @@ -1,6 +1,7 @@ package clob_test import ( + "github.com/dydxprotocol/v4-chain/protocol/indexer" "testing" "golang.org/x/exp/slices" @@ -9,7 +10,6 @@ import ( "github.com/dydxprotocol/v4-chain/protocol/dtypes" "github.com/dydxprotocol/v4-chain/protocol/lib" - "github.com/dydxprotocol/v4-chain/protocol/indexer" indexerevents "github.com/dydxprotocol/v4-chain/protocol/indexer/events" "github.com/dydxprotocol/v4-chain/protocol/indexer/indexer_manager" "github.com/dydxprotocol/v4-chain/protocol/indexer/msgsender" @@ -25,11 +25,7 @@ import ( ) func TestPlaceOrder(t *testing.T) { - msgSender := msgsender.NewIndexerMessageSenderInMemoryCollector() - appOpts := map[string]interface{}{ - indexer.MsgSenderInstanceForTest: msgSender, - } - tApp := testapp.NewTestAppBuilder(t).WithAppCreatorFn(testapp.DefaultTestAppCreatorFn(appOpts)).Build() + tApp := testapp.NewTestAppBuilder(t).Build() ctx := tApp.InitChain() aliceSubaccount := tApp.App.SubaccountsKeeper.GetSubaccount(ctx, constants.Alice_Num0) @@ -568,10 +564,13 @@ func TestPlaceOrder(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - // Reset for each iteration of the loop - tApp.Reset() - + msgSender := msgsender.NewIndexerMessageSenderInMemoryCollector() + appOpts := map[string]interface{}{ + indexer.MsgSenderInstanceForTest: msgSender, + } + tApp = testapp.NewTestAppBuilder(t).WithAppOptions(appOpts).Build() ctx = tApp.InitChain() + // Clear any messages produced prior to these checkTx calls. msgSender.Clear() for _, order := range tc.orders { diff --git a/protocol/x/rewards/keeper/keeper_test.go b/protocol/x/rewards/keeper/keeper_test.go index 229fdbbc94..ef84a1dad1 100644 --- a/protocol/x/rewards/keeper/keeper_test.go +++ b/protocol/x/rewards/keeper/keeper_test.go @@ -80,8 +80,6 @@ func TestSetRewardShare_FailsWithNonpositiveWeight(t *testing.T) { } func TestAddRewardShareToAddress(t *testing.T) { - tApp := testapp.NewTestAppBuilder(t).Build() - tests := map[string]struct { prevRewardShare *types.RewardShare // nil if no previous share newWeight *big.Int @@ -120,7 +118,7 @@ func TestAddRewardShareToAddress(t *testing.T) { // Run tests. for name, tc := range tests { t.Run(name, func(t *testing.T) { - tApp.Reset() + tApp := testapp.NewTestAppBuilder(t).Build() ctx := tApp.InitChain() k := tApp.App.RewardsKeeper @@ -143,7 +141,6 @@ func TestAddRewardShareToAddress(t *testing.T) { } func TestAddRewardSharesForFill(t *testing.T) { - tApp := testapp.NewTestAppBuilder(t).Build() makerAddress := TestAddress1 takerAdderss := TestAddress2 @@ -268,7 +265,7 @@ func TestAddRewardSharesForFill(t *testing.T) { // Run tests. for name, tc := range tests { t.Run(name, func(t *testing.T) { - tApp.Reset() + tApp := testapp.NewTestAppBuilder(t).Build() ctx := tApp.InitChain() k := tApp.App.RewardsKeeper @@ -653,8 +650,6 @@ func TestProcessRewardsForBlock(t *testing.T) { ) return genesis }).Build() - - tApp.Reset() ctx := tApp.InitChain() k := tApp.App.RewardsKeeper diff --git a/protocol/x/sending/app_test.go b/protocol/x/sending/app_test.go index ac78bac745..88de257123 100644 --- a/protocol/x/sending/app_test.go +++ b/protocol/x/sending/app_test.go @@ -95,7 +95,7 @@ func TestMsgDepositToSubaccount(t *testing.T) { appOpts := map[string]interface{}{ indexer.MsgSenderInstanceForTest: msgSender, } - tApp := testapp.NewTestAppBuilder(t).WithAppCreatorFn(testapp.DefaultTestAppCreatorFn(appOpts)).Build() + tApp := testapp.NewTestAppBuilder(t).WithAppOptions(appOpts).Build() ctx := tApp.AdvanceToBlock(2, testapp.AdvanceToBlockOptions{}) // Clear any messages produced prior to CheckTx calls. msgSender.Clear() @@ -284,7 +284,7 @@ func TestMsgWithdrawFromSubaccount(t *testing.T) { appOpts := map[string]interface{}{ indexer.MsgSenderInstanceForTest: msgSender, } - tApp := testapp.NewTestAppBuilder(t).WithAppCreatorFn(testapp.DefaultTestAppCreatorFn(appOpts)).Build() + tApp := testapp.NewTestAppBuilder(t).WithAppOptions(appOpts).Build() ctx := tApp.AdvanceToBlock(2, testapp.AdvanceToBlockOptions{}) // Clear any messages produced prior to CheckTx calls. msgSender.Clear()