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

test: adding interchain accounts grpc query tests #368

Merged
merged 1 commit into from
Sep 2, 2021
Merged

test: adding interchain accounts grpc query tests #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
80 changes: 80 additions & 0 deletions modules/apps/27-interchain-accounts/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package keeper_test

import (
"fmt"

sdk "github.com/cosmos/cosmos-sdk/types"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"

"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types"
)

func (suite *KeeperTestSuite) TestQueryInterchainAccountAddress() {
var (
req *types.QueryInterchainAccountAddressRequest
expAddr string
)

testCases := []struct {
msg string
malleate func()
expPass bool
}{
{
"empty request",
func() {
req = &types.QueryInterchainAccountAddressRequest{}
Copy link
Member Author

Choose a reason for hiding this comment

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

This testcase is essentially redundant but also causes no harm as req is the same as below in "invalid counterparty portID" due to protobuf defaulting to empty string.

Using req = nil here causes a panic: invalid memory address or nil pointer dereference as protobuf attempts to marshal the nil request.

Can be removed if anyone feels it should. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to leave in.

Copy link
Contributor

@colin-axner colin-axner Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Might be good to update invalid counterparty portID to be empty with spaces (" ") and add strings.TrimSpace to the grpc.go code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with this, I think it's a good call. Can handle this in a PR tomorrow

},
false,
},
{
"invalid counterparty portID",
func() {
req = &types.QueryInterchainAccountAddressRequest{
CounterpartyPortId: "",
}
},
false,
},
{
"interchain account address not found",
func() {
req = &types.QueryInterchainAccountAddressRequest{
CounterpartyPortId: "ics-27",
}
},
false,
},
{
"success",
func() {
expAddr = authtypes.NewBaseAccountWithAddress(types.GenerateAddress("ics-27")).GetAddress().String()
req = &types.QueryInterchainAccountAddressRequest{
CounterpartyPortId: "ics-27",
}

suite.chainA.GetSimApp().ICAKeeper.SetInterchainAccountAddress(suite.chainA.GetContext(), "ics-27", expAddr)
},
true,
},
}

for _, tc := range testCases {
suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
suite.SetupTest() // reset

tc.malleate()
ctx := sdk.WrapSDKContext(suite.chainA.GetContext())

res, err := suite.queryClient.InterchainAccountAddress(ctx, req)

if tc.expPass {
suite.Require().NoError(err)
suite.Require().NotNil(res)
suite.Require().Equal(expAddr, res.GetInterchainAccountAddress())
} else {
suite.Require().Error(err)
}
})
}
}
7 changes: 7 additions & 0 deletions modules/apps/27-interchain-accounts/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package keeper_test
import (
"testing"

"github.com/cosmos/cosmos-sdk/baseapp"
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
"github.com/stretchr/testify/suite"

Expand All @@ -20,13 +21,19 @@ type KeeperTestSuite struct {
chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
chainC *ibctesting.TestChain

queryClient types.QueryClient
}

func (suite *KeeperTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(2))

queryHelper := baseapp.NewQueryServerTestHelper(suite.chainA.GetContext(), suite.chainA.GetSimApp().InterfaceRegistry())
types.RegisterQueryServer(queryHelper, suite.chainA.GetSimApp().ICAKeeper)
suite.queryClient = types.NewQueryClient(queryHelper)
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the legacy grpc testing, you should be able to use the query server without adding a field, see #143

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, nice! I had just referenced what had been done in apps/transfer

Will include with other changes in a follow up PR.

Copy link
Member Author

@damiannolan damiannolan Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I took a look at this. For apps/ I don't think we can use suite.chainA.QueryServer. as the query server that exists on the ibctesting.TestChain is defined here and only encapsulates core.

The other option which removes the queryClient field is to invoke the InterchainAccountAddress func directly on the ICAKeeper. This allows us to test with req = nil also.

res, err := suite.chainA.GetSimApp().ICAKeeper.InterchainAccountAddress(ctx, req)

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do the second option you outlined since the QueryServer is just the keeper anyways 👍

}

func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
Expand Down