Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: remove api that should not be used #15678

Merged
merged 6 commits into from
Apr 3, 2023
Merged

test: remove api that should not be used #15678

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e978de0
test: remove api that should not be used
julienrbrt Apr 3, 2023
e602b01
updates
julienrbrt Apr 3, 2023
1fc92a7
feedback
julienrbrt Apr 3, 2023
e94867d
improve comment
julienrbrt Apr 3, 2023
465cb07
Merge branch 'main' into julien/clean-up
julienrbrt Apr 3, 2023
5e25fac
Merge branch 'main' into julien/clean-up
julienrbrt Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions tests/integration/bank/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
cmttime "github.com/cometbft/cometbft/types/time"
"github.com/stretchr/testify/require"
"gotest.tools/v3/assert"

storetypes "cosmossdk.io/store/types"
Expand All @@ -20,7 +21,6 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/runtime"
"github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/configurator"
"github.com/cosmos/cosmos-sdk/testutil/sims"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -242,15 +242,15 @@ func TestSupply_SendCoins(t *testing.T) {
authKeeper.SetModuleAccount(ctx, burnerAcc)
authKeeper.SetAccount(ctx, baseAcc)

testutil.AssertPanics(t, func() {
require.Panics(t, func() {
_ = keeper.SendCoinsFromModuleToModule(ctx, "", holderAcc.GetName(), initCoins) //nolint:errcheck // no error check is needed because we are testing for a panic
})

testutil.AssertPanics(t, func() {
require.Panics(t, func() {
_ = keeper.SendCoinsFromModuleToModule(ctx, authtypes.Burner, "", initCoins) //nolint:errcheck // no error check is needed because we are testing for a panic
})

testutil.AssertPanics(t, func() {
require.Panics(t, func() {
_ = keeper.SendCoinsFromModuleToAccount(ctx, "", baseAcc.GetAddress(), initCoins) //nolint:errcheck // no error check is needed because we are testing for a panic
})

Expand Down Expand Up @@ -294,14 +294,14 @@ func TestSupply_MintCoins(t *testing.T) {
assert.NilError(t, err)

// no module account
testutil.AssertPanics(t, func() { keeper.MintCoins(ctx, "", initCoins) }) //nolint:errcheck // we're testing for a panic
require.Panics(t, func() { keeper.MintCoins(ctx, "", initCoins) }) //nolint:errcheck // we're testing for a panic
// invalid permission
testutil.AssertPanics(t, func() { keeper.MintCoins(ctx, authtypes.Burner, initCoins) }) //nolint:errcheck // we're testing for a panic
require.Panics(t, func() { keeper.MintCoins(ctx, authtypes.Burner, initCoins) }) //nolint:errcheck // we're testing for a panic

err = keeper.MintCoins(ctx, authtypes.Minter, sdk.Coins{sdk.Coin{Denom: "denom", Amount: sdk.NewInt(-10)}})
assert.Error(t, err, fmt.Sprintf("%sdenom: invalid coins", sdk.NewInt(-10)))

testutil.AssertPanics(t, func() { keeper.MintCoins(ctx, randomPerm, initCoins) }) //nolint:errcheck // we're testing for a panic
require.Panics(t, func() { keeper.MintCoins(ctx, randomPerm, initCoins) }) //nolint:errcheck // we're testing for a panic

err = keeper.MintCoins(ctx, authtypes.Minter, initCoins)
assert.NilError(t, err)
Expand All @@ -323,7 +323,7 @@ func TestSupply_MintCoins(t *testing.T) {
assert.NilError(t, err)
assert.DeepEqual(t, initCoins, getCoinsByName(ctx, keeper, authKeeper, multiPermAcc.GetName()))
assert.DeepEqual(t, initialSupply.Add(initCoins...), totalSupply)
testutil.AssertPanics(t, func() { keeper.MintCoins(ctx, authtypes.Burner, initCoins) }) //nolint:errcheck // we're testing for a panic
require.Panics(t, func() { keeper.MintCoins(ctx, authtypes.Burner, initCoins) }) //nolint:errcheck // we're testing for a panic
}

func TestSupply_BurnCoins(t *testing.T) {
Expand All @@ -344,11 +344,11 @@ func TestSupply_BurnCoins(t *testing.T) {
supplyAfterInflation, _, err := keeper.GetPaginatedTotalSupply(ctx, &query.PageRequest{})
assert.NilError(t, err)
// no module account
testutil.AssertPanics(t, func() { keeper.BurnCoins(ctx, "", initCoins) }) //nolint:errcheck // we're testing for a panic
require.Panics(t, func() { keeper.BurnCoins(ctx, "", initCoins) }) //nolint:errcheck // we're testing for a panic
// invalid permission
testutil.AssertPanics(t, func() { keeper.BurnCoins(ctx, authtypes.Minter, initCoins) }) //nolint:errcheck // we're testing for a panic
require.Panics(t, func() { keeper.BurnCoins(ctx, authtypes.Minter, initCoins) }) //nolint:errcheck // we're testing for a panic
// random permission
testutil.AssertPanics(t, func() { keeper.BurnCoins(ctx, randomPerm, supplyAfterInflation) }) //nolint:errcheck // we're testing for a panic
require.Panics(t, func() { keeper.BurnCoins(ctx, randomPerm, supplyAfterInflation) }) //nolint:errcheck // we're testing for a panic
err = keeper.BurnCoins(ctx, authtypes.Burner, supplyAfterInflation)
assert.Error(t, err, fmt.Sprintf("spendable balance %s is smaller than %s: insufficient funds", initCoins, supplyAfterInflation))

Expand Down
4 changes: 2 additions & 2 deletions tests/integration/genutil/gentx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ import (

"cosmossdk.io/math"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/require"
"gotest.tools/v3/assert"

"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
sdktestutil "github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/configurator"
simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -297,7 +297,7 @@ func TestDeliverGenTxs(t *testing.T) {
tc.malleate()

if tc.expPass {
sdktestutil.AssertNotPanics(t, func() {
require.NotPanics(t, func() {
genutil.DeliverGenTxs(
f.ctx, genTxs, f.stakingKeeper, f.baseApp.DeliverTx,
f.encodingConfig.TxConfig,
Expand Down
6 changes: 3 additions & 3 deletions tests/integration/staking/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import (
"cosmossdk.io/simapp"
abci "github.com/cometbft/cometbft/abci/types"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/require"
"gotest.tools/v3/assert"

codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdktestutil "github.com/cosmos/cosmos-sdk/testutil"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/bank/testutil"
"github.com/cosmos/cosmos-sdk/x/staking"
Expand Down Expand Up @@ -137,7 +137,7 @@ func TestInitGenesis_PoolsBalanceMismatch(t *testing.T) {
BondDenom: "stake",
}

sdktestutil.AssertPanics(t, func() {
require.Panics(t, func() {
// setting validator status to bonded so the balance counts towards bonded pool
validator.Status = types.Bonded
app.StakingKeeper.InitGenesis(ctx, &types.GenesisState{
Expand All @@ -148,7 +148,7 @@ func TestInitGenesis_PoolsBalanceMismatch(t *testing.T) {
// "should panic because bonded pool balance is different from bonded pool coins",
)

sdktestutil.AssertPanics(t, func() {
require.Panics(t, func() {
// setting validator status to unbonded so the balance counts towards not bonded pool
validator.Status = types.Unbonded
app.StakingKeeper.InitGenesis(ctx, &types.GenesisState{
Expand Down
10 changes: 5 additions & 5 deletions tests/integration/staking/keeper/slash_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import (
"cosmossdk.io/math"
"cosmossdk.io/simapp"
cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/stretchr/testify/require"
"gotest.tools/v3/assert"

sdktestutil "github.com/cosmos/cosmos-sdk/testutil"
sdk "github.com/cosmos/cosmos-sdk/types"
banktestutil "github.com/cosmos/cosmos-sdk/x/bank/testutil"
"github.com/cosmos/cosmos-sdk/x/staking/keeper"
Expand Down Expand Up @@ -382,7 +382,7 @@ func TestSlashWithRedelegation(t *testing.T) {
_, found := app.StakingKeeper.GetValidatorByConsAddr(ctx, consAddr)
assert.Assert(t, found)

sdktestutil.AssertNotPanics(t, func() {
require.NotPanics(t, func() {
app.StakingKeeper.Slash(ctx, consAddr, 10, 10, fraction)
})
burnAmount := sdk.NewDecFromInt(app.StakingKeeper.TokensFromConsensusPower(ctx, 10)).Mul(fraction).TruncateInt()
Expand Down Expand Up @@ -415,7 +415,7 @@ func TestSlashWithRedelegation(t *testing.T) {
_, found = app.StakingKeeper.GetValidatorByConsAddr(ctx, consAddr)
assert.Assert(t, found)

sdktestutil.AssertNotPanics(t, func() {
require.NotPanics(t, func() {
app.StakingKeeper.Slash(ctx, consAddr, 10, 10, math.LegacyOneDec())
})
burnAmount = app.StakingKeeper.TokensFromConsensusPower(ctx, 7)
Expand Down Expand Up @@ -451,7 +451,7 @@ func TestSlashWithRedelegation(t *testing.T) {
_, found = app.StakingKeeper.GetValidatorByConsAddr(ctx, consAddr)
assert.Assert(t, found)

sdktestutil.AssertNotPanics(t, func() {
require.NotPanics(t, func() {
app.StakingKeeper.Slash(ctx, consAddr, 10, 10, math.LegacyOneDec())
})

Expand Down Expand Up @@ -486,7 +486,7 @@ func TestSlashWithRedelegation(t *testing.T) {
validator, _ = app.StakingKeeper.GetValidatorByConsAddr(ctx, consAddr)
assert.Equal(t, validator.GetStatus(), types.Unbonding)

sdktestutil.AssertNotPanics(t, func() {
require.NotPanics(t, func() {
app.StakingKeeper.Slash(ctx, consAddr, 10, 10, math.LegacyOneDec())
})

Expand Down
25 changes: 0 additions & 25 deletions testutil/assert_helpers.go
View file
Open in desktop

This file was deleted.

15 changes: 15 additions & 0 deletions testutil/compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package testutil

import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/testing/protocmp"
)

// ProtoDeepEqual is a helper function that uses the protocmp package to compare two protobuf messages.
func ProtoDeepEqual(t *testing.T, p1, p2 interface{}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RequireProtoDeepEqual is probably a better function name.

RequireProtoDeepEqual fails the test t if p1 and p2 are not equivalent protobuf messages.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is why I am going to request reviews from you more often :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For that matter, I think p1 and p2 should be of type proto.Message iirc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmp.Diff accepts interface{}, so I think we should accept interfaces as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmp.Diff does accept any, but I'm not sure what the intended behavior is if a programmer mistakenly passes a non-proto.Message type when using protocmp.Transform. If there is a runtime failure in that case, then it would be better to fail earlier, at compile time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, maybe proto.Message would be too restrictive if you were comparing slices instead. Maybe the doc should mention that the function can handle values or slices in the case of continuing to accept any for p1 and p2.

t.Helper()
require.Empty(t, cmp.Diff(p1, p2, protocmp.Transform()))
}