feat: [indexer] add test cases to pair_publisher::PublishPoolPairs #8746
Conversation
cryptomatictrader
added
A:no-changelog
A:backport/v26.x
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
ingest/indexer/domain/mocks/pair_publisher_mock.go (2)
13-21: LGTM: Comprehensive MockPairPublisher struct definition.The
MockPairPublisherstruct is well-designed with fields that allow for flexible testing scenarios, including error simulation, call tracking, and result counting. The field names are clear and self-explanatory.For consistency, consider renaming
PublishPoolPairsCalledtoPublishPoolPairsWasCalledto match the past tense used in other field names likeCalledWithPools.
23-34: LGTM: PublishPoolPairs method implementation is correct and comprehensive.The
PublishPoolPairsmethod correctly updates all relevant fields of the mock and properly handles thecreatedPoolIDsmap to count pools with creation data. The implementation aligns well with the expected behavior described in the PR objectives.For improved robustness, consider adding a nil check for the
createdPoolIDsmap. This would prevent potential panic if the map is nil:func (m *MockPairPublisher) PublishPoolPairs(ctx sdk.Context, pools []poolmanagertypes.PoolI, createdPoolIDs map[uint64]commondomain.PoolCreation) error { m.PublishPoolPairsCalled = true m.CalledWithPools = pools m.CalledWithCreatedPoolIDs = createdPoolIDs m.NumPoolPairPublished += len(pools) for _, pool := range pools { + if createdPoolIDs == nil { + continue + } if _, ok := createdPoolIDs[pool.GetId()]; ok { m.NumPoolPairWithCreationData++ } } return m.PublishPoolPairsError }ingest/indexer/service/blockprocessor/export_test.go (3)
10-23: LGTM: BlockUpdatesIndexerBlockProcessStrategy is well-structured.The implementation follows good practices:
- Constructor initializes all fields.
- Type alias allows external access to the unexported type.
PublishCreatedPoolsmethod encapsulates the internal implementation.Consider adding a brief comment describing the purpose of
BlockUpdatesIndexerBlockProcessStrategyfor better documentation.
25-42: LGTM: FullIndexerBlockProcessStrategy is well-implemented.The implementation follows good practices:
- Constructor initializes all fields.
- Type alias allows external access to the unexported type.
- Methods encapsulate the internal implementations.
Consider adding a brief comment describing the purpose of
FullIndexerBlockProcessStrategyfor better documentation.
1-42: Overall, the file is well-structured and serves its purpose effectively.The implementation follows good Go practices and conventions:
- Proper encapsulation of internal details
- Use of type aliases for external access to unexported types
- Consistent naming and structure
For consistency, consider adding error handling in the
PublishAllSuppliesmethod similar toPublishCreatedPoolsandProcessPools. If error handling is not required, it might be worth adding a comment explaining why.ingest/indexer/domain/mocks/publisher_mock.go (2)
31-35: LGTM! Enhanced tracking for pair publications.The changes to the
PublishPairmethod improve the mock's ability to track different types of pair publications. This aligns well with the PR objectives, particularly for testing scenarios with pool creations.A minor suggestion for improved readability:
Consider extracting the creation time check into a separate method for clarity:
func (p *PublisherMock) PublishPair(ctx context.Context, pair indexerdomain.Pair) error { p.CalledWithPair = pair p.NumPublishPairCalls++ - if !pair.PairCreatedAt.IsZero() { + if p.isPairCreation(pair) { p.NumPublishPairCallsWithCreation++ } return p.ForcePairError } + +func (p *PublisherMock) isPairCreation(pair indexerdomain.Pair) bool { + return !pair.PairCreatedAt.IsZero() +}This change would make the main method more readable and the creation check more explicit.
49-49: LGTM! Consistent updates to remaining publish methods.The additions of counters to
PublishTokenSupply,PublishTokenSupplyOffset, andPublishTransactionmethods are consistent with the updates to other methods in this mock. These changes will allow for accurate tracking of various publish calls in tests, which is valuable for comprehensive test coverage.For consistency and potential future extensibility:
Consider updating these methods to match the structure of
PublishPair. This could involve adding checks for specific conditions that might be relevant in the future:func (p *PublisherMock) PublishTokenSupply(ctx context.Context, tokenSupply indexerdomain.TokenSupply) error { p.CalledWithTokenSupply = tokenSupply p.NumPublishTokenSupplyCalls++ + // Add any specific condition checks here if needed in the future return p.ForceTokenSupplyError }Apply similar changes to
PublishTokenSupplyOffsetandPublishTransactionmethods. This structure would make it easier to add more detailed tracking in the future if needed.Also applies to: 56-56, 63-63
ingest/indexer/service/blockprocessor/pair_publisher_test.go (3)
15-31: LGTM: Well-structured test suite.The test suite is properly set up using the testify/suite pattern. Good job on embedding
ConcentratedKeeperTestHelperfor the necessary test setup.Consider adding a brief comment explaining the purpose of
ConcentratedKeeperTestHelperfor better clarity:type PairPublisherTestSuite struct { // ConcentratedKeeperTestHelper provides setup and utilities for testing concentrated liquidity pools apptesting.ConcentratedKeeperTestHelper }
33-81: LGTM: Well-structured test cases.The test function and test cases are well-defined, covering the scenarios mentioned in the PR objectives. Good job on providing clear descriptions for each test case.
To improve readability, consider using constants for the expected call counts:
const ( baseExpectedCalls = 12 twoDenomCreationCalls = 2 fourDenomCreationCalls = 6 ) // Then in the test cases: expectedNumPublishPairCalls: baseExpectedCalls, expectedNumPublishPairCallsWithCreation: twoDenomCreationCalls,This change would make it easier to understand and maintain the expected values.
83-115: LGTM: Well-implemented test execution.The test execution loop is well-structured and follows best practices for table-driven tests. Good job on setting up the test environment and using a mock publisher to track call counts.
Consider adding error checks after calling
GetPoolsmethods:concentratedPools, err := s.App.ConcentratedLiquidityKeeper.GetPools(s.Ctx) s.Require().NoError(err) s.Require().NotEmpty(concentratedPools, "Expected non-empty concentrated pools") cfmmPools, err := s.App.GAMMKeeper.GetPools(s.Ctx) s.Require().NoError(err) s.Require().NotEmpty(cfmmPools, "Expected non-empty CFMM pools") cosmWasmPools, err := s.App.CosmwasmPoolKeeper.GetPoolsWithWasmKeeper(s.Ctx) s.Require().NoError(err) s.Require().NotEmpty(cosmWasmPools, "Expected non-empty CosmWasm pools")This addition would ensure that the pools are not only retrieved without error but also contain the expected data.
ingest/indexer/service/blockprocessor/block_updates_indexer_block_process_strategy_test.go (2)
17-32: LGTM: Well-structured test suite with clear documentation.The test suite is well-structured using the
testify/suitepackage and embedsConcentratedKeeperTestHelperfor setup. The comment provides clear information about the test scenarios and setup.Consider adding a brief explanation of what
ConcentratedKeeperTestHelperprovides to make the setup process more transparent to readers unfamiliar with the project structure.
34-97: LGTM: Well-structured table-driven tests with comprehensive scenarios.The
TestPublishCreatedPoolsfunction uses a table-driven test approach, which is a good practice for testing multiple scenarios. The test cases cover a range of scenarios, including edge cases, and are structured clearly.Consider adding a constant for the expected number of pools (5) used in multiple test cases to improve maintainability. For example:
const expectedTotalPools = 5This would make it easier to update if the number of pools changes in the future.
ingest/indexer/service/blockprocessor/full_indexer_block_process_strategy_test.go (2)
79-81: Typo: Correct 'poolsExtracter' to 'poolsExtractor'The variable name
poolsExtracteris misspelled. It should bepoolsExtractorto match standard spelling and improve code readability.Apply this diff to fix the typo:
-poolsExtracter := &commonmocks.PoolsExtractorMock{ +poolsExtractor := &commonmocks.PoolsExtractorMock{ BlockPools: blockPools, }Ensure all references to
poolsExtracterin this context are updated accordingly.
198-201: Typo: Correct 'poolsExtracter' to 'poolsExtractor'The variable
poolsExtracteris misspelled here as well. Correcting it topoolsExtractorimproves consistency and readability.Apply this diff:
-poolsExtracter := &commonmocks.PoolsExtractorMock{ +poolsExtractor := &commonmocks.PoolsExtractorMock{ BlockPools: blockPools, CreatedPoolIDs: test.createdPoolIDs, }Be sure to update all occurrences within this function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- ingest/indexer/domain/mocks/pair_publisher_mock.go (1 hunks)
- ingest/indexer/domain/mocks/publisher_mock.go (1 hunks)
- ingest/indexer/service/blockprocessor/block_updates_indexer_block_process_strategy_test.go (1 hunks)
- ingest/indexer/service/blockprocessor/export_test.go (1 hunks)
- ingest/indexer/service/blockprocessor/full_indexer_block_process_strategy_test.go (1 hunks)
- ingest/indexer/service/blockprocessor/pair_publisher.go (0 hunks)
- ingest/indexer/service/blockprocessor/pair_publisher_test.go (1 hunks)
💤 Files with no reviewable changes (1)
- ingest/indexer/service/blockprocessor/pair_publisher.go
🔇 Additional comments (9)
ingest/indexer/domain/mocks/pair_publisher_mock.go (2)
1-9: LGTM: Package declaration and imports are appropriate.The package name "mocks" is suitable for a mock implementation, and the imports are relevant to the mock's functionality. There are no unused imports.
11-11: Excellent: Interface implementation check is in place.The line
var _ indexerdomain.PairPublisher = &MockPairPublisher{}ensures at compile-time thatMockPairPublisherimplements thePairPublisherinterface. This is a crucial practice for maintaining type safety and catching implementation errors early.ingest/indexer/service/blockprocessor/export_test.go (1)
1-8: LGTM: Package declaration and imports are appropriate.The package name and imports are relevant to the file's purpose. There are no unused imports, which is good practice.
ingest/indexer/domain/mocks/publisher_mock.go (3)
16-21: LGTM! Improved mock for better test coverage.The addition of these counters to the
PublisherMockstruct is a great improvement. It will allow for more precise tracking of method calls in tests, which aligns well with the PR objective of enhancing test coverage for thePublishPoolPairsfunctionality. The naming convention for these counters is consistent and clear, making the code easy to understand and maintain.
41-42: LGTM! Consistent update to PublishBlock method.The addition of the
NumPublishBlockCallscounter in thePublishBlockmethod is consistent with the updates to other methods in this mock. This change will allow for accurate tracking of block publications in tests, which is valuable for comprehensive test coverage.
Line range hint
1-68: Overall assessment: Well-implemented enhancements to the PublisherMockThe changes made to this file consistently improve the
PublisherMockstruct by adding counters to track various method calls. These enhancements align well with the PR objectives, particularly in improving the ability to test thePublishPoolPairsfunctionality.Key points:
- The addition of counter fields to the struct provides a clear way to track different types of publish calls.
- The implementation of these counters in each method is consistent and correct.
- The changes, especially in the
PublishPairmethod, allow for more detailed testing scenarios, including differentiating between regular pair publications and those with creation details.The suggestions provided in the review are minor and aimed at improving readability and consistency. Overall, these changes will significantly enhance the mock's usefulness in testing scenarios.
ingest/indexer/service/blockprocessor/pair_publisher_test.go (1)
1-13: LGTM: Package declaration and imports are appropriate.The package name and imports are well-structured and relevant for the test suite. Good job on following Go conventions for test files.
ingest/indexer/service/blockprocessor/block_updates_indexer_block_process_strategy_test.go (2)
1-15: LGTM: Package declaration and imports are appropriate.The package name
blockprocessor_testfollows Go conventions for test files. The imports include necessary testing libraries and project-specific packages, which are appropriate for the test file's purpose.
1-155: Overall, excellent test coverage and structure.This test file demonstrates high-quality test practices:
- Well-structured test suite using
testify/suite.- Comprehensive coverage of various scenarios using table-driven tests.
- Thorough setup and mocking of dependencies.
- Clear and descriptive test cases.
The suggested improvements are minor and aimed at enhancing maintainability and robustness. Great job on creating a solid test suite for the block update indexer strategy!
| // PairPublisherTestSuite tests the pair publisher. | ||
| // The pair publisher is responsible for iterating over all pools and their denoms combinations, | ||
| // with optional creation details provided in the parameter, it also append creation details to the pair struct. | ||
| // Since the test suite initializes all supported pools (concentrated, cfmm, cosmwasm) via s.App.PrepareAllSupportedPools(), | ||
| // the created pool IDs are 1-5, and the number of expected calls to the publisher is 12 coz: | ||
| // pool id 1 has 2 denoms, pool id 2 has 4 denoms, pool id 3 has 3 denoms, pool id 4 has 2 denoms, pool id 5 has 2 denoms. | ||
| // Scenarios tested: | ||
| // - Happy path without pool creation | ||
| // - publish pool pairs with two-denom pool creations | ||
| // - publish pool pairs with four-denom pool creations | ||
| func TestPairPublisherTestSuite(t *testing.T) { | ||
| suite.Run(t, new(PairPublisherTestSuite)) | ||
| } | ||
|
|
||
| func (s *PairPublisherTestSuite) TestPublishPoolPairs() { | ||
| tests := []struct { | ||
| name string | ||
| // map of pool id to pool creation | ||
| createdPoolIDs map[uint64]commondomain.PoolCreation | ||
| // expected number of calls to the PublishPoolPair method | ||
| expectedNumPublishPairCalls int | ||
| // expected number of calls to the PublishPoolPair method with pool creation details | ||
| expectedNumPublishPairCallsWithCreation int | ||
| }{ | ||
| { | ||
| name: "happy path without pool creation", | ||
| createdPoolIDs: map[uint64]commondomain.PoolCreation{}, | ||
| expectedNumPublishPairCalls: 12, | ||
| expectedNumPublishPairCallsWithCreation: 0, | ||
| }, | ||
| { | ||
| name: "publish pool pairs with multiple two-denom pool creations", | ||
| createdPoolIDs: map[uint64]commondomain.PoolCreation{ | ||
| 1: { | ||
| PoolId: 1, | ||
| BlockHeight: 12345, | ||
| BlockTime: time.Now(), | ||
| TxnHash: "txhash1", | ||
| }, | ||
| 4: { | ||
| PoolId: 4, | ||
| BlockHeight: 12346, | ||
| BlockTime: time.Now(), | ||
| TxnHash: "txhash2", | ||
| }, | ||
| }, | ||
| expectedNumPublishPairCalls: 12, | ||
| expectedNumPublishPairCallsWithCreation: 2, | ||
| }, | ||
| { | ||
| name: "publish pool pairs with four-denom pool creations", | ||
| createdPoolIDs: map[uint64]commondomain.PoolCreation{ | ||
| 2: { | ||
| PoolId: 2, | ||
| BlockHeight: 12345, | ||
| BlockTime: time.Now(), | ||
| TxnHash: "txhash1", | ||
| }, | ||
| }, | ||
| expectedNumPublishPairCalls: 12, | ||
| expectedNumPublishPairCallsWithCreation: 6, | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| s.Run(test.name, func() { | ||
| s.Setup() | ||
|
|
||
| // Initialized chain pools | ||
| s.PrepareAllSupportedPools() | ||
|
|
||
| // Get all chain pools from state for asserting later | ||
| // pool id 1 (2 denoms) created below | ||
| concentratedPools, err := s.App.ConcentratedLiquidityKeeper.GetPools(s.Ctx) | ||
| s.Require().NoError(err) | ||
| // pool id 2 (4 denoms), 3 (3 denoms) created below | ||
| cfmmPools, err := s.App.GAMMKeeper.GetPools(s.Ctx) | ||
| s.Require().NoError(err) | ||
| // pool id 4 (2 denoms), 5 (2 denoms) created below | ||
| cosmWasmPools, err := s.App.CosmwasmPoolKeeper.GetPoolsWithWasmKeeper(s.Ctx) | ||
| s.Require().NoError(err) | ||
| blockPools := commondomain.BlockPools{ | ||
| ConcentratedPools: concentratedPools, | ||
| CFMMPools: cfmmPools, | ||
| CosmWasmPools: cosmWasmPools, | ||
| } | ||
|
|
||
| // Mock out publisher | ||
| publisherMock := &indexermocks.PublisherMock{} | ||
|
|
||
| pairPublisher := blockprocessor.NewPairPublisher(publisherMock, s.App.PoolManagerKeeper) | ||
|
|
||
| err = pairPublisher.PublishPoolPairs(s.Ctx, blockPools.GetAll(), test.createdPoolIDs) | ||
| s.Require().NoError(err) | ||
| s.Require().Equal(test.expectedNumPublishPairCalls, publisherMock.NumPublishPairCalls) | ||
| s.Require().Equal(test.expectedNumPublishPairCallsWithCreation, publisherMock.NumPublishPairCallsWithCreation) | ||
|
|
||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Great job on test coverage and alignment with PR objectives.
The test suite effectively covers the scenarios mentioned in the PR objectives, providing good coverage for the PairPublisher component. The tests validate the PublishPoolPairs functionality with various pool configurations, including the cases without pool creation, with two-denom pool creations, and with four-denom pool creations.
To further enhance the test suite, consider adding a negative test case:
{
name: "error case: invalid pool ID",
createdPoolIDs: map[uint64]commondomain.PoolCreation{
PoolId: 999,
BlockHeight: 12345,
BlockTime: time.Now(),
TxnHash: "txhash1",
},
},
expectedNumPublishPairCalls: 12,
expectedNumPublishPairCallsWithCreation: 0,
expectedError: true,
},Then, modify the test execution to handle the error case:
err = pairPublisher.PublishPoolPairs(s.Ctx, blockPools.GetAll(), test.createdPoolIDs)
if test.expectedError {
s.Require().Error(err)
} else {
s.Require().NoError(err)
s.Require().Equal(test.expectedNumPublishPairCalls, publisherMock.NumPublishPairCalls)
s.Require().Equal(test.expectedNumPublishPairCallsWithCreation, publisherMock.NumPublishPairCallsWithCreation)
}This addition would ensure that the PairPublisher correctly handles invalid input and improves the overall robustness of the test suite.
| for _, test := range tests { | ||
| s.Run(test.name, func() { | ||
| s.Setup() | ||
|
|
||
| // Initialized chain pools | ||
| s.PrepareAllSupportedPools() | ||
|
|
||
| // Get all chain pools from state for asserting later | ||
| // pool id 1 created below | ||
| concentratedPools, err := s.App.ConcentratedLiquidityKeeper.GetPools(s.Ctx) | ||
| s.Require().NoError(err) | ||
| // pool id 2, 3 created below | ||
| cfmmPools, err := s.App.GAMMKeeper.GetPools(s.Ctx) | ||
| s.Require().NoError(err) | ||
| // pool id 4, 5 created below | ||
| cosmWasmPools, err := s.App.CosmwasmPoolKeeper.GetPoolsWithWasmKeeper(s.Ctx) | ||
| s.Require().NoError(err) | ||
| blockPools := commondomain.BlockPools{ | ||
| ConcentratedPools: concentratedPools, | ||
| CFMMPools: cfmmPools, | ||
| CosmWasmPools: cosmWasmPools, | ||
| } | ||
|
|
||
| // Mock out block updates process utils | ||
| blockUpdatesProcessUtilsMock := &sqsmocks.BlockUpdateProcessUtilsMock{} | ||
|
|
||
| // Mock out pool extractor | ||
| poolsExtracter := &commonmocks.PoolsExtractorMock{ | ||
| BlockPools: blockPools, | ||
| CreatedPoolIDs: test.createdPoolIDs, | ||
| } | ||
|
|
||
| // Mock out publisher | ||
| publisherMock := &indexermocks.PublisherMock{} | ||
|
|
||
| // Mock out pair publisher | ||
| pairPublisherMock := &indexermocks.MockPairPublisher{} | ||
|
|
||
| bprocess := blockprocessor.NewBlockUpdatesIndexerBlockProcessStrategy(blockUpdatesProcessUtilsMock, publisherMock, poolsExtracter, pairPublisherMock) | ||
|
|
||
| err = bprocess.PublishCreatedPools(s.Ctx) | ||
| s.Require().NoError(err) | ||
|
|
||
| // Check that the pair publisher is called correctly | ||
| s.Require().Equal(test.expectedPublishPoolPairsCalled, pairPublisherMock.PublishPoolPairsCalled) | ||
| if test.expectedPublishPoolPairsCalled { | ||
| // Check that the number of pools published | ||
| s.Require().Equal(test.expectedNumPoolsPublished, pairPublisherMock.NumPoolPairPublished) | ||
| // Check that the pools and created pool IDs are set correctly | ||
| s.Require().Equal(blockPools.GetAll(), pairPublisherMock.CalledWithPools) | ||
| s.Require().Equal(test.createdPoolIDs, pairPublisherMock.CalledWithCreatedPoolIDs) | ||
| // Check that the number of pools with creation data | ||
| s.Require().Equal(test.expectedNumPoolsWithCreationData, pairPublisherMock.NumPoolPairWithCreationData) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM: Comprehensive test setup and assertions.
The test execution includes a thorough setup with initialization of all supported pools and mocking of dependencies. The assertions cover all expected outcomes specified in the test cases.
-
Consider enhancing error handling for
PublishCreatedPools. Instead of just checking for no error, you could assert on specific error types or messages for negative test cases. -
To prevent potential flakiness due to time-based assertions, consider using a fixed time for
BlockTimein test data. For example:
fixedTime := time.Date(2023, 1, 1, 0, 0, 0, 0, time.UTC)
// Use fixedTime instead of time.Now() in test data- The
s.Setup()call at the beginning of each test run might be redundant ifConcentratedKeeperTestHelperalready provides aSetupTestmethod. Verify if this is necessary or if it can be removed to improve test performance.
cryptomatictrader
added
the
V:state/compatible/backport
ingest/indexer/service/blockprocessor/block_updates_indexer_block_process_strategy_test.go
Outdated
Show resolved
Hide resolved
…8746) * add test cases to block_update_indexer_block_process_strat * enhanced pair publisher mock * renamed some vars * add test cases to full_indexer_block_process_strategy * add test cases to publishpoolpairs * fixed racing cond when multi go routine calling the mock * make GetTradingPairTakerFee and GetSpreadFactor thread safe * try * fix race test issues * dry up test data creation --------- Co-authored-by: Roman <roman@osmosis.team> (cherry picked from commit f0a195f)
…8746) (#8753) * add test cases to block_update_indexer_block_process_strat * enhanced pair publisher mock * renamed some vars * add test cases to full_indexer_block_process_strategy * add test cases to publishpoolpairs * fixed racing cond when multi go routine calling the mock * make GetTradingPairTakerFee and GetSpreadFactor thread safe * try * fix race test issues * dry up test data creation --------- Co-authored-by: Roman <roman@osmosis.team> (cherry picked from commit f0a195f) Co-authored-by: Calvin <calvinchso@gmail.com>
This commit adds test cases to pair_publisher::PublishPoolPairs
The pair publisher is responsible for iterating over all pools and their denoms combinations, with optional creation details provided in the parameter, it also append creation details to the pair struct.
Since the test suite initializes all supported pools (concentrated, cfmm, cosmwasm) via s.App.PrepareAllSupportedPools(), the created pool IDs are 1-5, and the number of expected calls to the publisher is 12 coz: pool id 1 has 2 denoms, pool id 2 has 4 denoms, pool id 3 has 3 denoms, pool id 4 has 2 denoms, pool id 5 has 2 denoms.
Scenarios tested: