test: allo & registry integration tests #67
Conversation
📝 WalkthroughWalkthroughThe changes update the integration testing framework for the Orchestrator by adding new test suites for both Allo and Registry event processing. New integration test files simulate event processing using the Vitest framework and verify that appropriate changesets are applied to repositories, manage roles, and log warnings where applicable. Several helper modules have been introduced or enhanced to provide mock implementations of repositories, providers, and event generators. A utility function was also added for monitoring asynchronous processing during tests. Additionally, modifications were made to event handler and type definitions by changing how nonce and metadata values are represented—shifting from bigint to string. These updates affect the testing infrastructure and ensure that events are processed consistently with the revised type definitions across the system. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (14)
packages/data-flow/test/integration/allo.integration.spec.ts (1)
239-317: "PoolMetadataUpdated" event handling is clear, but avoid casting to "unknown as Round".
Currently, mocking the round object usesunknown as Round. As a good-to-have improvement, consider creating a more strongly typed mock object to reflect the real shape of theRoundtype. This can help prevent type errors from creeping into tests.packages/data-flow/test/integration/helpers/testing.ts (1)
3-17: Helpful waiting mechanism for asynchronous event processing.
Usingvi.waitForto confirm both spies have been called is a good approach to stabilize tests. However, if CI environments experience slowdowns, consider increasing the 500ms timeout or the interval to reduce flakiness.packages/data-flow/test/integration/helpers/eventFactory.ts (3)
18-56: Add JSDoc documentation for the createTestAlloEvent function.The function implementation is well-structured, but it's missing JSDoc documentation as required by the coding guidelines.
Add documentation like this:
+/** + * Creates a test event for the Allo contract with default values. + * @template E - The type of Allo event + * @param {Object} params - The event parameters + * @param {E} params.eventName - The name of the event + * @param {EventParams<"Allo", E>} params.params - The event-specific parameters + * @param {number} [params.blockNumber] - The block number (defaults to DEFAULT_BLOCK_NUMBER) + * @param {TimestampMs} [params.blockTimestamp] - The block timestamp (defaults to DEFAULT_TIMESTAMP_MS) + * @param {Address} [params.srcAddress] - The source address (defaults to DEFAULT_SRC_ADDRESS) + * @param {number} [params.logIndex] - The log index (defaults to 0) + * @param {number} [params.chainId] - The chain ID (defaults to 1) + * @param {Address} [params.from] - The from address (defaults to DEFAULT_FROM_ADDRESS) + * @param {number} [params.txIndex] - The transaction index (defaults to 0) + * @returns {ProcessorEvent<"Allo", AlloEvent>} The created test event + */ export const createTestAlloEvent = <E extends AlloEvent>({
58-94: Add JSDoc documentation for the createTestRegistryEvent function.Similar to the Allo event factory, this function needs JSDoc documentation.
Add documentation like this:
+/** + * Creates a test event for the Registry contract with default values. + * @template E - The type of Registry event + * @param {Object} params - The event parameters + * @param {E} params.eventName - The name of the event + * @param {EventParams<"Registry", E>} params.params - The event-specific parameters + * @param {number} [params.blockNumber] - The block number (defaults to DEFAULT_BLOCK_NUMBER) + * @param {TimestampMs} [params.blockTimestamp] - The block timestamp (defaults to DEFAULT_TIMESTAMP_MS) + * @param {number} [params.logIndex] - The log index (defaults to 0) + * @param {number} [params.chainId] - The chain ID (defaults to 1) + * @param {Address} [params.srcAddress] - The source address (defaults to DEFAULT_SRC_ADDRESS) + * @param {Address} [params.from] - The from address (defaults to DEFAULT_FROM_ADDRESS) + * @param {number} [params.txIndex] - The transaction index (defaults to 0) + * @returns {ProcessorEvent<"Registry", RegistryEvent>} The created test event + */ export const createTestRegistryEvent = <E extends RegistryEvent>({
96-135: Add JSDoc documentation for the createTestStrategyEvent function.The Strategy event factory also needs JSDoc documentation.
Add documentation like this:
+/** + * Creates a test event for the Strategy contract with default values. + * @template E - The type of Strategy event + * @param {Object} params - The event parameters + * @param {E} params.eventName - The name of the event + * @param {EventParams<"Strategy", E>} params.params - The event-specific parameters + * @param {number} params.blockNumber - The block number + * @param {Address} params.strategyId - The strategy ID + * @param {TimestampMs} [params.blockTimestamp] - The block timestamp (defaults to DEFAULT_TIMESTAMP_MS) + * @param {number} [params.logIndex] - The log index (defaults to 0) + * @param {number} [params.chainId] - The chain ID (defaults to 1) + * @param {Address} [params.srcAddress] - The source address (defaults to DEFAULT_SRC_ADDRESS) + * @param {Address} [params.from] - The from address (defaults to DEFAULT_FROM_ADDRESS) + * @param {number} [params.txIndex] - The transaction index (defaults to 0) + * @returns {ProcessorEvent<"Strategy", E>} The created test event + */ export const createTestStrategyEvent = <E extends StrategyEvent>({packages/data-flow/test/integration/helpers/dependencies.ts (3)
16-82: Add JSDoc documentation for createMockRepositories.The function provides comprehensive mocking but lacks documentation.
Add documentation like this:
+/** + * Creates mock implementations of core repositories for testing. + * @returns {Object} Mock repositories with Vi spies for all methods + * @property {Object} projectRepository - Mock project repository + * @property {Object} roundRepository - Mock round repository + * @property {Object} applicationRepository - Mock application repository + * @property {Object} donationRepository - Mock donation repository + * @property {Object} applicationPayoutRepository - Mock application payout repository + * @property {Object} transactionManager - Mock transaction manager + */ export const createMockRepositories = (): Pick<
85-100: Add JSDoc documentation for createMockProviders.The providers mock implementation needs documentation.
Add documentation like this:
+/** + * Creates mock implementations of core providers for testing. + * @returns {Object} Mock providers + * @property {Object} pricingProvider - DummyPricingProvider instance + * @property {Object} metadataProvider - Mock metadata provider + * @property {Object} evmProvider - Mock EVM provider + */ export const createMockProviders = (): Pick<
120-150: Simplify the strategy registry mock implementation.The nested map lookup could be simplified using optional chaining.
Consider this simpler implementation:
- getStrategyId: vi.fn().mockImplementation((chainId: ChainId, strategyAddress: Address) => { - const chainIdMap = strategiesMap.get(chainId); - if (!chainIdMap) { - return undefined; - } - const strategyId = chainIdMap.get(strategyAddress); - if (!strategyId) { - return undefined; - } - return { - id: strategyId, - address: strategyAddress, - chainId: chainId, - handled: true, - }; - }), + getStrategyId: vi.fn().mockImplementation((chainId: ChainId, strategyAddress: Address) => { + const strategyId = strategiesMap.get(chainId)?.get(strategyAddress); + return strategyId ? { + id: strategyId, + address: strategyAddress, + chainId, + handled: true, + } : undefined; + }),packages/processors/test/registry/handlers/profileCreated.handler.spec.ts (3)
83-83: Improve test description to better convey intent.The test description could be more descriptive. Consider renaming it to better convey what is being tested.
Apply this diff to improve the test description:
- it("handles ProfileCreated event and return the correct changeset", async () => { + it("returns InsertProject and InsertProjectRole changesets when handling ProfileCreated event", async () => {
143-143: Remove "should" from test description.According to the coding guidelines, test descriptions should avoid using the word "should".
Apply this diff to improve the test description:
- it.skip("logs a warning if metadata parsing fails", async () => { + it.skip("logs a warning when metadata parsing fails", async () => {
35-77: Extract test setup into helper functions.The test setup is quite lengthy. Consider extracting the mock setup into helper functions to improve readability and maintainability.
Create a new helper file
test/helpers/mockSetup.tswith the following content:import { ILogger } from "@grants-stack-indexer/shared"; import { vi } from "vitest"; export function createMockLogger(): ILogger { return { debug: vi.fn(), error: vi.fn(), info: vi.fn(), warn: vi.fn(), }; } export function createMockDependencies(mockedAddress: string) { return { projectRepository: { getPendingProjectRolesByRole: vi.fn().mockResolvedValue([]), } as unknown as IProjectReadRepository, evmProvider: { getTransaction: vi.fn().mockResolvedValue({ from: mockedAddress }), } as unknown as EvmProvider, pricingProvider: { getTokenPrice: vi.fn(), } as unknown as IPricingProvider, metadataProvider: { getMetadata: vi.fn(), } as unknown as IMetadataProvider, roundRepository: {} as unknown as IRoundReadRepository, applicationRepository: {} as unknown as IApplicationReadRepository, logger: createMockLogger(), }; }Then simplify the test setup:
- const logger: ILogger = { - debug: vi.fn(), - error: vi.fn(), - info: vi.fn(), - warn: vi.fn(), - }; beforeEach(() => { - mockDependencies = { - projectRepository: { - getPendingProjectRolesByRole: vi.fn().mockResolvedValue([]), - } as unknown as IProjectReadRepository, - evmProvider: { - getTransaction: vi.fn().mockResolvedValue({ from: mockedAddress }), - } as unknown as EvmProvider, - pricingProvider: { - getTokenPrice: vi.fn(), - } as unknown as IPricingProvider, - metadataProvider: { - getMetadata: vi.fn(), - } as unknown as IMetadataProvider, - roundRepository: {} as unknown as IRoundReadRepository, - applicationRepository: {} as unknown as IApplicationReadRepository, - logger, - }; + mockDependencies = createMockDependencies(mockedAddress); });packages/data-flow/test/integration/registry.integration.spec.ts (3)
42-47: Enhance test cleanup.The test cleanup could be more robust by ensuring all mocks are properly restored.
Apply this diff to improve the cleanup:
afterEach(async () => { vi.clearAllMocks(); + vi.restoreAllMocks(); abortController.abort(); await runPromise; runPromise = undefined; + + // Ensure all spies are properly cleaned up + Object.values(mocks.dependencies).forEach((dependency) => { + Object.values(dependency).forEach((method) => { + if (method?.mockRestore) { + method.mockRestore(); + } + }); + }); });
29-40: Extract test setup into a helper function.The test setup could be more concise by extracting the orchestrator initialization into a helper function.
Move the orchestrator initialization to
helpers/setup.ts:export function initializeTestOrchestrator(chainId: ChainId, abortController: AbortController) { const res = createTestOrchestrator({ chainId, strategiesMap: DEFAULT_STRATEGY_MAP, }); return { orchestrator: res.orchestrator, mocks: res.mocks, runPromise: undefined, }; }Then simplify the test setup:
beforeEach(() => { - const res = createTestOrchestrator({ - chainId, - strategiesMap: DEFAULT_STRATEGY_MAP, - }); - - orchestrator = res.orchestrator; - mocks = res.mocks; - - abortController = new AbortController(); - runPromise = undefined; + abortController = new AbortController(); + const setup = initializeTestOrchestrator(chainId, abortController); + orchestrator = setup.orchestrator; + mocks = setup.mocks; + runPromise = setup.runPromise; });
392-392: Improve test descriptions for role-related events.The test descriptions for role-related events could be more descriptive to better convey the test scenarios.
Apply these diffs to improve the test descriptions:
- it("process RoleGranted event and apply InsertProjectRole changeset when project exists", async () => { + it("inserts project role when processing RoleGranted event for existing project", async () => { - it("process RoleGranted event and apply InsertPendingProjectRole changeset when project doesn't exist", async () => { + it("inserts pending project role when processing RoleGranted event for non-existent project", async () => { - it("process RoleRevoked event and apply DeleteAllProjectRolesByRoleAndAddress changeset when project exists", async () => { + it("deletes project roles when processing RoleRevoked event for existing project", async () => {Also applies to: 453-453, 506-506
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/data-flow/test/integration/allo.integration.spec.ts(1 hunks)packages/data-flow/test/integration/helpers/dependencies.ts(1 hunks)packages/data-flow/test/integration/helpers/eventFactory.ts(1 hunks)packages/data-flow/test/integration/helpers/setup.ts(1 hunks)packages/data-flow/test/integration/helpers/testing.ts(1 hunks)packages/data-flow/test/integration/registry.integration.spec.ts(1 hunks)packages/processors/src/processors/registry/handlers/profileCreated.handler.ts(1 hunks)packages/processors/test/registry/handlers/profileCreated.handler.spec.ts(1 hunks)packages/shared/src/types/events/registry.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`: Review TypeScript files for adherence to the fo...
**/*.ts: Review TypeScript files for adherence to the following guidelines:
- Avoid over-abstraction; prioritize composition over inheritance.
- Use dependency injection and follow SOLID principles.
- Avoidany; useunknownwhen necessary.
- Use runtime type-checking for environment variables (e.g., Zod).
- Prevent circular dependencies with the internal module pattern.
- Libraries should have anexternal.tsfile explicitly listing public exports.
- Usebigintas-is; cast toNumberonly when exposing values via APIs.
- Document all code with JSDoc.
- Encourage static async factory functions for constructors.
- Avoid overly nitpicky feedback beyond these best practices.
packages/processors/src/processors/registry/handlers/profileCreated.handler.tspackages/processors/test/registry/handlers/profileCreated.handler.spec.tspackages/data-flow/test/integration/helpers/setup.tspackages/data-flow/test/integration/helpers/dependencies.tspackages/shared/src/types/events/registry.tspackages/data-flow/test/integration/registry.integration.spec.tspackages/data-flow/test/integration/allo.integration.spec.tspackages/data-flow/test/integration/helpers/eventFactory.tspackages/data-flow/test/integration/helpers/testing.ts
`**/*.spec.ts`: Review the unit test files with the followin...
**/*.spec.ts: Review the unit test files with the following guidelines:
- Avoid using the word "should" in test descriptions.
- Ensure descriptive test names convey the intent of each test.
- Validate adherence to the Mocha/Chai/Jest test library best practices.
- Be concise and avoid overly nitpicky feedback outside of these best practices.
packages/processors/test/registry/handlers/profileCreated.handler.spec.tspackages/data-flow/test/integration/registry.integration.spec.tspackages/data-flow/test/integration/allo.integration.spec.ts
🔇 Additional comments (8)
packages/data-flow/test/integration/allo.integration.spec.ts (4)
1-2: Imports and dependencies look well-organized.
No issues detected with importing fromviemandvitest. The chosen approach facilitates straightforward test setup.
19-54: Efficient use of beforeEach and afterEach blocks.
The approach to reset mocks and abort the controller in each test run is clear, preventing cross-test pollution. This test setup is well-structured.
56-174: Comprehensive test for "PoolCreated" event.
The test thoroughly verifies calls to spies, database operations, and final state checks. Great job mocking the round repository and ensuring the correct parameters for all changesets (InsertRound, InsertRoundRole, DeletePendingRoundRoles).
512-558: Tests for unmatched roles demonstrate negative scenario coverage.
The logic that merely logs a warning instead of applying changesets when no round is found is well-validated. This ensures resilience in unexpected role assignments.packages/data-flow/test/integration/helpers/setup.ts (1)
18-82: Well-structured creation of a test orchestrator with dependency injection.
This factory utilizes a mock logger, notifier, repositories, and clients, aligning with SOLID principles. It also helps ensure each test can run with a fresh orchestrator instance and stable mocks.packages/shared/src/types/events/registry.ts (2)
52-54: LGTM! Improved type definitions for blockchain values.The change from
biginttostringwith clarifying comments about the underlying Solidity types (uint256) improves type safety and aligns with blockchain best practices.
60-60: LGTM! Consistent type representation.The metadata protocol type change maintains consistency with the updated type definitions.
packages/processors/src/processors/registry/handlers/profileCreated.handler.ts (1)
62-62: LGTM! Proper type conversion for nonce value.The change correctly converts the string nonce to BigInt, aligning with the updated type definitions while maintaining proper data handling.
| chainId?: number; | ||
| from?: Address; | ||
| txIndex?: number; | ||
| }): ProcessorEvent<"Allo", AlloEvent> => { |
There was a problem hiding this comment.
should it be :
| }): ProcessorEvent<"Allo", AlloEvent> => { | |
| }): ProcessorEvent<"Allo", E> => { |
??
| srcAddress?: Address; | ||
| from?: Address; | ||
| txIndex?: number; | ||
| }): ProcessorEvent<"Registry", RegistryEvent> => { |
| srcAddress?: Address; | ||
| from?: Address; | ||
| txIndex?: number; | ||
| }): ProcessorEvent<"Strategy", E> => { |
# 🤖 Linear Closes GIT-265 GIT-262 ## Description Write integration tests for Strategy events on Orchestrator level (continuation of #67). Also, fix some issues on Registration handler regarding Date and BlockTimestamp conversion <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Expanded strategy mapping to support additional entries. - **Refactor** - Enhanced event handling by making a key parameter optional with a default value. - Refined timestamp processing to ensure consistent and accurate date representations. - **Tests** - Introduced new integration tests to validate and monitor strategy event processing and time accuracy. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
# 🤖 Linear Closes GIT-265 GIT-262 ## Description Write integration tests for Strategy events on Orchestrator level (continuation of #67). Also, fix some issues on Registration handler regarding Date and BlockTimestamp conversion <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Expanded strategy mapping to support additional entries. - **Refactor** - Enhanced event handling by making a key parameter optional with a default value. - Refined timestamp processing to ensure consistent and accurate date representations. - **Tests** - Introduced new integration tests to validate and monitor strategy event processing and time accuracy. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 Linear
Closes GIT-263 GIT-264
Description
We write integration tests for Allo and Registry events on Orchestrator level. We aim at verifying that Processors and DataLoader communication is working correctly. Although these are integration tests, we mock some components (mainly external ones and Repositories)
Summary by CodeRabbit
New Features
Bug Fixes