-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleaner context mocks (testing infrastructure for modules) #111
Comments
12 tasks
8 tasks
adizere
changed the title
Modular testing infrastructure for modules (re: context mocks)
Cleaner context mocks (testing infrastructure for modules)
Oct 8, 2020
This was referenced Oct 12, 2020
6 tasks
hu55a1n1
pushed a commit
that referenced
this issue
Sep 29, 2022
* (#91) WIP: Added validation for ICS7 ClientState::new(). * Bit more progress on validation for ICS7 ClientState::new() * Initialization for dummy header done. Tests done. * Added the JSON representation of a dummy signed header.
livelybug
pushed a commit
to octopus-network/ibc-rs
that referenced
this issue
Oct 14, 2022
* (cosmos#91) WIP: Added validation for ICS7 ClientState::new(). * Bit more progress on validation for ICS7 ClientState::new() * Initialization for dummy header done. Tests done. * Added the JSON representation of a dummy signed header.
shuoer86
pushed a commit
to shuoer86/ibc-rs
that referenced
this issue
Nov 4, 2023
* add go tests * add js tests * fix how license is shown * add rust flow * fix syntax * cd into rust * test * test++ * test++ * test++ * migrate * add rust linting * fix concurrency * formatting * lint pr * rename main to master * fix test * fix clippy issues Co-authored-by: Marko Baricevic <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
This issue is for discussing the problem of making mock context more modular & composable.
Problem
Currently, each ICS has its own context mock, designed specifically for unit testing the
dispatch
andhandler
s of that ICS. For instance, unit tests in ICS3 employ aMockConnectionContext
comprising both a chain context and a client context.https://github.com/informalsystems/ibc-rs/blob/2438ede7acac0c9481bebd77a13ca284d8b134c5/modules/src/ics03_connection/context_mock.rs#L15-L20
Similarly to
MockConnectionContext
, the mock context for ICS 26 also comprises a client context:https://github.com/informalsystems/ibc-rs/blob/53d21475b14968c1e529e13da29deada5d84175c/modules/src/ics26_routing/context_mock.rs#L23-L26
The problem is as follows: neither of the client context members of
MockConnectionContext
andMockICS26Context
is a reference, hence the test vectors for ICS26 will involve essentially two copies of a client context (one copy in theMockConnectionContext
and another copy inMockICS26Context
). In other words, the problem is that the mock contexts structures are not composable, and hence involve a lot ofclone
operations leading to redundancy at the level of these structures. The straightforward solution to this is to make the client context members references, which would involve introducing lifetimes.It's not clear if lifetimes are desirable or if they are the best solution.
Later Edit: @brapse proposed a solution that eliminates altogether dependencies across contexts. PRO: flat dependency space, no need for lifetimes or excessive clone(); CON: duplicate code.
LE2: @ancazamfir proposed a solution to have a single data structure that implements all the handler's reader & keeper traits. We can use this data structure to test any IBC handler.
Solution
We'll introduce a global mock context, which will serve in unit testing all and every IBC module. Towards making PRs smaller and more manageable, we break this work down by ICS:
Futher issues
dispatch
. More generally: align testing across the handlers of ICS02 and ICS03 (and others); tentative strategy: testprocess
as well askeep
separately, then testdispatch
by comparing its output to the ones ofprocess
andkeep
(deferred here from Follow-up on message processor (handler) implementations #115). This was fixed in Cleaner context mocks for ICS2 informalsystems/hermes#296Smaller aesthetic issues that should also be fixed with this work:
Mostnot applicable, since all mock context were reunited in a single global structureMockXContext
structures (for exampleMockConnectionContext
) can be renamed toMockICSXContext
(e.g., for connections we'd haveMockICS3Context
)MockContext
Consider renaming context traits (not an obvious improvement, deferred to later..ConnectionReader
,ClientKeeeper
) to also refer to explicit ICS rather than objects, so for instanceConnectionReader
would be renamed toICS3Reader
Movethese traits/context were deletedChainReader
andChainKeeper
andMockChainContext
into ICS24 (host requirements).Consider renamingthese traits were deletedChainReader
toICS24Reader
(same for keeper and context)The dependencies among contexts is as follows:
So we can refactor the contexts to follow this hierarchy: ICS18 -> ICS24 -> ICS26 -> ICS4, 3, 2
For Admin Use
The text was updated successfully, but these errors were encountered: