test(gov): add test for Tally function

Although rather complex, the `govKeeper.Tally` function has no unit
tests. This change adds a test that covers around 91% of the code, only
some unexpected errors are not covered.

If this change is accepted, another will follow to refactor the
function into smaller parts (without changing the test). This will
address the TODO on top, reduce complexity,  improve readability and
reusability.

This test should also help for issues like cosmos#11422 and cosmos#10353. It's more
comfortable to improve performance or completely rewrite the
implementation with a high code coverage.

The `setupGovKeeper` had to be modified because it was registering some
mocks expectations that cannot be overridden. It now takes an additional
variadic argument that can be used to better customize the mocks
expectations. Also, to improve readability, the mocks are all gathered in
a new specific `mocks` struct.

x/gov/keeper/common_test.go

@@ -54,13 +54,38 @@ func getTestProposal() []sdk.Msg {
 	}
 }
 
+type mocks struct {
+	acctKeeper         *govtestutil.MockAccountKeeper
+	bankKeeper         *govtestutil.MockBankKeeper
+	stakingKeeper      *govtestutil.MockStakingKeeper
+	distributionKeeper *govtestutil.MockDistributionKeeper
+}
+
+func mockAccountKeeperExpectations(ctx sdk.Context, m mocks) {
+	m.acctKeeper.EXPECT().GetModuleAddress(types.ModuleName).Return(govAcct).AnyTimes()
+	m.acctKeeper.EXPECT().GetModuleAddress(disttypes.ModuleName).Return(distAcct).AnyTimes()
+	m.acctKeeper.EXPECT().GetModuleAccount(gomock.Any(), types.ModuleName).Return(authtypes.NewEmptyModuleAccount(types.ModuleName)).AnyTimes()
+	m.acctKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes()
+}
+
+func mockDefaultExpectations(ctx sdk.Context, m mocks) {
+	mockAccountKeeperExpectations(ctx, m)
+	trackMockBalances(m.bankKeeper, m.distributionKeeper)
+	m.stakingKeeper.EXPECT().TokensFromConsensusPower(ctx, gomock.Any()).DoAndReturn(func(ctx sdk.Context, power int64) math.Int {
+		return sdk.TokensFromConsensusPower(power, math.NewIntFromUint64(1000000))
+	}).AnyTimes()
+
+	m.stakingKeeper.EXPECT().BondDenom(ctx).Return("stake", nil).AnyTimes()
+	m.stakingKeeper.EXPECT().IterateBondedValidatorsByPower(gomock.Any(), gomock.Any()).AnyTimes()
+	m.stakingKeeper.EXPECT().IterateDelegations(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
+	m.stakingKeeper.EXPECT().TotalBondedTokens(gomock.Any()).Return(math.NewInt(10000000), nil).AnyTimes()
+	m.distributionKeeper.EXPECT().FundCommunityPool(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+}
+
 // setupGovKeeper creates a govKeeper as well as all its dependencies.
-func setupGovKeeper(t *testing.T) (
+func setupGovKeeper(t *testing.T, expectations ...func(sdk.Context, mocks)) (
 	*keeper.Keeper,
-	*govtestutil.MockAccountKeeper,
-	*govtestutil.MockBankKeeper,
-	*govtestutil.MockStakingKeeper,
-	*govtestutil.MockDistributionKeeper,
+	mocks,
 	moduletestutil.TestEncodingConfig,
 	sdk.Context,
 ) {
@@ -80,30 +105,23 @@ func setupGovKeeper(t *testing.T) (
 
 	// gomock initializations
 	ctrl := gomock.NewController(t)
-	acctKeeper := govtestutil.NewMockAccountKeeper(ctrl)
-	bankKeeper := govtestutil.NewMockBankKeeper(ctrl)
-	stakingKeeper := govtestutil.NewMockStakingKeeper(ctrl)
-	distributionKeeper := govtestutil.NewMockDistributionKeeper(ctrl)
-
-	acctKeeper.EXPECT().GetModuleAddress(types.ModuleName).Return(govAcct).AnyTimes()
-	acctKeeper.EXPECT().GetModuleAddress(disttypes.ModuleName).Return(distAcct).AnyTimes()
-	acctKeeper.EXPECT().GetModuleAccount(gomock.Any(), types.ModuleName).Return(authtypes.NewEmptyModuleAccount(types.ModuleName)).AnyTimes()
-	acctKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes()
-
-	trackMockBalances(bankKeeper, distributionKeeper)
-	stakingKeeper.EXPECT().TokensFromConsensusPower(ctx, gomock.Any()).DoAndReturn(func(ctx sdk.Context, power int64) math.Int {
-		return sdk.TokensFromConsensusPower(power, math.NewIntFromUint64(1000000))
-	}).AnyTimes()
-
-	stakingKeeper.EXPECT().BondDenom(ctx).Return("stake", nil).AnyTimes()
-	stakingKeeper.EXPECT().IterateBondedValidatorsByPower(gomock.Any(), gomock.Any()).AnyTimes()
-	stakingKeeper.EXPECT().IterateDelegations(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
-	stakingKeeper.EXPECT().TotalBondedTokens(gomock.Any()).Return(math.NewInt(10000000), nil).AnyTimes()
-	distributionKeeper.EXPECT().FundCommunityPool(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+	m := mocks{
+		acctKeeper:         govtestutil.NewMockAccountKeeper(ctrl),
+		bankKeeper:         govtestutil.NewMockBankKeeper(ctrl),
+		stakingKeeper:      govtestutil.NewMockStakingKeeper(ctrl),
+		distributionKeeper: govtestutil.NewMockDistributionKeeper(ctrl),
+	}
+	if len(expectations) == 0 {
+		mockDefaultExpectations(ctx, m)
+	} else {
+		for _, exp := range expectations {
+			exp(ctx, m)
+		}
+	}
 
 	// Gov keeper initializations
 
-	govKeeper := keeper.NewKeeper(encCfg.Codec, storeService, acctKeeper, bankKeeper, stakingKeeper, distributionKeeper, msr, types.DefaultConfig(), govAcct.String())
+	govKeeper := keeper.NewKeeper(encCfg.Codec, storeService, m.acctKeeper, m.bankKeeper, m.stakingKeeper, m.distributionKeeper, msr, types.DefaultConfig(), govAcct.String())
 	require.NoError(t, govKeeper.ProposalID.Set(ctx, 1))
 	govRouter := v1beta1.NewRouter() // Also register legacy gov handlers to test them too.
 	govRouter.AddRoute(types.RouterKey, v1beta1.ProposalHandler)
@@ -118,7 +136,7 @@ func setupGovKeeper(t *testing.T) (
 	v1.RegisterMsgServer(msr, keeper.NewMsgServerImpl(govKeeper))
 	banktypes.RegisterMsgServer(msr, nil) // Nil is fine here as long as we never execute the proposal's Msgs.
 
-	return govKeeper, acctKeeper, bankKeeper, stakingKeeper, distributionKeeper, encCfg, ctx
+	return govKeeper, m, encCfg, ctx
 }
 
 // trackMockBalances sets up expected calls on the Mock BankKeeper, and also

x/gov/keeper/deposit_test.go

@@ -38,7 +38,8 @@ func TestDeposits(t *testing.T) {
 
 	for _, tc := range testcases {
 		t.Run(tc.name, func(t *testing.T) {
-			govKeeper, authKeeper, bankKeeper, stakingKeeper, distKeeper, _, ctx := setupGovKeeper(t)
+			govKeeper, mocks, _, ctx := setupGovKeeper(t)
+			authKeeper, bankKeeper, stakingKeeper, distKeeper := mocks.acctKeeper, mocks.bankKeeper, mocks.stakingKeeper, mocks.distributionKeeper
 			trackMockBalances(bankKeeper, distKeeper)
 
 			// With expedited proposals the minimum deposit is higher, so we must
@@ -249,7 +250,7 @@ func TestValidateInitialDeposit(t *testing.T) {
 
 	for name, tc := range testcases {
 		t.Run(name, func(t *testing.T) {
-			govKeeper, _, _, _, _, _, ctx := setupGovKeeper(t)
+			govKeeper, _, _, ctx := setupGovKeeper(t)
 
 			params := v1.DefaultParams()
 			if tc.expedited {
@@ -312,7 +313,8 @@ func TestChargeDeposit(t *testing.T) {
 			}
 
 			t.Run(testName(i), func(t *testing.T) {
-				govKeeper, authKeeper, bankKeeper, stakingKeeper, _, _, ctx := setupGovKeeper(t)
+				govKeeper, mocks, _, ctx := setupGovKeeper(t)
+				authKeeper, bankKeeper, stakingKeeper := mocks.acctKeeper, mocks.bankKeeper, mocks.stakingKeeper
 				params := v1.DefaultParams()
 				params.ProposalCancelRatio = tc.proposalCancelRatio
 				TestAddrs := simtestutil.AddTestAddrsIncremental(bankKeeper, stakingKeeper, ctx, 2, sdkmath.NewInt(10000000000))

x/gov/keeper/hooks_test.go

@@ -49,7 +49,8 @@ func (h *MockGovHooksReceiver) AfterProposalVotingPeriodEnded(ctx context.Contex
 
 func TestHooks(t *testing.T) {
 	minDeposit := v1.DefaultParams().MinDeposit
-	govKeeper, authKeeper, bankKeeper, stakingKeeper, _, _, ctx := setupGovKeeper(t)
+	govKeeper, mocks, _, ctx := setupGovKeeper(t)
+	authKeeper, bankKeeper, stakingKeeper := mocks.acctKeeper, mocks.bankKeeper, mocks.stakingKeeper
 	addrs := simtestutil.AddTestAddrs(bankKeeper, stakingKeeper, ctx, 1, minDeposit[0].Amount)
 
 	authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes()

x/gov/keeper/keeper_test.go

@@ -45,7 +45,8 @@ func (suite *KeeperTestSuite) SetupSuite() {
 }
 
 func (suite *KeeperTestSuite) reset() {
-	govKeeper, acctKeeper, bankKeeper, stakingKeeper, distKeeper, encCfg, ctx := setupGovKeeper(suite.T())
+	govKeeper, mocks, encCfg, ctx := setupGovKeeper(suite.T())
+	acctKeeper, bankKeeper, stakingKeeper, distKeeper := mocks.acctKeeper, mocks.bankKeeper, mocks.stakingKeeper, mocks.distributionKeeper
 
 	// Populate the gov account with some coins, as the TestProposal we have
 	// is a MsgSend from the gov account.
@@ -80,7 +81,8 @@ func (suite *KeeperTestSuite) reset() {
 }
 
 func TestIncrementProposalNumber(t *testing.T) {
-	govKeeper, authKeeper, _, _, _, _, ctx := setupGovKeeper(t)
+	govKeeper, mocks, _, ctx := setupGovKeeper(t)
+	authKeeper := mocks.acctKeeper
 
 	authKeeper.EXPECT().AddressCodec().Return(address.NewBech32Codec("cosmos")).AnyTimes()
 
@@ -106,7 +108,8 @@ func TestIncrementProposalNumber(t *testing.T) {
 }
 
 func TestProposalQueues(t *testing.T) {
-	govKeeper, authKeeper, _, _, _, _, ctx := setupGovKeeper(t)
+	govKeeper, mocks, _, ctx := setupGovKeeper(t)
+	authKeeper := mocks.acctKeeper
 
 	ac := address.NewBech32Codec("cosmos")
 	addrBz, err := ac.StringToBytes(address1)
@@ -133,7 +136,7 @@ func TestProposalQueues(t *testing.T) {
 }
 
 func TestSetHooks(t *testing.T) {
-	govKeeper, _, _, _, _, _, _ := setupGovKeeper(t)
+	govKeeper, _, _, _ := setupGovKeeper(t)
 	require.Empty(t, govKeeper.Hooks())
 
 	govHooksReceiver := MockGovHooksReceiver{}
@@ -145,7 +148,8 @@ func TestSetHooks(t *testing.T) {
 }
 
 func TestGetGovGovernanceAndModuleAccountAddress(t *testing.T) {
-	govKeeper, authKeeper, _, _, _, _, ctx := setupGovKeeper(t)
+	govKeeper, mocks, _, ctx := setupGovKeeper(t)
+	authKeeper := mocks.acctKeeper
 	mAcc := authKeeper.GetModuleAccount(ctx, "gov")
 	require.Equal(t, mAcc, govKeeper.GetGovernanceAccount(ctx))
 	mAddr := authKeeper.GetModuleAddress("gov")