Add tests to verify AbciEvent match emitted events
#363
Conversation
Codecov ReportBase: 57.68% // Head: 60.00% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
==========================================
+ Coverage 57.68% 60.00% +2.32%
==========================================
Files 124 124
Lines 15333 15650 +317
==========================================
+ Hits 8845 9391 +546
+ Misses 6488 6259 -229
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Farhad-Shabani
changed the title
AbciEvent keys match emitted eventsAbciEvent match emitted events
| let expected_values = vec![ | ||
| client_id.to_string(), | ||
| "9999-mock".to_string(), | ||
| consensus_height.to_string(), | ||
| consensus_heights | ||
| .iter() | ||
| .map(|h| h.to_string()) | ||
| .collect::<Vec<_>>() | ||
| .join(","), | ||
| String::from_utf8(hex::encode(header.clone().value)).unwrap(), | ||
| ]; |
There was a problem hiding this comment.
This test doesn't give me the confidence that we're constructing the events in the exact same way as ibc-go. We're still using our own objects for the expected_values; we want hardcoded strings here too.
The best way to write this test is to
- run ibc-go's
EmitCreateClientEvent()with a given set of arguments - print the output events to the console
- copy/paste the strings to construct
expected_values - Recreate the same arguments in (1) for
client_type,client_id, etc
Repeat for the connection/channel events tests.
Make sure to use the main branch for ibc-go; the "consensus_heights" attribute doesn't exist yet in 5.0.1, and I made the mistake of adding it to ibc-rs.
There was a problem hiding this comment.
Yes let's discard it; and we have more Default implementations like that that we should discard at the same time. Can you create a new issue? Should be low priority though
| "consensus_heights", | ||
| "header", | ||
| ]; | ||
| let expected_values = vec!["07-tendermint-0", "07-tendermint", "0-5", "0-5,0-7"]; |
There was a problem hiding this comment.
Oops, we're still missing the value for header
There was a problem hiding this comment.
oh, weird. let me fix it
There was a problem hiding this comment.
As discussed, this header's expected value is checked using MockHeaderhere. In order to verify it later against an emitted ibc-go event, a PR has been opened (#374)
| let port_id = PortId::default(); | ||
| let channel_id = ChannelId::default(); | ||
| let connection_id = ConnectionId::default(); | ||
| let counterparty_port_id = PortId::default(); |
There was a problem hiding this comment.
Let's not use default() here too; we can just use new(). I don't like the use of default() in tests as they make it harder to verify correctness quickly (I need to go check the implementation for default() for each of these), for little gain.
* Verify AbciEvent keys match emitted events * Add changelog entry * Add expected value checks * Hardcode attribute key constants * Remove pub before channel attr key consts * Hardcode expected values * Fix version const * Move away from default + Fix header issue
Closes: #163
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.