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.

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

test(types/tx/msgs.go): add unit tests #20199

Merged
merged 10 commits into from
May 27, 2024
182 changes: 182 additions & 0 deletions types/tx/msgs_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
package tx_test
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this? Unless you have to avoid cicular dependencies or want to call from the "outside", the _test package mostly just adds complexity, IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this practice because it improving package design since using the _test package forces you to test the package as if you are an external user, it encourages you to think more carefully about the design of your package’s API. You'll need to ensure that all necessary functionality is properly exposed through exported types and functions, which can lead to better, more user-friendly design. If you have problems with testing and using package probably the clients of the package will have the same problems.


import (
"encoding/json"
"testing"

"github.com/stretchr/testify/require"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx"
)

func Test_SetMsg(t *testing.T) {
cases := map[string]struct {
msg sdk.Msg
msgTypeURL string
expErr bool
}{
"Set nil Msg": {
msg: nil,
expErr: true,
},
"Set empty message": {
msg: &DummyProtoMessage1{},
msgTypeURL: "/dummy.proto.message1",
expErr: false,
},
"Set a valid message": {
msg: &DummyProtoMessage1{Name: "some-name"},
msgTypeURL: "/dummy.proto.message1",
expErr: false,
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
actual, err := tx.SetMsg(tc.msg)
if tc.expErr {
require.Error(t, err)
return
}
require.NoError(t, err)

b, err := json.Marshal(tc.msg)
require.NoError(t, err)

require.Equal(t, tc.msgTypeURL, actual.TypeUrl)
require.Equal(t, b, actual.GetValue())
require.Equal(t, tc.msg, actual.GetCachedValue())
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the map type easier to read and you can get rid of the name field. For example:

	specs := map[string]struct{
        	msg    sdk.Msg
		expErr bool
	}{
	"Set nil Msg": {
		msg:    nil,
		expErr: true,
         },
	}
	for name, spec := range specs {
	t.Run(name, func(t *testing.T) {
	            
	})
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment. I really like how it looks with a map. I will start using map[string]struct{} instead struct{}.

Fixed


func Test_SetMsgs(t *testing.T) {
cases := map[string]struct {
msgs []sdk.Msg
msgTypeURL []string
expErr bool
}{
"Set nil slice of messages": {
msgs: nil,
expErr: false,
},
"Set empty slice of messages": {
msgs: []sdk.Msg{},
expErr: false,
},
"Set nil message inside the slice of messages": {
msgs: []sdk.Msg{nil},
expErr: true,
},
"Set valid messages": {
msgs: []sdk.Msg{&DummyProtoMessage1{Name: "name1"}, &DummyProtoMessage2{}},
msgTypeURL: []string{"/dummy.proto.message1", "/dummy.proto.message2"},
expErr: false,
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
actual, err := tx.SetMsgs(tc.msgs)
if tc.expErr {
require.Error(t, err)
return
}
require.NoError(t, err)
require.Equal(t, len(tc.msgs), len(actual))

for i, msg := range tc.msgs {
b, err := json.Marshal(msg)
require.NoError(t, err)

require.Equal(t, msg, actual[i].GetCachedValue())
require.Equal(t, tc.msgTypeURL[i], actual[i].GetTypeUrl())
require.Equal(t, b, actual[i].GetValue())
}
})
}
}

func Test_GetMsgs(t *testing.T) {
sdkMsgs := []sdk.Msg{&DummyProtoMessage1{}, &DummyProtoMessage2{}}
anyMsg, err := tx.SetMsgs(sdkMsgs)
require.NoError(t, err)

cases := map[string]struct {
msgs []*types.Any
expected []sdk.Msg
expErr bool
}{
"GetMsgs from a nil slice of Any messages": {
msgs: nil,
expected: []sdk.Msg{},
expErr: false,
},
"GetMsgs from empty slice of Any messages": {
msgs: []*types.Any{},
expected: []sdk.Msg{},
expErr: false,
},
"GetMsgs from a slice with valid Any messages": {
msgs: anyMsg,
expected: sdkMsgs,
expErr: false,
},
"GetMsgs from a slice that contains uncached Any message": {
msgs: []*types.Any{{}},
expErr: true,
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
actual, err := tx.GetMsgs(tc.msgs, "dummy")
if tc.expErr {
require.Error(t, err)
return
}

require.NoError(t, err)
require.Equal(t, tc.expected, actual)
})
}
}

func TestTx_UnpackInterfaces(t *testing.T) {
unpacker := codec.NewProtoCodec(types.NewInterfaceRegistry())
sdkMsgs := []sdk.Msg{&DummyProtoMessage1{}, &DummyProtoMessage2{}}
anyMsg, err := tx.SetMsgs(sdkMsgs)
require.NoError(t, err)

cases := map[string]struct {
msgs []*types.Any
expErr bool
}{
"Unpack nil slice messages": {
msgs: nil,
expErr: false,
},
"Unpack empty slice of messages": {
msgs: []*types.Any{},
expErr: false,
},
"Unpack valid messages": {
msgs: anyMsg,
expErr: false,
},
"Unpack uncashed message": {
msgs: []*types.Any{{TypeUrl: "uncached"}},
expErr: true,
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
err = tx.UnpackInterfaces(unpacker, tc.msgs)
require.Equal(t, tc.expErr, err != nil)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

good test cases

}
88 changes: 48 additions & 40 deletions types/tx/types_test.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
package tx
package tx_test

Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.

import (
"encoding/hex"
"encoding/json"
"testing"

"github.com/cosmos/gogoproto/proto"
Expand All @@ -15,6 +16,7 @@ import (
"github.com/cosmos/cosmos-sdk/codec/address"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx"
)

func TestTx_GetMsgs(t *testing.T) {
Expand Down Expand Up @@ -56,8 +58,8 @@ func TestTx_GetMsgs(t *testing.T) {

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
transaction := Tx{
Body: &TxBody{
transaction := tx.Tx{
Body: &tx.TxBody{
Messages: tc.msgs,
Comment on lines +61 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using require.PanicsWithValue to check for specific panic messages, enhancing the clarity and robustness of your panic-related tests.

},
}
Expand All @@ -77,7 +79,7 @@ func TestTx_GetMsgs(t *testing.T) {
func TestTx_ValidateBasic(t *testing.T) {
cases := []struct {
name string
transaction *Tx
transaction *tx.Tx
expErr bool
}{
{
Expand All @@ -87,46 +89,46 @@ func TestTx_ValidateBasic(t *testing.T) {
},
{
name: "Tx without body",
transaction: &Tx{},
transaction: &tx.Tx{},
expErr: true,
},
{
name: "Tx without AuthInfo",
transaction: &Tx{Body: &TxBody{}},
transaction: &tx.Tx{Body: &tx.TxBody{}},
expErr: true,
},
{
name: "Tx without Fee",
transaction: &Tx{Body: &TxBody{}, AuthInfo: &AuthInfo{}},
transaction: &tx.Tx{Body: &tx.TxBody{}, AuthInfo: &tx.AuthInfo{}},
expErr: true,
},
{
name: "Tx with gas limit greater than Max gas wanted",
transaction: &Tx{Body: &TxBody{}, AuthInfo: &AuthInfo{Fee: &Fee{GasLimit: MaxGasWanted + 1}}},
transaction: &tx.Tx{Body: &tx.TxBody{}, AuthInfo: &tx.AuthInfo{Fee: &tx.Fee{GasLimit: tx.MaxGasWanted + 1}}},
expErr: true,
},
{
name: "Tx without Fee Amount",
transaction: &Tx{Body: &TxBody{}, AuthInfo: &AuthInfo{Fee: &Fee{GasLimit: MaxGasWanted}}},
transaction: &tx.Tx{Body: &tx.TxBody{}, AuthInfo: &tx.AuthInfo{Fee: &tx.Fee{GasLimit: tx.MaxGasWanted}}},
expErr: true,
},
{
name: "Tx with negative Fee Amount",
transaction: &Tx{
Body: &TxBody{},
AuthInfo: &AuthInfo{
Fee: &Fee{GasLimit: MaxGasWanted, Amount: sdk.Coins{sdk.Coin{Amount: math.NewInt(-1)}}},
transaction: &tx.Tx{
Body: &tx.TxBody{},
AuthInfo: &tx.AuthInfo{
Fee: &tx.Fee{GasLimit: tx.MaxGasWanted, Amount: sdk.Coins{sdk.Coin{Amount: math.NewInt(-1)}}},
},
},
expErr: true,
},
{
name: "Tx with invalid fee payer address",
transaction: &Tx{
Body: &TxBody{},
AuthInfo: &AuthInfo{
Fee: &Fee{
GasLimit: MaxGasWanted,
transaction: &tx.Tx{
Body: &tx.TxBody{},
AuthInfo: &tx.AuthInfo{
Fee: &tx.Fee{
GasLimit: tx.MaxGasWanted,
Payer: "invalidPayerAddress",
Amount: sdk.Coins{sdk.NewCoin("aaa", math.NewInt(10))},
},
Expand All @@ -136,11 +138,11 @@ func TestTx_ValidateBasic(t *testing.T) {
},
{
name: "Tx without signature",
transaction: &Tx{
Body: &TxBody{},
AuthInfo: &AuthInfo{
Fee: &Fee{
GasLimit: MaxGasWanted,
transaction: &tx.Tx{
Body: &tx.TxBody{},
AuthInfo: &tx.AuthInfo{
Fee: &tx.Fee{
GasLimit: tx.MaxGasWanted,
Payer: "cosmos1ulav3hsenupswqfkw2y3sup5kgtqwnvqa8eyhs",
Amount: sdk.Coins{sdk.NewCoin("aaa", math.NewInt(11))},
},
Expand All @@ -150,11 +152,11 @@ func TestTx_ValidateBasic(t *testing.T) {
},
{
name: "Tx is valid",
transaction: &Tx{
Body: &TxBody{},
AuthInfo: &AuthInfo{
Fee: &Fee{
GasLimit: MaxGasWanted,
transaction: &tx.Tx{
Body: &tx.TxBody{},
AuthInfo: &tx.AuthInfo{
Fee: &tx.Fee{
GasLimit: tx.MaxGasWanted,
Payer: "cosmos1ulav3hsenupswqfkw2y3sup5kgtqwnvqa8eyhs",
Amount: sdk.Coins{sdk.NewCoin("aaa", math.NewInt(11))},
},
Expand All @@ -174,11 +176,11 @@ func TestTx_ValidateBasic(t *testing.T) {
}

func TestTx_GetSigners(t *testing.T) {
transaction := &Tx{
Body: &TxBody{},
AuthInfo: &AuthInfo{
Fee: &Fee{
GasLimit: MaxGasWanted,
transaction := &tx.Tx{
Body: &tx.TxBody{},
AuthInfo: &tx.AuthInfo{
Fee: &tx.Fee{
GasLimit: tx.MaxGasWanted,
Payer: "cosmos1ulav3hsenupswqfkw2y3sup5kgtqwnvqa8eyhs",
Comment on lines +179 to +183
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding more test cases to cover scenarios with multiple signers and invalid signer addresses to ensure comprehensive coverage of the GetSigners method.

Amount: sdk.Coins{sdk.NewCoin("aaa", math.NewInt(11))},
},
Expand Down Expand Up @@ -206,17 +208,23 @@ func TestTx_GetSigners(t *testing.T) {
require.Nil(t, err)
}

type DummyProtoMessage1 struct{}
type DummyProtoMessage1 struct {
Name string
}

func (d *DummyProtoMessage1) Reset() {}
func (d *DummyProtoMessage1) String() string { return "/dummy.proto.message1" }
func (d *DummyProtoMessage1) ProtoMessage() {}
func (d *DummyProtoMessage1) Reset() {}
func (d *DummyProtoMessage1) String() string { return "/dummy.proto.message1" }
func (d *DummyProtoMessage1) ProtoMessage() {}
func (d *DummyProtoMessage1) Marshal() ([]byte, error) { return json.Marshal(d) }
func (d *DummyProtoMessage1) XXX_MessageName() string { return "dummy.proto.message1" }

type DummyProtoMessage2 struct{}

func (d *DummyProtoMessage2) Reset() {}
func (d *DummyProtoMessage2) String() string { return "/dummy.proto.message2" }
func (d *DummyProtoMessage2) ProtoMessage() {}
func (d *DummyProtoMessage2) Reset() {}
func (d *DummyProtoMessage2) String() string { return "/dummy.proto.message2" }
func (d *DummyProtoMessage2) ProtoMessage() {}
func (d *DummyProtoMessage2) Marshal() ([]byte, error) { return json.Marshal(d) }
func (d *DummyProtoMessage2) XXX_MessageName() string { return "dummy.proto.message2" }

type dummyAddressCodec struct{}

Expand Down