Events unit testing across the board

[Anmol1696]

Summary

Currently there is no tests for events emitted by any of the msgs. The following proposal is a way to have easy testablity in unit tests for events emitted per msg.

Problem Definition

Why do we need this feature?

• Events are slowly becoming source of truth for many downstream applications, we need a way to make sure there are no regressions happening on events
• Events produced is untested code, we will also increase the code coverage

What benefits does the ibc-go stand to gain by including this feature?

• Programatic way to catch regressions on events faster

Are there any disadvantages of including this feature?

• More overhead for developers as testing events has more boiler plate
• Benifits might be minimal compared to the efforts

Proposal

The proposal is to add a testutil function which can assert events.

package testutil

assertEvents(suite suite.Suite, expected map[string]map[string]string, actual sdk.Events)

To make this function importable, I propose we create a new package testutil at the root level, where we can have multiple test utils that can then be imported. Something like testutil. package in cosmos-sdk

In the test functions where the events are emmited we can call the above function as:

// Create expected event
expEvents := map[string]map[string]string{
  "ibc_transfer": {
	  "sender":   suite.chainA.SenderAccount.GetAddress().String(),
	  "receiver": suite.chainB.SenderAccount.GetAddress().String(),
	  "amount":   coin.Amount.String(),
	  "denom":    coin.Denom,
  },
  "send": {
    ...
  }
}

// Actual event
events := ctx.EventManager().Events()

// Perform the assert
assertEvents(suite, expEvents, events)

Points of disucssion

• As per the current proposal we hardcode the event types and attribute keys per test, so as to make the test case itself brittle, if the default strings changes the test should fail as well. But this is a point of dicussion, open to changing it to less hard coded.
• For this proposal we need to make assertEvents function an importable function which can be used by various functions. Creating a new package might be an overkill, but can go a long way in the future to have common test utils.

PRs

• assert function and app/transfer - imp: event assertion tests function #2829
• 04-channel - Event tests assertion for 04-channel #2830
• 03-connection Add event testing for connection handshake #5682
• 02-client Add testing for client events #5686
• apps/27-ica Add testing for ICA events #5687
• apps/29-fee Add tests for fee events #5706

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[colin-axner]

Thanks @Anmol1696. I like the addition 👍 We can add the function into the ibc testing package. @damiannolan @chatton what do y'all think?

As per the current proposal we hardcode the event types and attribute keys per test, so as to make the test case itself brittle, if the default strings changes the test should fail as well.

This makes sense. Given that changing the key value would likely be very disruptive to external users and thus should rarely change

[Anmol1696]

Sorry for the hurry, but can this PR: #2829 be reviewd? So i can then open all the small PRs for various parts of the code. This PR has the main function needed, after which we can have parallel PRs, they can be reviewd slowly.

From what i can tell there will be these PRs

• 03-connection
• 02-client
• core (keeper and msg server)
• apps/27-ica
• apps/29-fee

[colin-axner]

We had a regression in event behaviour (v5.2, v6.1, v7+) for no-op relays which affected hermes which shows that increased event testing would have been useful in being aware of the issue before it reaches production code

Would be nice if we could split up this issue and prioritize some of its work over the months cc @crodriguezvega