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

imp: event assertion tests function #2829

Merged
merged 22 commits into from
Dec 12, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3267c7a
Update ibc_transfer event testing to use AssertEvents from common module
Anmol1696 Nov 25, 2022
126b73e
Update assertion of events
Anmol1696 Nov 26, 2022
517d006
Add changelog, update AssertEvents to pass as pointer
Anmol1696 Nov 26, 2022
3ce88e0
Pass suite by pointer to ibctest
Anmol1696 Nov 26, 2022
489d184
Remove nil and zero values of expected map
Anmol1696 Nov 27, 2022
c5588ea
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Nov 28, 2022
d861ce3
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Nov 29, 2022
f507ef7
Update testing/events.go
Anmol1696 Nov 29, 2022
9991a42
Update testing/events.go
Anmol1696 Nov 29, 2022
4283fd6
Add type to events map
Anmol1696 Nov 29, 2022
c616dae
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Nov 30, 2022
9d319fe
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Dec 1, 2022
f5a47e8
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Dec 2, 2022
3c46d8d
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Dec 4, 2022
360646a
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Dec 5, 2022
0a7a042
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Dec 8, 2022
54c2110
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Dec 10, 2022
1585467
Use expPass instead of hasEvents for checking for events
Anmol1696 Dec 10, 2022
3cc23c4
Combine the code blocks for verification of events
Anmol1696 Dec 10, 2022
7ee6b8b
Remove unused function assertTransferEvents
Anmol1696 Dec 12, 2022
966eb13
Merge branch 'main' into anmol/event-tests-transfer
Anmol1696 Dec 12, 2022
a6da8c6
Merge branch 'main' into anmol/event-tests-transfer
colin-axner Dec 12, 2022
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (core/24-host) [\#2820](https://github.com/cosmos/ibc-go/pull/2820) Add `MustParseClientStatePath` which parses the clientID from a client state key path.
* (apps/27-interchain-accounts) [\#2147](https://github.com/cosmos/ibc-go/pull/2147) Adding a `SubmitTx` gRPC endpoint for the ICS27 Controller module which allows owners of interchain accounts to submit transactions. This replaces the previously existing need for authentication modules to implement this standard functionality.
* (testing/simapp) [\#2190](https://github.com/cosmos/ibc-go/pull/2190) Adding the new `x/group` cosmos-sdk module to simapp.
* (testing) [\#2829](https://github.com/cosmos/ibc-go/pull/2829) Add `AssertEvents` which asserts events against expected event map.

### Bug Fixes

Expand Down
32 changes: 26 additions & 6 deletions modules/apps/transfer/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper_test
import (
sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
ibctesting "github.com/cosmos/ibc-go/v6/testing"

"github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
)
Expand Down Expand Up @@ -38,7 +39,10 @@ func (suite *KeeperTestSuite) assertTransferEvents(
}

func (suite *KeeperTestSuite) TestMsgTransfer() {
var msg *types.MsgTransfer
var (
msg *types.MsgTransfer
hasEvents bool
)

testCases := []struct {
name string
Expand All @@ -47,12 +51,15 @@ func (suite *KeeperTestSuite) TestMsgTransfer() {
}{
{
"success",
func() {},
func() {
hasEvents = true
},
true,
},
{
"bank send enabled for denom",
func() {
hasEvents = true
suite.chainA.GetSimApp().BankKeeper.SetParams(suite.chainA.GetContext(),
banktypes.Params{
SendEnabled: []*banktypes.SendEnabled{{Denom: sdk.DefaultBondDenom, Enabled: true}},
Expand Down Expand Up @@ -109,6 +116,7 @@ func (suite *KeeperTestSuite) TestMsgTransfer() {
for _, tc := range testCases {
suite.Run(tc.name, func() {
suite.SetupTest()
hasEvents = false // must be explicitly set

path := NewTransferPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)
Expand All @@ -131,14 +139,26 @@ func (suite *KeeperTestSuite) TestMsgTransfer() {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().NotEqual(res.Sequence, uint64(0))

events := ctx.EventManager().Events()
suite.assertTransferEvents(events, coin, "memo")
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
} else {
suite.Require().Error(err)
suite.Require().Nil(res)
}

events := ctx.EventManager().Events()
// Verify events
events := ctx.EventManager().Events()
expEvents := map[string]map[string]string{
"ibc_transfer": {
"sender": suite.chainA.SenderAccount.GetAddress().String(),
"receiver": suite.chainB.SenderAccount.GetAddress().String(),
"amount": coin.Amount.String(),
"denom": coin.Denom,
"memo": "memo",
},
}

if hasEvents {
ibctesting.AssertEvents(&suite.Suite, expEvents, events)
crodriguezvega marked this conversation as resolved.
Show resolved Hide resolved
} else {
suite.Require().Len(events, 0)
}
})
Expand Down
31 changes: 31 additions & 0 deletions testing/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"strconv"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/suite"

clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
connectiontypes "github.com/cosmos/ibc-go/v6/modules/core/03-connection/types"
Expand Down Expand Up @@ -129,3 +130,33 @@ func ParseAckFromEvents(events sdk.Events) ([]byte, error) {
}
return nil, fmt.Errorf("acknowledgement event attribute not found")
}

// AssertEvents asserts that expected events are present in the actual events
Anmol1696 marked this conversation as resolved.
Show resolved Hide resolved
// expected map needs to be a subset of actual events to pass
Anmol1696 marked this conversation as resolved.
Show resolved Hide resolved
func AssertEvents(
suite *suite.Suite,
expected map[string]map[string]string,
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to use map[string]map[string]string as opposed to taking an sdk.Events directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wanted the map of expected events being readable as a map for the test itself.

We can change this to sdk.Events as well, but i chose readability of expected events. Do you think we should change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, I'm happy with it as a map, I was just curious as to why a different type was used. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, this snipet is the reason, to me this just makes intutive sense of what to expect from the event. https://github.com/cosmos/ibc-go/pull/2829/files#diff-ae66bd29b0c68b16e3fc64fb2d8c88dcc90ef4ed6c1f67be36baf8d693473f8eR150-R156

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible though to make a type definition for map[string]map[string]string just to improve readability? Like eventsMap or something like that? Something that signals the purpose of the type more clearly than the mouthful of map[string]map[string]string. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense will do

actual sdk.Events,
) {
hasEvents := make(map[string]bool)
for eventType := range expected {
hasEvents[eventType] = false
}

for _, event := range actual {
expEvent, eventFound := expected[event.Type]
if eventFound {
hasEvents[event.Type] = true
suite.Require().Len(event.Attributes, len(expEvent))
for _, attr := range event.Attributes {
expValue, found := expEvent[string(attr.Key)]
suite.Require().True(found)
suite.Require().Equal(expValue, string(attr.Value))
}
}
}

for eventName, hasEvent := range hasEvents {
suite.Require().True(hasEvent, "event: %s was not found in events", eventName)
}
}