test: introduce tests for OnTimeoutPacket and OnAcknowledgementPacket

[insumity]

Description

Closes: #362

As described in this comment we cannot test those methods in ibc_module_test.go so we test in relay_test.go instead.

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• Included the correct type prefix in the PR title
• Targeted the correct branch (see PR Targeting)
• Provided a link to the relevant issue or specification
• Reviewed "Files changed" and left comments if necessary
• Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• Confirmed the correct type prefix in the PR title
• Confirmed all author checklist items have been addressed
• Confirmed that this PR does not change production code

[shaspitz]

I like the granularity of the individual tests, these are easy to follow 👍

I've left some comments regarding intention about what we're actually testing.

In the past there's been some tests in this repo that make it seem like a method is functioning as expected, but in fact the method had a bug that was hidden by vaguely intentioned and overly generic tests (example: just checking that a method doesn't return an error, when there's more complexity to be tested under the hood). Imo it's better to have no test coverage over vaguely intentioned test coverage.

This is where I'm coming from on these seemingly nitpicky comments :)

[shaspitz]

Note that when closeChan is false, we're not mocking anything for StopConsumerChain, as the name (SetupForStoppingConsumerChain) suggests. This func becomes a more generic setup func which mocks and calls CreateConsumerClient and SetConsumerChain.

Maybe we refactor to split things out and/or make naming more accurate

[insumity]

Good catch! I rewrote the godoc of this function to clearly state this.

Maybe we refactor to split things out and/or make naming more accurate

I agree. It would be nice to have 2 functions here: i) SetupForStoppingConsumerChain that just sets up the consumer chain for stopping, ii) TestChannelIsClosed that asserts that the underlying channel is closed when we call StopConsumerChain with closeChan being true. Then the idea would be that in a test we do something like the following:

SetupForStoppingConsumerChain(...)
StopConsumerChain(...) // e.g., we call to stop the consumer chain through `OnTimeoutPacket`
TestChannelIsClosed(...)
TestProviderStateIsCleanedAfterConsumerChainIsStopped(...) // tests that the provider's chain is closed and hence is used as a proxy to verify that the chain was closed 

where TestChannelIsClosed would need to set the expectations from GetMocksForStopConsumerChain, so something like:

func TestChannelIsClosed(...) {
    ....
    mocks.MockChannelKeeper.EXPECT().ChanCloseInit(...).Times(1)
}

The issue with the above approach is that expectations are set after the StopConsumerChain call and hence when StopConsumerChain is called we would fail with something akin to Unexpected call to ChanCloseInit([1]).
To solve the issue, this PR introduces the SetupForStoppingConsumerChainChannel function and does

SetupForStoppingConsumerChain(...)
SetupForStoppingConsumerChainChannel(...)
StopConsumerChain(...)
TestProviderStateIsCleanedAfterConsumerChainIsStopped(...)

The above approach might not be that beautiful since we do testing before (i.e., test that the channel would be closed) and after (i.e., that the producer's chain is cleaned) the StopConsumerChain. Nevertheless, this approachs feels the nicest way to circumvent the mock expectations issue I mentioned above.

[p-offtermatt]

Thanks for the work!
I agree with Shawns comments, it's hard to tell whether the tests actually test meaningful behaviour. I'd recommend asserting stronger properties in the tests.

It may or may not be helpful to go back to the spec and see what the spec claims about the functionality here, e.g. see https://github.com/cosmos/ibc/blob/jk/ccvTLA/spec/app/ics-028-cross-chain-validation/methods.md#consumer-chain-removal

[insumity]

Because this is used in relay_test.go as well now, the function is moved to testutil/keeper/unit_test_helpers.go.

[shaspitz]

LGTM thanks!

[p-offtermatt]

LGTM, thanks for the work!