Improve UTs for ibc_module.go

[shaspitz]

Problem

Following #342 there will be better unit testing coverage of the IBC module interface implementations. However the following methods remain untested for the consumer AND provider.

• OnRecvPacket
• OnAcknowledgementPacket
• OnTimeoutPacket

Closing criteria

Unit tests for these methods. Would higher level e2e tests be useful?

[insumity]

There's no easy way to test the OnRecvPacket, OnAcknowledgementPacket, and OnTimeoutPacket methods in a clean way as is.
For example, the already tested method OnChanOpenConfirm is tested in ibc_module_test.go by checking different edge cases, e.g., "channel not found," "too many connection hops," etc. Therefore, the test of OnChanOpenConfirm is testing the underlying SetConsumerChain method from the keeper package instead, and not the logic of the OnChanOpenConfirm in the provider package.

The reason testing is difficult for the aforementioned methods is because the methods operate on the underlying providerKeeper. For example, the OnTimeoutPacket does:

if err := am.keeper.OnTimeoutPacket(ctx, packet); err != nil {
	return err
}

and to test this logic we would need to be able to mock the underlying providerKeeper (= am.keeper). This cannot be accomplished since the keeper is a struct and not an interface and hence there's no easy way to mock the providerKeeper. Although we could potentially change the providerKeeper to be an interface and hence make it easy to mock providerKeeper this would need a huge refactoring since app modules assume that keepers are structs (see here). Such a refactoring would need to extensively modify the cosmos-sdk codebase as well.

Nevertheless, we can still test but instead of testing OnRecvPacket, OnAcknowledgementPacket, and OnTimeoutPacket from the provider package, test the OnRecvPacket, OnAcknowledgementPacket, and OnTimeoutPacket methods from the keeper package (i.e., in the relay_test.go). Because OnRecvPacket is already tested in relay_test.go, we just have to test the OnAcknowledgementPacket and OnTimeoutPacket methods. This approach does not test the logic of the methods in the provider package but it is nevertheless a step forward.