Add additional IBC Channel Tests #5578
Conversation
jackzampolin
requested review from
alessio,
alexanderbez,
fedekunze and
rigelrozanski
as code owners
| ) | ||
|
|
||
| testCases := []testCase{ | ||
| // {"success", func() { |
There was a problem hiding this comment.
Do we still not have a working success case here?
| packet, err = suite.app.IBCKeeper.ChannelKeeper.RecvPacket(suite.ctx, packet, ibctypes.ValidProof{}, uint64(suite.ctx.BlockHeader().Height)) | ||
| suite.Require().NoError(err) | ||
| } else { | ||
| packet, err = suite.app.IBCKeeper.ChannelKeeper.RecvPacket(suite.ctx, packet, ibctypes.InvalidProof{}, uint64(suite.ctx.BlockHeader().Height)) |
There was a problem hiding this comment.
Should proof be part of testCase so that we know this is erroring for expected reason, and not always because we pass in invalid proof?
There was a problem hiding this comment.
Ideally yes, was difficult to write them that way. If you see a path to change that please push changes
There was a problem hiding this comment.
I think i may have a way, will look into it once all the ICS-7 stuff is merged. Doesn't have to block this pr tho
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(packetOut) | ||
| } else { | ||
| packetOut, err := suite.app.IBCKeeper.ChannelKeeper.AcknowledgePacket(suite.ctx, packet, ack, ibctypes.InvalidProof{}, uint64(suite.ctx.BlockHeader().Height)) |
| packet = types.NewPacket(mockSuccessPacket{}, 1, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID()) | ||
| suite.createChannel(testPort1, testChannel1, testPort2, testChannel2, exported.OPEN, exported.ORDERED, testConnectionID1) | ||
| }, false}, | ||
| {"connection not OPEN", func() { |
There was a problem hiding this comment.
should we have another test case with channel not open?
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(packetOut) | ||
| } else { | ||
| packetOut, err := suite.app.IBCKeeper.ChannelKeeper.CleanupPacket(suite.ctx, packet, ibctypes.InvalidProof{}, uint64(suite.ctx.BlockHeader().Height), nextSeqRecv, ack) |
| suite.Require().NoError(err) | ||
| suite.Require().NotNil(packetOut) | ||
| } else { | ||
| packetOut, err := suite.app.IBCKeeper.ChannelKeeper.TimeoutPacket(suite.ctx, packet, ibctypes.InvalidProof{}, proofHeight, nextSeqRecv) |
| tc.malleate() | ||
|
|
||
| var err error | ||
| if tc.expPass { |
There was a problem hiding this comment.
Using the variable on range scope tc in function literal (from scopelint)
| for i, tc := range testCases { | ||
| suite.Run(fmt.Sprintf("Case %s, %d/%d tests", tc.msg, i, len(testCases)), func() { | ||
| suite.SetupTest() // reset | ||
| tc.malleate() |
There was a problem hiding this comment.
Using the variable on range scope tc in function literal (from scopelint)
| func (suite *KeeperTestSuite) createClient(clientID string) { | ||
| suite.app.Commit() | ||
| commitID := suite.app.LastCommitID() | ||
| func (suite *KeeperTestSuite) commitNBlocks(n int) { |
There was a problem hiding this comment.
I am not familiar with the codebase yet, but this commit N blocks looks like it can be a nice thing to have maybe in the SimApp? Or the simapp helpers if we don't want to add other methods than the abci in SimApp.
I dont know if this is used a lot though.
There was a problem hiding this comment.
It would be helpful for testing purposes, yes
| {"channel not found", func() {}, false}, | ||
| {"channel not open", func() { |
There was a problem hiding this comment.
why was this test case removed?
There was a problem hiding this comment.
TimeoutOnClose doesn't check for the state of the channel
Closes: #5470
This PR adds tests for the
x/ibc/04-channel/keeper/packet.goandx/ibc/04-channel/keeper/timeout.gofiles.