feat: implement AllPools query in x/poolmanager #4659
Conversation
github-actions
bot
added
C:app-wiring
| // and overlapping and duplicate pool ids. The expected results and potential errors are defined for each test case. | ||
| // The test suite sets up mock pool modules and configures their behavior for the GetPools method, injecting them into the pool manager for testing. | ||
| // The actual results of the AllPools function are then compared to the expected results, ensuring the function behaves as intended in each scenario. | ||
| func (suite *KeeperTestSuite) TestAllPools() { |
There was a problem hiding this comment.
What is the reason we test with mock here? On the first look, it seems like something that could be tested by real pools (Also personally would prefer real pools testing if compared with mock testings, although I agree with using mock testings if unevitable or testing setup gets too hard)
There was a problem hiding this comment.
The mocks are used for the ease of setting up the test case in such a way as to verify all the edge cases.
Mocks are useful for testing code that has side effects. By using a mock object to simulate the side effect of returning pools with IDs in a desired sequence, I can test the code's behavior without actually setting up the pools in such an order.
What we want to test is that no matter how many pool modules we have, and the spread of the pool ids across them, we get pools sorted by IDs in the end.
The pool type used is irrelevant for testing the correctness of this method.
Independent of this test, I think that using mock is an elegant approach. It allows for easily maximizing test coverage by mocking out all of the dependencies within it to return many if not all combinations of the output. These combinations can test out all the edge cases.
The reason we cannot use it more across SDK is that the dependency inversion principle is commonly violated. What I mean is interfaces are rarely used and, instead, concrete implementations are relied upon, complicating the use of mocks. Without having interfaces, we cannot create mocks and have to use concrete implementations.
CC: @czarcas7ic
There was a problem hiding this comment.
The pool type used is irrelevant for testing the correctness of this method.
I guess this was one of the main reasons I was requesting for a test that does not use mock. I do agree with you in the sense that mockings have great advantage when it comes to maximizing test coverage of this method. Meanwhile, I think it is also important for us to have a test that tests this logic addition via the app wiring that we use instead of mocked values. Let me know if you have any opinions on this!
There was a problem hiding this comment.
The only reason I don't have a strong opinion on this is that its just a query, so the risk is negligible. So I am fine with mocks (and Roman explained offline why we don't need to test various pool types)
There was a problem hiding this comment.
I added the requested test: 4b185f9 to unblock this PR for FE.
However, I would like to continue discussing the mocks so that we come to a common agreement to avoid getting blocked on discussions when mocks are used in the future.
I guess this was one of the main reasons I was requesting for a test that does not use mock. I do agree with you in the sense that mockings have great advantage when it comes to maximizing test coverage of this method. Meanwhile, I think it is also important for us to have a test that tests this logic addition via the app wiring that we use instead of mocked values. Let me know if you have any opinions on this!
What are you referring to by this? I have no problem with adding tests but I have trouble seeing the value being justified.
The mocks allow for achieving maximum test coverage of the system under test. Each pool's underlying implementation should be tested individually by a direct test. Testing the internals of each pool implementation at the poolmanager (higher level of abstraction) only introduces additional complexity with negligible benefit.
Co-authored-by: Matt, Park <[email protected]>
This reverts commit 40986dd.
|
Closing and reopening to trigger CI |
|
Merging this change since the requested test was added. @mattverse @czarcas7ic I'm going to follow up offline on the mocks discussion |
Closes: #XXX
What is the purpose of the change
This PR introduces the
AllPoolsfunction inx/poolmanagerand a corresponding test suite to ensure its proper functionality. TheAllPoolsfunction retrieves a sorted list of all pools from different pool modules in the Osmosis chain.Key changes:
AllPoolsfunction, which returns a sorted list of all pools from different pool modules, sorted by their ids.LessFunctype for generic less functions and aMergeSlicesutility function to efficiently merge two sorted slices into a single sorted slice.PoolModuleIinterface and inject them into the pool manager for testing purposes.By adding the
AllPoolsfunction and a corresponding test suite, we ensure the correct and efficient retrieval of pools from different pool modules in the Osmosis chain. This will allow FE to integrate the concentrated liquidity feature, as well as provide developers and users with the ability to query all available pools in a single, unified manner.Testing and Verifying
This change added tests.
Documentation and Release Note
Unreleasedsection inCHANGELOG.md? yes