ibc-rs icon indicating copy to clipboard operation
ibc-rs copied to clipboard

Refactor unit tests

Open plafer opened this issue 2 years ago • 3 comments

Our unit tests make extensive use of default values, including in "util" functions implicitly used, which creates hard to understand relationships, and ultimately makes it difficult to understand why a test succeeds or fails. For example, some validation checks probably work because we used the default value for our ClientId, which happens to be also used when, say, creating the "dummy" packet.

I've also seen tests such as this one which claims a reason for the test to fail (e.g. the client doesn't have a consensus state at the required height), but no client is even installed in that MockContext.

plafer avatar Feb 14 '23 18:02 plafer

After we refactor our errors with #269, each test should confirm that the right error was emitted as opposed to using Result::is_err().

plafer avatar Feb 14 '23 18:02 plafer

One possible solution regarding defaults: Found it effective and can result in a cleaner, easier-to-follow code, not just for default values, even for any new object construction within our tests, is to implement and use more helper methods. This can exist for each type in its own module under the mod test_util. Then simply be called everywhere needed. Similar to what we did here for MsgConnectionOpenInit, a new_valid_dummy method or something like that can be defined for e.g CleintId, Height, etc

Farhad-Shabani avatar May 12 '23 16:05 Farhad-Shabani

I think #1074 was a duplicate of this. If that is the case, we can close this after fixing this title.

rnbguy avatar Apr 08 '24 14:04 rnbguy