Issues/1566 simplify event testing 2

[mootz12]

What

Improves testing and validation for events while remaining backwards compatible with existing event tests.

Adds a matches function to contractevent's Event trait to compare against the event tuple, and adds a contains function to testutils Events implementation to search published events for a matching event.

Why

This serves as an alternate, less invasive solution to #1566, as opposed to #1633.

Known limitations

matches does not attempt to validate events across different env objects

[dmkozh]

I would generally recommend against using non-exhaustive assertions where possible. Using contains both misses the unintentionally published events, and also misses the event order for the cases when there is more than one event present. I can see this being sometimes a more concise option for testing complex multi-contract interactions, but generally I think we still need better tools for validating the whole event array (i.e. another PR in some shape), and promote that as the 'best practice' for the event testing.

[tupui]

Yes but then we really need better tooling for that. For instance if you do some calls to transfer within your contract, you will get the events from these transfers. You don't necessarily care about these and since you don't have the event trait itself and are not the one raising the event. You need to hunt it down or play around until you get the structure right in your tests. It's proven to be quite tedious to test on my side.

[dmkozh]

I think a contract_id filter makes sense for when you only care about 1 contract, would that make sense for your case?

BTW, in case of transfers, the spec actually exists in the SDK. I understand that not every test needs this, but OTOH I can see how at least some tests would want this. Transfers are actually pretty important side effects, and it would make sense for at least a fraction of tests to make sure that the expected amounts have actually been transferred. So even if there is a way to narrow down the event assertion scope, I would still argue that we should also have a simple way to cover the common token events.

[tupui]

Transfers are actually pretty important side effects, and it would make sense for at least a fraction of tests to make sure that the expected amounts have actually been transferred.

Totally agree, but I prefer to check balances vs rely on events, as this is the actual thing that matters.

[tupui]

Regardless, if this is the way, we should then have easier way to pull in events which are not ours. Transfer as you said is ok to find, but for cross contracts I then have to go on GitHub or else to try to find the specs.

[mootz12]

This was primarily added out of personal annoyance, trying to improve testing like is done here: https://github.com/blend-capital/blend-contracts-v2/blob/main/test-suites/tests/test_pool.rs#L83-L108

Transfers are verified via balance checks, which IMO is the way things like this should be tested. IMO I don't think testing the event logic of other contracts is necessary. In this case, I only care that the correct event I intended to emit during this call was emitted.

If we want to avoid recommending this, I can remove the contains fn. The primary lift of this change is the matches function, and you could just do env.events().all().any(... yourself. No strong opinions either way, as long as I can directly compare contractevent to the tuple event type.

[leighmcculloch]

One thing to consider is that these style of assertions are difficult to inspect when things aren't working in comparison to using assert_eq! where the two values will be displayed in test output.

[tupui]

Could we do some test first on the trait type maybe? This way we could say if the event trait is event present in the reported events or not.

[mootz12]

This is a good point, good catch.

We could add an assertion macro that helps with this assert_event_eq! that essentially calls event.match and prints debug info for both events if false.