test(types/tx/msgs.go): add unit tests #20199
Conversation
WalkthroughThe changes involve renaming the Go package from Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
types/tx/msgs_test.go
Outdated
| "github.com/cosmos/cosmos-sdk/codec/types" | ||
| codectypes "github.com/cosmos/cosmos-sdk/codec/types" |
There was a problem hiding this comment.
| "github.com/cosmos/cosmos-sdk/codec/types" | |
| codectypes "github.com/cosmos/cosmos-sdk/codec/types" | |
| "github.com/cosmos/cosmos-sdk/codec/types" |
types/tx/msgs_test.go
Outdated
| return | ||
| } | ||
|
|
||
| expected := mustAny(tc.msg) |
There was a problem hiding this comment.
This calls the same types.NewAnyWithValue method as tx.SetMsg. If you want to ensure that the Any type has the expected data then you can verify cache, url and bytes. Or Unmarshal with and without cache
There was a problem hiding this comment.
Agree. It will be better to test cache, url and bytes. Thank you for the suggestion
Fixed. The function "mustAny" is removed
types/tx/msgs_test.go
Outdated
|
|
||
| expected := mustAny(tc.msg) | ||
| require.Equal(t, expected, actual) | ||
| require.Nil(t, err) |
There was a problem hiding this comment.
let's check this first to fail fast.
nit: here and others: require.NoError() should be preferred for better output
types/tx/msgs_test.go
Outdated
| } | ||
|
|
||
| require.Equal(t, expected, actual) | ||
| require.Nil(t, err) |
There was a problem hiding this comment.
Same comments as above
btw, personal preference: tx.SetMsg is called from one place only. maybe it can be refactored to use tx.SetMsgs(txs...sdk.Msg) ([]*types.Any, error) instead.
There was a problem hiding this comment.
Your idea is to remove tx.SetMsg and use tx.SetMsgs instead? tx.SetMsg is used only in one place in Cosmos-SDK, but it is exported and we don't know how many users use it their projects.
I think we should keep tx.SetMsg but we can simplified it from:
func SetMsg(msg sdk.Msg) (*types.Any, error) {
any, err := types.NewAnyWithValue(msg)
if err != nil {
return nil, err
}
return any, nil
}to:
func SetMsg(msg sdk.Msg) (*types.Any, error) {
return types.NewAnyWithValue(msg)
}
| err = tx.UnpackInterfaces(unpacker, tc.msgs) | ||
| require.Equal(t, tc.expErr, err != nil) | ||
| }) | ||
| } |
| require.Nil(t, err) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
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) {
})
}There was a problem hiding this comment.
Thanks for the comment. I really like how it looks with a map. I will start using map[string]struct{} instead struct{}.
Fixed
| @@ -1,9 +1,10 @@ | |||
| package tx | |||
| package tx_test | |||
| @@ -0,0 +1,195 @@ | |||
| package tx_test | |||
There was a problem hiding this comment.
Why this? Unless you have to avoid cicular dependencies or want to call from the "outside", the _test package mostly just adds complexity, IMHO.
There was a problem hiding this comment.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| transaction := tx.Tx{ | ||
| Body: &tx.TxBody{ |
There was a problem hiding this comment.
Consider using require.PanicsWithValue to check for specific panic messages, enhancing the clarity and robustness of your panic-related tests.
| transaction := &tx.Tx{ | ||
| Body: &tx.TxBody{}, | ||
| AuthInfo: &tx.AuthInfo{ | ||
| Fee: &tx.Fee{ | ||
| GasLimit: tx.MaxGasWanted, |
There was a problem hiding this comment.
Consider adding more test cases to cover scenarios with multiple signers and invalid signer addresses to ensure comprehensive coverage of the GetSigners method.
Description
Closes: #XXXX
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...
!in 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...
Summary by CodeRabbit
DummyProtoMessage1andDummyProtoMessage2.types/tx/msgs_test.gofor message manipulation within transaction context.