Cleanup mock context for ICS3 #298
Conversation
Codecov Report
@@ Coverage Diff @@
## master #298 +/- ##
=========================================
+ Coverage 13.6% 37.0% +23.3%
=========================================
Files 69 121 +52
Lines 3752 7828 +4076
Branches 1374 2761 +1387
=========================================
+ Hits 513 2897 +2384
- Misses 2618 4625 +2007
+ Partials 621 306 -315
Continue to review full report at Codecov.
|
|
Aside from the build failing, this looks good! |
modules/src/mock_context.rs
Outdated
| pub fn with_connection( | ||
| self, | ||
| connection_id: ConnectionId, | ||
| connection_end: ConnectionEnd, | ||
| ) -> Self { | ||
| let mut connections = self.connections.clone(); | ||
| connections.insert(connection_id, connection_end); | ||
| Self { | ||
| connections, | ||
| ..self | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Wondering if we should check for sanity for the context, even if it's mock, just to make sure the tests don't initialize with a bad context. For example check that the client of a connection is present. Could also add the connection to client here.
I am also wondering if we should allow for a vector of connections, i.e. call it with_connections().
There was a problem hiding this comment.
I am also wondering if we should allow for a vector of connections, i.e. call it with_connections().
I'm slowly expanding the mocks to support more and more capabilities. So it's quite likely that we'll have such a function with_connections, which will enable more complex test cases. I can do that in the next iteration of mock refurbishing (i.e., for #296).
Wondering if we should check for sanity for the context, even if it's mock, just to make sure the tests don't initialize with a bad context. For example check that the client of a connection is present. Could also add the connection to client here.
Hmm, not sure I understand. Maybe this would be a good use case, indeed, but as I understand the mock context, its purpose is to support testing of handlers (and not more than that at the moment). So I would not add too much logic, validation, or any kind of support, unless it is to exercise more paths in the codebase.
There was a problem hiding this comment.
Hmm, not sure I understand. Maybe this would be a good use case, indeed, but as I understand the mock context, its purpose is to support testing of handlers (and not more than that at the moment). So I would not add too much logic, validation, or any kind of support, unless it is to exercise more paths in the codebase.
Exactly, but in tests we always want to start with a correct context. This becomes harder to setup correctly with a more complex initial state.
There was a problem hiding this comment.
I'm slowly expanding the mocks to support more and more capabilities. So it's quite likely that we'll have such a function with_connections, ....
This is would be the same as with_connection, just pass a slice and do a loop.
There was a problem hiding this comment.
Now I understand! Will track these two suggestions in the other iteration on this work (#297 (comment)).
| connections: Default::default(), | ||
| clients: Default::default(), | ||
| client_connections: Default::default(), |
There was a problem hiding this comment.
| connections: Default::default(), | |
| clients: Default::default(), | |
| client_connections: Default::default(), | |
| clients: Default::default(), | |
| client_connections: Default::default(), | |
| connections: Default::default(), |
Closes: #295
Description
For contributor use:
docs/) and code commentsFiles changedin the Github PR explorer