refactor: use expResult in acknowledgePacket tests (#4439)
#4472
Conversation
* refactor: adapt acknowledgePacket tests to use expResult func * cleanup: removing redundant stale tests * fix: assert ErrInvalidProof for verification failure. modify suite.Run tc naming
| expResult: func(commitment []byte, err error) { | ||
| suite.Require().Error(err) | ||
| suite.Require().ErrorIs(err, types.ErrChannelNotFound) | ||
| suite.Require().Nil(commitment) |
There was a problem hiding this comment.
Looks like this test will still need some more love. commitment should be NotNil for this test case (but it is never set)
There was a problem hiding this comment.
Glad to see you're on your toes this morning! 😛
I was hoping you'd catch this - so, yes, the commitment should not be nil. However, above in malleate() we modify srcPort and srchChannel such that they are invalid IDs and commitment lookup in the test body uses packet.GetSourcePort and packet.GetSourceChannel.
I can look at refactoring the tests some more but I didn't want to change too much logic in this PR.
There was a problem hiding this comment.
Added more love in 1c5e64b
| suite.Require().NoError(err) | ||
| suite.Require().Nil(commitment) | ||
|
|
||
| // create packet commitment | ||
| sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData) | ||
| suite.Require().NoError(err) | ||
| nextSequenceAck, found := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetNextSequenceAck(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel()) | ||
| suite.Require().True(found) | ||
| suite.Require().Equal(uint64(1), nextSequenceAck, "sequence incremented for UNORDERED channel") |
There was a problem hiding this comment.
feels like we could create some helper expResult functions:
assertSuccess := func(commitment []byte, err error) {
suite.Require().NoError(err)
suite.Require().Nil(commitment)
nextSequenceAck, found := suite.chainA.App.GetIBCKeeper().ChannelKeeper.GetNextSequenceAck(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel())
suite.Require().True(found)
suite.Require().Equal(uint64(1), nextSequenceAck, "sequence incremented for UNORDERED channel")
}
assertNoOp := func(commitment []byte, err error) {
suite.Require().Error(err)
suite.Require().ErrorIs(err, types.ErrNoOpMsg)
suite.Require().Nil(commitment)
}
errorIs := func(expError error) func(commitment []byte, err error) {
return func(commitment []byte, err error) {
suite.Require().Error(err)
suite.Require().ErrorIs(err, types.ErrNoOpMsg)
suite.Require().NotNil(commitment)
}
}
// define test cases
expResult: assertSuccess
...
expResult: assertNoOp
...
expResult: errorIs(channeltypes.ErrChannelNotFound)maybe in another followup if we like
There was a problem hiding this comment.
Nice suggestion! I'm on board, but maybe best in another follow up
|
Seems to be odd runner activity at the moment causing random failures.
Once CI is passing, we can get this merged 👍 edit: spamming rerun got me there in the end |
Description
Cherry-pick 9c54f23 to main, minus the channel-upgradability diffs
closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.