Centralized Mock Context Factory for GraphQL Resolver Unit Tests #3319
Conversation
WalkthroughThis pull request refactors the way GraphQL context mocks are created throughout the test suite. Custom implementations of mock context creation (e.g., using Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test File
participant MCTX as createMockGraphQLContext
participant Resolver as GraphQL Resolver
participant Drizzle as DrizzleClient (mock)
Test->>MCTX: Call createMockGraphQLContext(isAuthenticated, userId)
MCTX-->>Test: Return { context, mocks }
Test->>Resolver: Invoke resolver with context
Resolver->>Drizzle: Execute query via mocks.drizzleClient.query
Drizzle-->>Resolver: Return mock data
Resolver-->>Test: Return result
Possibly related PRs
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3319 +/- ##
=================================================
Coverage 51.90% 51.90%
=================================================
Files 466 466
Lines 35029 35029
Branches 1170 1169 -1
=================================================
Hits 18182 18182
Misses 16845 16845
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (4)
test/graphql/types/Advertisement/updatedAt.test.ts (1)
46-48: 🧹 Nitpick (assertive)Consider using mockImplementation for complex types
Based on the retrieved learning, when dealing with complex types, consider using
mockImplementation(() => Promise.resolve(value))instead ofmockResolvedValue(value)for better type safety.- ).mockResolvedValue(undefined); + ).mockImplementation(() => Promise.resolve(undefined));test/graphql/types/FundCampaign/updatedAt.test.ts (1)
39-40: 🧹 Nitpick (assertive)Consider using mockImplementation for complex return types
Based on previous feedback in the organization, when mocking functions with specific return types in Vitest, prefer using
mockImplementation(() => Promise.resolve(value))overmockResolvedValue(value)for complex types to ensure type safety.For example:
- ).mockResolvedValue(mockUser); + ).mockImplementation(() => Promise.resolve(mockUser));Also applies to: 48-49, 69-70, 89-90, 108-109, 119-120, 129-130, 139-140, 149-150, 159-160, 170-171, 187-188, 205-206
test/graphql/types/tag/creator.test.ts (2)
53-54: 🧹 Nitpick (assertive)Consider using mockImplementation for complex return types
Based on previous feedback in the organization, when mocking functions with specific return types in Vitest, prefer using
mockImplementation(() => Promise.resolve(value))overmockResolvedValue(value)for complex types to ensure type safety.For example:
- ).mockResolvedValue(mockUserData); + ).mockImplementation(() => Promise.resolve(mockUserData));Also applies to: 71-72, 87-88, 104-105, 113-114, 160-161, 176-177, 193-194
264-268: 🧹 Nitpick (assertive)Inconsistent error message handling
Some GraphQL errors include a custom message while others don't. For consistency, consider either always including a message parameter or never including it.
Compare:
new TalawaGraphQLError({ extensions: { code: "unexpected" } })vs:
new TalawaGraphQLError({ message: "Internal server error", extensions: { code: "unexpected" }, })Also applies to: 279-284, 295-300, 311-316, 342-345
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
test/MockContext/mockContextCreator.ts(1 hunks)test/MockContext/mocks/drizzleClientMock.ts(1 hunks)test/MockContext/mocks/mockMinioClient.ts(1 hunks)test/MockContext/mocks/pubSubMock.ts(1 hunks)test/graphql/types/Advertisement/updatedAt.test.ts(2 hunks)test/graphql/types/Community/updater.test.ts(8 hunks)test/graphql/types/Event/creator.test.ts(2 hunks)test/graphql/types/Event/updatedAt.test.ts(2 hunks)test/graphql/types/Fund/updater.test.ts(10 hunks)test/graphql/types/FundCampaign/updatedAt.test.ts(2 hunks)test/graphql/types/FundCampaignPledge/updater.test.ts(10 hunks)test/graphql/types/Organization/creator.test.ts(2 hunks)test/graphql/types/tag/creator.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
test/graphql/types/Event/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-02-23T16:10:02.480Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
test/graphql/types/FundCampaign/updatedAt.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
test/graphql/types/Advertisement/updatedAt.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
test/graphql/types/Community/updater.test.ts (1)
Learnt from: shivasankaran18
PR: PalisadoesFoundation/talawa-api#3175
File: test/graphql/types/Mutation/updateAgendaItem.test.ts:62-79
Timestamp: 2025-02-10T18:48:15.739Z
Learning: When mocking GraphQLContext in tests, keep the minio property with minimal implementation to satisfy TypeScript types, even if minio operations aren't used in the specific resolver being tested.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (23)
test/MockContext/mocks/pubSubMock.ts (1)
5-17: Well-implemented mock PubSub factoryThe implementation elegantly simulates both publish/subscribe operations with minimal code. The empty Readable stream with an async iterator is a good pattern for mocking event streams without actual data flow.
test/MockContext/mockContextCreator.ts (2)
17-27: Good separation of authenticated and unauthenticated client mocksClear separation of concerns with well-defined constants for different authentication states.
29-38: JSDoc comments are well detailedComprehensive documentation with usage examples and parameter descriptions that make the function's purpose clear.
test/graphql/types/Event/updatedAt.test.ts (2)
3-9: Successfully migrated to centralized context creatorThe import changes properly reflect the migration from a local mock context to the centralized implementation.
24-26: Clean implementation of the new mock context factoryThe test setup now uses the centralized context creator with appropriate parameters for authentication and user ID.
test/graphql/types/FundCampaignPledge/updater.test.ts (4)
1-8: Clean implementation of the new mock context importsThe top-level setup correctly uses the new mock context factory.
47-59: Good handling of multi-line type assertionsThe type assertion is properly formatted across multiple lines for readability.
196-203: Nice implementation of conditional mock behaviorThe mock implementation with conditional behavior based on input parameters is a robust pattern for testing different scenarios.
209-226: Well-structured mock implementation for complex returnsThe mock implementation correctly handles complex nested object structures that reflect the real data model.
test/graphql/types/Organization/creator.test.ts (2)
1-1: Centralized mock context improves test maintainabilityThe implementation of a centralized mock context is a welcome improvement. Replacing the custom mock context with
createMockGraphQLContextsimplifies the code and promotes consistency across test files.Also applies to: 4-4, 23-23
61-62: Type assertion is correctly used for mocked implementationsThe type assertion with
ReturnType<typeof vi.fn>is properly applied to access mock functions, addressing the challenge mentioned in the PR objectives regarding discrepancies between actual and mocked types.test/MockContext/mocks/drizzleClientMock.ts (2)
4-29: Well-structured type definitions for mocked database operationsThe type definitions for
TableMethodsand the implementation ofcreateTableMethodsprovide a solid foundation for mocking database operations. The default return values make sense for testing scenarios.
31-43: Dynamic creation of mock tables based on schemaThe approach of dynamically creating mock methods for all tables in the schema is elegant and maintainable. This ensures that the mock structure will automatically adapt if new tables are added to the schema.
test/MockContext/mocks/mockMinioClient.ts (1)
10-27: Consider implementing full MinIO client interfaceThe mock implementation returns
Partial<MinioClient>which is sufficient if only these methods are used in tests. However, this might cause issues if tests start using other methods from the MinioClient interface.Consider implementing all required methods or adding a comment explaining which methods are expected to be used in tests.
test/graphql/types/Advertisement/updatedAt.test.ts (1)
1-1: Consistent mock context usage across test filesThe replacement of the custom mock context with the centralized
createMockGraphQLContextensures consistency with other test files, making the codebase more maintainable.Also applies to: 4-4, 24-24
test/graphql/types/Event/creator.test.ts (1)
89-104: Review commented test case.Consider either implementing this test case if it tests valid functionality or removing it entirely if it's no longer needed. Commented-out test code can confuse future maintainers about its purpose or status.
If this commented code represents a valid test scenario that should be tested, uncomment and implement it. Otherwise, remove it to keep the test file clean.
test/graphql/types/Fund/updater.test.ts (2)
1-1: LGTM! Good use of the centralized mock contextThe change from a custom TestContext to using the centralized mock context factory improves consistency and reduces duplication across tests, which aligns perfectly with the PR objectives.
Also applies to: 6-6, 9-9, 24-24
38-40: Consistent type assertion pattern for mocked methodsThe type assertion pattern used throughout the file (
as ReturnType<typeof vi.fn>) effectively resolves the type compatibility issues between the actual drizzleClient and its mocked version as mentioned in the PR objectives.Also applies to: 48-50, 55-57, 75-77, 82-84, 102-104, 114-116, 134-136, 140-142, 159-161, 176-176, 179-181, 204-204, 207-209
test/graphql/types/FundCampaign/updatedAt.test.ts (2)
1-4: Good refactoring with appropriate importsThe import changes appropriately set up the necessary types and functions for the centralized mock context approach.
27-27: Simplified mock context creation looks goodThe refactored code uses the centralized factory function with clear parameters for authentication state and user ID, which greatly simplifies test setup.
test/graphql/types/tag/creator.test.ts (3)
1-4: Imports look goodThe import changes appropriately set up the necessary types and functions for the centralized mock context approach.
24-24: Simplified mock context creation looks goodThe refactored code uses the centralized factory function with clear parameters for authentication state and user ID, which greatly simplifies test setup.
220-224: Good sequential mock implementation patternThe chaining of multiple mock implementations using
mockResolvedValueOnceis a good pattern for testing sequential calls to the same method.
|
@palisadoes did not apply this factory function to these unit tests as some were following a bit different approach to the testing
should open an issue after this |
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
test/graphql/types/Chat/updatedAt.test.ts(7 hunks)test/graphql/types/Event/updater.test.ts(10 hunks)test/graphql/types/Fund/creator.test.ts(2 hunks)test/graphql/types/Venue/updater.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
test/graphql/types/Chat/updatedAt.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
test/graphql/types/Event/updater.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-02-23T16:10:02.480Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
test/graphql/types/Venue/updater.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-02-23T16:10:02.480Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
🪛 GitHub Actions: Pull request workflow
test/graphql/types/Chat/updatedAt.test.ts
[error] 1-1: Import statements could be sorted.
test/graphql/types/Event/updater.test.ts
[error] 1-1: Import statements could be sorted.
test/graphql/types/Fund/creator.test.ts
[error] 22-22: Formatter would have printed the following content: ctx = createMockGraphQLContext(true, 'user-123');
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (7)
test/graphql/types/Venue/updater.test.ts (2)
1-1: Good addition of the centralized mock context import.The import of
createMockGraphQLContextfrom the centralized location aligns with the PR objective of standardizing mock context creation.
21-21: Nice implementation of the centralized mock context.The replacement of custom context creation with the centralized factory function enhances maintainability and reduces code duplication. The parameters passed (authentication state and user ID) appropriately configure the mock context for testing.
test/graphql/types/Event/updater.test.ts (2)
30-32: Good implementation of mock contexts.Creating both authenticated and unauthenticated contexts using the centralized factory function improves test consistency and readability.
51-54: Good implementation of mock resolving.The mock implementation correctly applies ReturnType for type safety when accessing the mock function on the drizzle client.
test/graphql/types/Fund/creator.test.ts (1)
1-8: Good adoption of centralized mock context.The import of
createMockGraphQLContextand replacement of the custom context type withGraphQLContextaligns with the PR objective of standardizing mock context creation.test/graphql/types/Chat/updatedAt.test.ts (2)
20-22: Good implementation of mock contexts.Creating both authenticated and unauthenticated contexts using the centralized factory function improves test consistency and readability.
39-42: Effective type assertion for mock function.Using
ReturnType<typeof vi.fn>for type assertion addresses the challenge mentioned in the PR description regarding discrepancies between the actualdrizzleClienttype and its mocked version.The approach aligns with the learning retrieved from previous feedback about preferring
mockImplementationormockResolvedValuewhen dealing with complex types.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/graphql/types/Event/updater.test.ts (1)
1-5: 🛠️ Refactor suggestionSort import statements to fix pipeline failure.
The import statements should be sorted according to the codebase standards.
-import { createMockGraphQLContext } from "test/MockContext/mockContextCreator"; -import { describe, expect, it, type vi } from "vitest"; -import type { z } from "zod"; -import type { userRoleEnum } from "~/src/drizzle/enums/userRole"; -import { resolveEventUpdater } from "../../../../src/graphql/types/Event/updater"; +import { describe, expect, it, type vi } from "vitest"; +import type { z } from "zod"; +import { createMockGraphQLContext } from "test/MockContext/mockContextCreator"; +import type { userRoleEnum } from "~/src/drizzle/enums/userRole"; +import { resolveEventUpdater } from "../../../../src/graphql/types/Event/updater";test/graphql/types/Chat/updatedAt.test.ts (1)
1-5: 🛠️ Refactor suggestionSort import statements to fix pipeline failure.
The import statements should be sorted according to the codebase standards, with Vitest imports consolidated.
-import { createMockGraphQLContext } from "test/MockContext/mockContextCreator"; -import { beforeEach, describe, expect, it } from "vitest"; -import { vi } from "vitest"; -import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError"; -import { resolveUpdatedAt } from "../../../../src/graphql/types/Chat/updatedAt"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createMockGraphQLContext } from "test/MockContext/mockContextCreator"; +import { resolveUpdatedAt } from "../../../../src/graphql/types/Chat/updatedAt"; +import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
test/graphql/types/Chat/updatedAt.test.ts(7 hunks)test/graphql/types/Event/updater.test.ts(10 hunks)test/graphql/types/Fund/creator.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
test/graphql/types/Event/updater.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-02-23T16:10:02.480Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
test/graphql/types/Chat/updatedAt.test.ts (1)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (5)
test/graphql/types/Event/updater.test.ts (2)
30-32: LGTM! Good implementation of the centralized mock context.The refactoring to use the centralized mock context factory improves consistency across tests.
51-54: 🧹 Nitpick (assertive)Consider using mockImplementation for better type safety.
Based on project learnings, consider using
mockImplementation(() => Promise.resolve(undefined))instead ofmockResolvedValuewhen dealing with complex types to ensure type safety.( authenticatedContext.drizzleClient.query.usersTable .findFirst as ReturnType<typeof vi.fn> -).mockImplementation(() => Promise.resolve(undefined)); +).mockImplementation(() => Promise.resolve(undefined));Likely an incorrect or invalid review comment.
test/graphql/types/Fund/creator.test.ts (2)
1-8: Good job refactoring to use the centralized context creator.The imports are properly organized, and the removal of custom context creation simplifies the code.
22-22: Spacing is correct in the context initialization.The spacing in the
createMockGraphQLContextcall is now properly formatted with a space after the comma.test/graphql/types/Chat/updatedAt.test.ts (1)
20-22: Good implementation of the centralized mock context.Using the
createMockGraphQLContextfunction to create both authenticated and unauthenticated contexts simplifies test setup.
-didn't change that in this PR incorrect unit testing (not covering actual resolver) |
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
schema.graphql(2 hunks)src/graphql/types/Query/allUsers.ts(5 hunks)src/utilities/defaultGraphQLConnection.ts(4 hunks)test/MockContext/mockContextCreator.ts(1 hunks)test/MockContext/mocks/mockMinioClient.ts(1 hunks)test/routes/graphql/Query/allUsers.test.ts(4 hunks)test/routes/graphql/documentNodes.ts(1 hunks)test/routes/graphql/gql.tada-cache.d.ts(1 hunks)test/routes/graphql/gql.tada.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (21)
test/routes/graphql/gql.tada.d.ts (1)
159-159: LGTM! New QueryAllUsersWhereInput type added correctly.The addition of the
QueryAllUsersWhereInputtype as an input object with an optionalnamefield is properly structured and follows the existing pattern in the file. This change provides a more flexible way to filter users in GraphQL queries.test/routes/graphql/documentNodes.ts (1)
246-295: Good refactoring to use the new where input parameter.The GraphQL query has been properly updated to use the new
whereparameter of typeQueryAllUsersWhereInputinstead of the directnameparameter. This change makes the query more flexible and extensible for future filtering options.test/MockContext/mockContextCreator.ts (1)
1-62: Well-structured mock context factory implementation.This new file introduces a centralized approach for creating mock GraphQL contexts in tests, which improves code maintainability. The implementation includes:
- Clear constants for representing authenticated and unauthenticated clients
- Well-documented function with proper TypeScript typing
- Comprehensive mocking of all GraphQL context components
- Good explanation for the intentional type assertion
test/routes/graphql/Query/allUsers.test.ts (4)
307-307: LGTM! Updated test to use new where parameter structure.Test correctly updated to use the new
where: { name: uniqueName }structure instead of directly passingname.
341-344: LGTM! Properly formatted where parameter for non-matching name test.The test for non-matching name searches has been correctly updated to use the nested
whereobject structure.
358-361: LGTM! Properly formatted where parameter for "last" pagination test.The test using the
lastparameter with a non-matching name search has been correctly updated to use the new parameter structure.
376-378: LGTM! Updated minimum name length validation test.The input validation test for minimum name length correctly uses the new
whereobject structure.schema.graphql (2)
2870-2870: Refactored allUsers query parameter to use a structured input typeThe query parameter has been changed from a simple
nameparameter to a structuredwhereinput object. This follows GraphQL best practices by providing a more extensible approach for adding future filtering criteria.
2955-2957: New input type created to support the refactored queryThe
QueryAllUsersWhereInputtype provides a structured container for the existingnamefilter, creating a more maintainable and extensible API. This approach will make it easier to add additional filtering criteria in the future without changing the query signature.test/MockContext/mocks/mockMinioClient.ts (4)
1-8: Good organization with imports and type definitionsThe imports are well-organized, and the custom type definition for
UploadedObjectInfomakes the code more readable and maintainable.
13-19: Well-structured return type for the mock creator functionThe return type clearly defines both the mock client and its configuration, which will make it easier to use in tests.
20-47: Comprehensive mock implementation of MinioClientThe mock implementation covers essential MinIO client methods with sensible defaults and error handling. The
getObjectimplementation properly simulates a readable stream with content and includes error handling for invalid bucket names or missing object names.
32-45: Good stream implementation with proper error handlingThe
getObjectimplementation correctly validates parameters and creates a properly functioning readable stream with mock content.test/routes/graphql/gql.tada-cache.d.ts (1)
27-28: Update to Query_allUsers follows GraphQL best practices.The change from using a direct
nameparameter to a structuredwhereinput object enhances query flexibility and follows GraphQL best practices for filtering. This aligns with the corresponding schema changes and maintains proper typing.src/utilities/defaultGraphQLConnection.ts (3)
24-36: Well-structured type definition for connection arguments with filtering.The new
ParsedDefaultGraphQLConnectionArgumentsWithWheretype properly extends the base connection arguments with generic typing for the where clause, maintaining type safety while adding filtering capabilities.
140-153: Clean implementation of schema creation for where filtering.The
createGraphQLConnectionWithWhereSchemafunction elegantly extends the default connection schema with a custom where schema, supporting optional filtering with proper defaults.
155-184: Effective transformation logic for connection arguments with where clauses.The
transformGraphQLConnectionArgumentsWithWherefunction correctly separates the where clause handling from the base connection transformation, reusing existing logic while maintaining the where context. The implementation is well-structured and properly typed.src/graphql/types/Query/allUsers.ts (4)
26-31: Well-defined schema for user filtering.The
userWhereSchemaproperly defines the structure for filtering users, currently supporting name filtering with appropriate validation.
85-92: GraphQL schema field properly updated to use structured input type.The query field now accepts a properly typed where input parameter, which aligns with GraphQL best practices for filtering.
147-151: Type-safe extraction of where clause from parsed arguments.The resolver correctly extracts the where clause from parsed arguments with appropriate typing.
157-162: Proper variable renaming to avoid conflicts.Renaming the SQL
wherevariable toqueryWhereavoids naming conflicts with the where clause from arguments, improving code clarity.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/MockContext/mockContextCreator.ts(1 hunks)test/MockContext/mocks/mockMinioClient.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (4)
test/MockContext/mockContextCreator.ts (1)
52-54: Consider enhancing the mock logger.As mentioned in previous reviews, you might want to enhance the mock logger to capture more detailed information during tests, which could help with debugging test failures.
test/MockContext/mocks/mockMinioClient.ts (3)
43-47: Consider adding more validation for method parameters.The
putObjectmethod doesn't validate its input parameters, unlikegetObject. For more robust testing, consider adding parameter validation.
47-59: Add mock for stream error handling.The implementation of
getObjectreturns a basic Readable stream with minimal functionality. Consider enhancing it to support error simulation.
25-66: Consider a more type-safe approach for the client mock.While the current implementation doesn't use explicit type assertions, it only implements a subset of the MinioClient interface methods. Consider either implementing more required methods or creating a more specific mock type.
|
@palisadoes
|
|
palisadoes
added
the
ignore-sensitive-files-pr
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
test/MockContext/mockContextCreator.ts (1)
1-67: 🧹 Nitpick (assertive)Add unit tests for the mock context factory.
This factory function is central to GraphQL resolver testing but lacks its own tests. Consider adding a dedicated test file to verify its behavior.
Would you like me to generate a unit test file for this factory function?
♻️ Duplicate comments (4)
test/MockContext/mockContextCreator.ts (2)
59-61: 🧹 Nitpick (assertive)Consider adding more logging capabilities to the mock logger.
The current mock logger implementation is basic. Consider enhancing it to capture and expose logs during tests, which would help with debugging test failures.
- log: createMockLogger(), + log: createMockLogger({ + captureOutput: true, + logLevel: 'debug' + }),
28-36: 🧹 Nitpick (assertive)Fix JSDoc formatting.
There's an extra line break between the JSDoc comment block and the function declaration at line 36. This breaks the visual connection between the documentation and the function.
* @param userId - The user ID (only for authenticated users). * @returns A fully mocked GraphQLContext object. */ - export function createMockGraphQLContext(test/MockContext/mocks/mockMinioClient.ts (2)
36-60: 🧹 Nitpick (assertive)Add JSDoc comments for individual mock methods.
Each mocked method would benefit from documentation explaining its behavior and any limitations.
For example:
// Mocks the necessary methods +/** + * Checks if a bucket exists + * @param bucketName - The name of the bucket to check + * @returns Promise<boolean> - True if the bucket name matches the configured bucket name + */ mockClient.bucketExists = vi.fn( async (bucketName: string) => bucketName === mockMinioConfig.bucketName, );
44-47: 🧹 Nitpick (assertive)Add parameter validation to putObject.
Unlike
getObject, theputObjectmethod doesn't validate its input parameters, which could hide issues in tests.- mockClient.putObject = vi.fn(async () => ({ - etag: "mock-etag", - versionId: null, - })); + mockClient.putObject = vi.fn(async (bucketName, objectName, stream) => { + if (bucketName !== mockMinioConfig.bucketName || !objectName || !stream) { + throw new Error("Invalid bucket name, object name, or stream"); + } + return { + etag: "mock-etag", + versionId: null, + }; + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/MockContext/mockContextCreator.ts(1 hunks)test/MockContext/mocks/mockMinioClient.ts(1 hunks)
🔇 Additional comments (2)
test/MockContext/mockContextCreator.ts (1)
37-66: Well-structured mock context factory!This implementation provides a comprehensive mock GraphQL context for testing resolvers. The function is flexible with its parameters and creates all the necessary components with appropriate defaults.
test/MockContext/mocks/mockMinioClient.ts (1)
48-59: Good implementation of getObject with proper validation and stream creation.The implementation correctly validates input parameters and creates a proper Readable stream with content. This approach handles the complexity of Node.js streams well.
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
docs/docs/docs/developer-resources/testing.md (1)
116-124: 🧹 Nitpick (assertive)Informative ‘Key Benefits’ and Future Considerations Section
The “Key Benefits” section, along with the subsequent discussion on future considerations, effectively communicates the advantages of centralizing the mock context and provides useful guidance for writing reliable tests. The content is thorough and aligns well with the PR objectives. To improve readability, please add blank lines around headings and list items per markdownlint guidelines.
**
🧰 Tools
🪛 LanguageTool
[typographical] ~122-~122: After an expression of time at the start of a sentence, a comma is usually used.
Context: ...h. ### Future Considerations In the future there might be a requirement to run som...(IN_THE_PAST_COMMA)
🪛 markdownlint-cli2 (0.17.2)
116-116: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/docs/docs/developer-resources/testing.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/docs/docs/developer-resources/testing.md (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
🪛 markdownlint-cli2 (0.17.2)
docs/docs/docs/developer-resources/testing.md
62-62: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
62-62: Multiple top-level headings in the same document
null
(MD025, single-title, single-h1)
65-65: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
70-70: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
71-71: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
76-76: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
76-76: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
77-77: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
79-79: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
82-82: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
83-83: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
85-85: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
90-90: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
107-107: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
108-108: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
116-116: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
docs/docs/docs/developer-resources/testing.md (1)
70-74: Clear Code Example for Importing the Mock ContextThe provided code snippet for importing
createMockGraphQLContextis straightforward and easy to follow. No functional issues here. Just be sure that the fenced code block is surrounded by blank lines (MD031) for consistency.**
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
70-70: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below(MD022, blanks-around-headings)
71-71: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
docs/docs/docs/developer-resources/testing.md (1)
70-76: 🧹 Nitpick (assertive)Spacing and Code Fence Formatting in the “Usage” Section
Both the “## Usage” heading and the “### Importing the Mock Context” subheading should have blank lines preceding and following them. Also, ensure that the fenced code block (lines 74–76) is surrounded by blank lines to satisfy MD031. These changes will make the section more readable and compliant with markdownlint guidelines.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/docs/docs/developer-resources/testing.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/docs/docs/developer-resources/testing.md (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
🪛 markdownlint-cli2 (0.17.2)
docs/docs/docs/developer-resources/testing.md
62-62: Multiple top-level headings in the same document
null
(MD025, single-title, single-h1)
78-78: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
79-79: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
docs/docs/docs/developer-resources/testing.md (1)
64-65: Directory Location NoteThe line indicating “In Directory
test/MockContext/*” clearly communicates where the factory function is defined. No action is needed here.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
test/graphql/types/Event/updater.test.ts (1)
1-2: 🧹 Nitpick (assertive)Sort import statements to fix potential pipeline failure.
The import statements should be sorted alphabetically to prevent pipeline failures.
-import { createMockGraphQLContext } from "test/MockContext/mockContextCreator"; -import { describe, expect, it, type vi } from "vitest"; +import { describe, expect, it, type vi } from "vitest"; +import { createMockGraphQLContext } from "test/MockContext/mockContextCreator";docs/docs/docs/developer-resources/testing.md (1)
70-76: 🛠️ Refactor suggestionImport statement has incorrect file path
The import example references an incorrect file path.
-import { createMockGraphQLContext } from "test/MockContext/mockGraphQLContext"; +import { createMockGraphQLContext } from "test/MockContext/mockContextCreator";test/graphql/types/Chat/updatedAt.test.ts (2)
39-43: 🧹 Nitpick (assertive)Consider using mockImplementation for better type safety.
According to the project's best practices and the retrieved learning from PR #3253, use
mockImplementation(() => Promise.resolve(undefined))instead ofmockResolvedValuewhen dealing with complex types to ensure type safety.( authenticatedContext.drizzleClient.query.usersTable .findFirst as ReturnType<typeof vi.fn> -).mockResolvedValue(undefined); +).mockImplementation(() => Promise.resolve(undefined));
1-5: 🧹 Nitpick (assertive)Import ordering should be fixed.
The import statements should be sorted to fix linting issues. The current order mixes imports from "vitest" with imports from project paths.
+import { beforeEach, describe, expect, it, vi } from "vitest"; import { createMockGraphQLContext } from "test/MockContext/mockContextCreator"; -import { beforeEach, describe, expect, it } from "vitest"; -import { vi } from "vitest"; import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError"; import { resolveUpdatedAt } from "../../../../src/graphql/types/Chat/updatedAt";test/MockContext/mocks/mockMinioClient.ts (1)
48-59: 🧹 Nitpick (assertive)Consider adding error simulation capability
For more robust testing, consider enhancing the
getObjectmethod to simulate errors in the stream.mockClient.getObject = vi.fn(async (bucketName, objectName) => { if (bucketName !== mockMinioConfig.bucketName || !objectName) { throw new Error("Object not found"); } + // Check if this is a request for a file that should trigger an error + if (objectName === "error-test-file") { + const errorStream = new Readable({ + read() { + this.emit("error", new Error("Simulated stream error")); + } + }); + return errorStream; + } const stream = new Readable({ read() { this.push("mock file content"); this.push(null); }, }); return stream; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (25)
docs/docs/docs/developer-resources/testing.md(1 hunks)schema.graphql(2 hunks)src/graphql/types/Query/allUsers.ts(5 hunks)src/utilities/defaultGraphQLConnection.ts(4 hunks)test/MockContext/mockContextCreator.ts(1 hunks)test/MockContext/mocks/drizzleClientMock.ts(1 hunks)test/MockContext/mocks/mockMinioClient.ts(1 hunks)test/MockContext/mocks/pubSubMock.ts(1 hunks)test/graphql/types/Advertisement/updatedAt.test.ts(2 hunks)test/graphql/types/Chat/updatedAt.test.ts(7 hunks)test/graphql/types/Community/updater.test.ts(8 hunks)test/graphql/types/Event/creator.test.ts(2 hunks)test/graphql/types/Event/updatedAt.test.ts(2 hunks)test/graphql/types/Event/updater.test.ts(10 hunks)test/graphql/types/Fund/creator.test.ts(2 hunks)test/graphql/types/Fund/updater.test.ts(10 hunks)test/graphql/types/FundCampaign/updatedAt.test.ts(2 hunks)test/graphql/types/FundCampaignPledge/updater.test.ts(10 hunks)test/graphql/types/Organization/creator.test.ts(2 hunks)test/graphql/types/Venue/updater.test.ts(2 hunks)test/graphql/types/tag/creator.test.ts(2 hunks)test/routes/graphql/Query/allUsers.test.ts(4 hunks)test/routes/graphql/documentNodes.ts(1 hunks)test/routes/graphql/gql.tada-cache.d.ts(1 hunks)test/routes/graphql/gql.tada.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
test/graphql/types/Event/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-02-23T16:10:02.480Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Fund/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/FundCampaignPledge/updater.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-02-23T16:10:02.480Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Venue/updater.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/tag/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/MockContext/mockContextCreator.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Advertisement/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/FundCampaign/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/updater.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-02-23T16:10:02.480Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Organization/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Chat/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-02-23T15:57:18.082Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
docs/docs/docs/developer-resources/testing.md (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
src/graphql/types/Query/allUsers.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: src/graphql/types/Query/allUsers.ts:159-161
Timestamp: 2025-03-05T07:11:49.613Z
Learning: The PR #3319 is focused on implementing a centralized mock context factory for GraphQL resolver unit tests, not on enhancing query filtering logic.
test/routes/graphql/Query/allUsers.test.ts (2)
Learnt from: rahulch07
PR: PalisadoesFoundation/talawa-api#3282
File: test/routes/graphql/Query/allUsers.test.ts:19-62
Timestamp: 2025-02-23T14:46:56.337Z
Learning: In test files, when an authentication token (like adminAuthToken) is used across multiple test cases, it's better to keep its setup and related user creation code together in the main test suite rather than splitting them across different suites.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: src/graphql/types/Query/allUsers.ts:159-161
Timestamp: 2025-03-05T07:11:49.613Z
Learning: The PR #3319 is focused on implementing a centralized mock context factory for GraphQL resolver unit tests, not on enhancing query filtering logic.
test/graphql/types/Fund/updater.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Community/updater.test.ts (2)
Learnt from: shivasankaran18
PR: PalisadoesFoundation/talawa-api#3175
File: test/graphql/types/Mutation/updateAgendaItem.test.ts:62-79
Timestamp: 2025-02-10T18:48:15.739Z
Learning: When mocking GraphQLContext in tests, keep the minio property with minimal implementation to satisfy TypeScript types, even if minio operations aren't used in the specific resolver being tested.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-02T16:59:17.866Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
🔇 Additional comments (64)
test/routes/graphql/gql.tada.d.ts (1)
159-159: The new input type aligns with the revised GraphQL schema.This addition of
QueryAllUsersWhereInputproperly implements a structured input type for filtering users, following GraphQL best practices by using specialized input objects instead of flat parameters.test/MockContext/mocks/pubSubMock.ts (1)
1-17: Well-implemented mock for PubSub with appropriate interfaces.The
createMockPubSubfunction creates a proper mock implementation of the PubSub interface with mockedpublishandsubscribemethods. The use ofSymbol.asyncIteratoris correctly implemented to simulate real event streams.This implementation is a good example of how to mock asynchronous event-based components for testing. The empty Readable stream with a no-op
readmethod correctly simulates the stream behavior without producing actual data.test/routes/graphql/documentNodes.ts (1)
246-295: GraphQL query updated to use structured filtering.The
Query_allUsershas been refactored to use the newwhereparameter instead of a flatnameparameter, improving query structure and allowing for more flexible filtering options.This change aligns with the introduction of the
QueryAllUsersWhereInputtype and follows GraphQL best practices for structured filtering.test/routes/graphql/Query/allUsers.test.ts (4)
307-307: Test updated to use structured filtering.The test parameter structure has been appropriately updated to match the new GraphQL schema, using a nested object for the
wherecondition instead of a flat parameter.
341-344: Consistently implemented structured filtering.The test for non-matching name search correctly implements the nested structure for the
whereparameter, maintaining consistency with the schema changes.
358-361: Last parameter test properly updated.The test for the
lastparameter with name filtering correctly implements the updated parameter structure, ensuring all test cases consistently use the new schema.
376-377: Empty name validation test correctly updated.The input validation test for minimum name length now properly uses the structured
whereparameter, maintaining consistency across all tests.test/graphql/types/Venue/updater.test.ts (3)
1-1: Good integration of centralized mock context!The import of
createMockGraphQLContextfrom the centralized mock context creator aligns with the stated PR objectives to standardize the approach to mock context creation.
21-21: Clean implementation of the mock context factoryThe replacement of a custom context creation with the centralized factory function properly provides authentication state and user ID parameters, simplifying the test setup.
44-45: Type assertions are correctly applied for mock implementationsThe use of
ReturnType<typeof vi.fn>type assertions before calling mock methods ensures proper typing while working with the centralized mock context, in line with the project's approach to handle drizzle client mocking.Also applies to: 63-64, 81-82, 99-100, 133-134, 152-154, 176-178
schema.graphql (2)
2870-2870: Good enhancement of query parameters with structured input typeReplacing the flat
nameparameter with a more structuredwhere: QueryAllUsersWhereInputparameter improves extensibility for future filtering options.
2955-2957: Well-structured input type for query filteringThe new
QueryAllUsersWhereInputtype provides a clean structure for query filtering that can be easily extended in the future with additional filtering criteria without breaking the API.test/graphql/types/Event/updatedAt.test.ts (2)
3-3: Appropriate type imports for the GraphQL contextThe import of the correct
GraphQLContexttype and thecreateMockGraphQLContextfunction ensures proper typing throughout the test file.Also applies to: 9-9
26-26: Effectively simplified mock context creationThe use of
createMockGraphQLContext(true, "user-123")provides a cleaner and more maintainable approach to creating test contexts with predetermined authentication state.test/graphql/types/FundCampaignPledge/updater.test.ts (4)
1-1: Well-implemented mock context initializationThe import and use of the centralized mock context creator simplifies the test setup and provides a consistent approach to mocking the GraphQL context across tests.
Also applies to: 6-8
37-39: Consider using mockImplementation for complex mock returnsType assertions with
ReturnType<typeof vi.fn>are correctly used for mocking.Also applies to: 47-59, 70-80, 98-108, 125-135, 152-159, 176-183, 196-203
209-226: Good use of mockImplementation with proper verificationThe implementation correctly tests that the query is called with the expected parameters by using a mock implementation that verifies the arguments. This approach to testing parameter passing is effective.
286-290: Proper type assertion for accessing mock call argumentsThe type assertion for accessing mock call arguments ensures proper typing when verifying the mock function call details.
test/MockContext/mockContextCreator.ts (4)
28-36: Well-documented utility function with clear usage examples.The JSDoc comments effectively document the function's purpose, parameters, and return value with a helpful example. This provides good guidance for developers using this centralized mock factory.
46-48: Type assertion approach is well-documented and centralized.The type assertion used here aligns with the approach discussed in the PR objectives - centralizing the type assertion in one place rather than creating a custom TestContext type that would need casting in every test. The explanatory comment clearly describes the reason for this design decision.
51-57: Flexible JWT mock implementation is a good improvement.The implementation allows for dynamic JWT token generation based on the payload, which is more flexible than a hardcoded token would be. This will be useful for tests that need to verify JWT-related functionality with different token values.
63-65: Consider adding unit tests for the mock context creator.This factory function is central to many tests, but there's no test for the factory itself. Adding a basic unit test would ensure it works as expected and serve as documentation for future developers.
#!/bin/bash # Check if there's a test for the mock context creator fd "mockContextCreator.*\.test\.(ts|js)" test/test/graphql/types/Organization/creator.test.ts (2)
1-4: Clean import simplification with centralized mock context.The imports have been simplified by removing the
CurrentClienttype import and adding the newcreateMockGraphQLContextimport. This reduces the dependencies and improves code readability.
23-23: Simplified context setup using the centralized factory.The context creation is now handled by the centralized factory function, which improves consistency across tests and makes the setup more maintainable. The authentication state and user ID are explicitly passed, making the test's intent clearer.
test/graphql/types/Event/updater.test.ts (2)
30-32: Well-structured context creation with clear intent.Creating separate authenticated and unauthenticated contexts at the top of the test file makes the test setup more explicit and easier to understand. This approach also allows for better reuse of the contexts across multiple test cases.
51-54: Consistent mock implementation approach.Using
mockImplementation()withPromise.resolve()instead ofmockResolvedValue()is consistent with the best practices for handling complex types in Vitest mocks, as noted in the retrieved learnings.test/graphql/types/Advertisement/updatedAt.test.ts (2)
1-4: Streamlined imports with centralized mock context.The imports have been simplified by removing the
CurrentClienttype import and adding the newcreateMockGraphQLContextimport, making the code more maintainable.
24-24: Simplified and standardized context creation.Using the centralized factory function improves consistency across tests and makes the setup more maintainable. The explicit parameters make it clear that this test uses an authenticated context with a specific user ID.
docs/docs/docs/developer-resources/testing.md (7)
62-66: Well-structured introduction to the Mock GraphQL Context FactoryThe new section provides a clear introduction to the centralized mock context solution. The directory location is specified correctly, which helps developers locate the implementation files.
66-69: Clear purpose statement that aligns with PR objectivesThe purpose section effectively communicates the value of the new
createMockGraphQLContextfunction, highlighting its role in providing fully mocked contexts without requiring real databases or services. This directly supports the PR's goal of centralizing mock context creation.
78-87: Well-documented unauthenticated context creation exampleThe example clearly demonstrates how to create a mock context for an unauthenticated user, with a helpful note about the resulting
isAuthenticatedproperty value.
88-95: Clear authenticated context creation exampleThe example for creating an authenticated context is concise and clearly identifies how the user ID is used in the mock context.
98-111: Comprehensive components tableThe table provides excellent details about each component in the mock context, making it clear what is being mocked and what functionality is available. This will be valuable for developers writing tests.
114-124: Practical example demonstrating test usageThe example shows a complete test case using the mock context, which helps developers understand how to integrate the new function into their tests. This aligns well with the PR goal of standardizing resolver test context creation.
128-132: Key benefits highlight the value propositionThe key benefits section effectively summarizes why this centralized approach is superior to individual mock contexts, emphasizing both isolation and scalability. This reinforces the PR's objective of having a single source of truth for mock contexts.
test/graphql/types/FundCampaign/updatedAt.test.ts (3)
1-5: Good implementation of centralized mock contextThe import changes correctly introduce the new
createMockGraphQLContextfunction while maintaining the necessary types for GraphQL context. This aligns with the PR objective of standardizing mock context creation.
26-28: Simplified context initialization with mock factoryThe context setup is now streamlined using the centralized mock factory, which makes the code more maintainable and consistent across tests.
38-43: Proper type assertion for mock implementationThe type assertion
as ReturnType<typeof vi.fn>ensures type safety while allowing for mocking. This approach is consistent with the learnings about centralizing type assertions for drizzle client mocks.test/graphql/types/Community/updater.test.ts (5)
1-8: Good adoption of centralized mock context patternThe imports have been updated to use the new mock context factory, which aligns with the project's standardization goals.
11-13: Type simplification using standard GraphQLContextChanged from a custom TestContext type to the standard GraphQLContext type, which reduces complexity and improves maintainability.
45-46: Streamlined context initialization with mock factoryThe context setup is significantly simplified, reducing boilerplate code while maintaining the necessary test environment.
84-86: Consistent type assertion for mock implementationThe type assertion approach used here ensures type safety for the mock functions, preventing potential runtime errors while maintaining good development experience.
171-174: Improved mock function typingThe proper type assertion for the
findFirstmock function ensures compatibility between the actual implementation and the test mock.test/graphql/types/tag/creator.test.ts (4)
1-7: Good adoption of centralized mock contextThe import changes correctly introduce the new mock context factory while maintaining the necessary GraphQL types. The explicit import of the
vitype also improves type safety.
23-25: Simplified context initializationThe
beforeEachsetup is now cleaner and more maintainable, using the standardized approach for creating mock contexts.
52-54: Consistent type assertion for mock functionsThe type assertion pattern applied consistently across the file ensures type safety for the mock implementations while centralizing the assertion pattern.
185-203: Test case demonstrates compatibility with centralized mock contextThis test case shows that the new mock context approach works seamlessly with existing test logic, maintaining the same behavior while simplifying the setup code.
test/graphql/types/Chat/updatedAt.test.ts (2)
20-22: Good implementation of centralized context factory.The use of
createMockGraphQLContextsimplifies test setup by providing consistent context objects across tests, reducing duplication and improving maintainability.
50-57: Type assertion is correctly applied to mock functions.The usage of
as ReturnType<typeof vi.fn>for the Drizzle client mock functions aligns with the centralized approach of type assertions mentioned in the retrieved learning from PR #3319.test/routes/graphql/gql.tada-cache.d.ts (1)
27-28: Well-structured GraphQL query definition with improved filtering.The updated query definition replaces the flat
nameparameter with a more flexiblewhereobject structure, enabling more complex filtering options. This follows best practices for GraphQL API design.src/utilities/defaultGraphQLConnection.ts (3)
24-36: Well-documented type for extending connection arguments with where filtering.This new type extends the base connection arguments to support filtering, maintaining a consistent pattern with the existing code while providing enhanced functionality.
140-153: Good implementation of schema builder for connection arguments with where clause.This helper function provides a clean way to extend the default connection arguments schema with custom where filtering, promoting code reuse and consistency.
155-184: Well-implemented transformation function for where-enabled connection arguments.The function cleanly separates the where clause handling from the base connection arguments transformation, maintaining the single responsibility principle while providing the needed functionality.
test/graphql/types/Fund/updater.test.ts (2)
1-7: Good adoption of centralized mock context factory.Using the
createMockGraphQLContextfunction aligns with the project-wide effort to standardize mock context creation, improving consistency across tests.
8-26: Simplified test setup with improved mock context.The changes reduce boilerplate by using the centralized mock context factory, maintaining the test's functionality while making it more maintainable.
test/graphql/types/Event/creator.test.ts (1)
1-1: Good refactoring to use the centralized mock contextThe changes effectively replace the custom context creation with the centralized factory function, aligning with the PR objectives.
Based on the retrieved learning, this refactoring centralizes the type assertion for the drizzle client in a single place rather than requiring it in each test file, which is a good design choice.
Also applies to: 4-4, 24-24
test/graphql/types/Fund/creator.test.ts (1)
1-1: Good refactoring to use the centralized mock contextThe changes effectively replace the custom context creation with the centralized factory function, aligning with the PR objectives. The formatting issue from previous reviews has been addressed.
Based on the retrieved learning, this refactoring centralizes the type assertion for the drizzle client in a single place rather than requiring it in each test file, which is a good design choice.
Also applies to: 4-4, 22-22
src/graphql/types/Query/allUsers.ts (7)
19-22: Good use of strongly typed imports
The imports forcreateGraphQLConnectionWithWhereSchema,transformGraphQLConnectionArgumentsWithWhere, and related types provide clear structure for the new "where" handling logic.
26-32: Schema definition is consistent with query filtering goals
Defining the userWhereSchema with.optional()at the top level is appropriate. It allows fully omitting thewhereargument, yet ensures valid constraints forname.
33-69: Comprehensive connection argument transformation
The transform logic correctly parses and validates cursors, applying default values towherewhere needed. This approach keeps the code clear and consistent with the rest of the GraphQL argument handling.
85-92: Alignment between the Zod schema and the GraphQL builder
Exposing thewhereargument as an optional input type closely matches theuserWhereSchema. This maintains consistency between the GraphQL schema definition and the underlying validation.
147-151: Clean type inference for parsed arguments
CastingparsedArgsto the correctParsedDefaultGraphQLConnectionArgumentsWithWheretype guarantees type-safe usage in downstream logic. This is a neat application of the generics-based approach.
159-161: Logical name-based filtering
Usingilike(usersTable.name, …)cleanly enacts case-insensitive partial matching. This is straightforward and readable while fully leveraging Drizzle’s capabilities.
221-221: Consistent final query application
AssigningqueryWhereto thewhereproperty in the Drizzle query unifies all filtering logic and cursor checks. It cleanly rounds out the flow established earlier in the function.
|
Please fix the conflicting files |
Piyushk8
force-pushed
the
mockFactoryFunction
branch
from
b212afb to
bf2e657
Compare
|
@disha1202 @meetulr kindly Review ! please take this into account that full review might as well raise issues as i have implemented this on all VALID unit tests without changing their underlying logic so, it may raise issues in them which was not part of this PRs motive but all tests were thoroughly tested and passed with no changes in coverage. thanks! |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
test/_Mocks_/pubsubMock.ts (1)
11-11: Consider adding test helpers for stream testingWhile the empty Readable stream is appropriate for basic testing, you might want to add a helper method to allow tests to trigger events for more advanced test scenarios.
export function createMockPubSub(): PubSub { + const streams = new Map<string, Readable>(); + return { publish: vi.fn(), subscribe: vi.fn().mockImplementation((channel) => { + const stream = Object.assign( new Readable({ read() {}, // Empty Readable Stream (mocks real event streams) }), { [Symbol.asyncIterator]: async function* () {} }, ); + streams.set(channel, stream); + return Promise.resolve(stream); }), + // Helper method for tests to push data to streams + __pushEvent: (channel: string, data: unknown) => { + const stream = streams.get(channel); + if (stream) { + stream.push(JSON.stringify(data)); + return true; + } + return false; + } }; }test/_Mocks_/mockContextCreator/mockContextCreator.ts (1)
35-36: Consider adding options parameter for more flexibilityTo make the factory even more versatile, consider adding an options object parameter that allows overriding default behaviors for specific tests.
export function createMockGraphQLContext( isAuthenticated = true, userId = "user123", + options?: { + envConfig?: Partial<ExplicitGraphQLContext['envConfig']>; + mockImplementations?: { + jwtSign?: (payload: any) => string; + // Add other mock implementations as needed + }; + } ) { // ... const mockJwtSign = vi .fn() - .mockImplementation( - (payload) => `mocked.jwt.${JSON.stringify(payload)}.token`, - ); + .mockImplementation( + options?.mockImplementations?.jwtSign || + ((payload) => `mocked.jwt.${JSON.stringify(payload)}.token`) + ); // ... const explicitContext: ExplicitGraphQLContext = { // ... - envConfig: { API_BASE_URL: "http://localhost:4000" }, + envConfig: { API_BASE_URL: "http://localhost:4000", ...options?.envConfig }, // ... }; // ... }test/_Mocks_/drizzleClientMock.ts (1)
19-29: Add custom data support to table method mocksWhile the default implementations work for basic cases, consider enhancing the
createTableMethodsfunction to allow specifying default data to return.- function createTableMethods<T>(): TableMethods<T> { + function createTableMethods<T>(defaultData?: { + single?: T; + many?: T[]; + count?: number; + }): TableMethods<T> { return { - findFirst: vi.fn(() => Promise.resolve(undefined)), - findMany: vi.fn(() => Promise.resolve([])), + findFirst: vi.fn(() => Promise.resolve(defaultData?.single)), + findMany: vi.fn(() => Promise.resolve(defaultData?.many || [])), insert: vi.fn(), update: vi.fn(), delete: vi.fn(), - count: vi.fn(() => Promise.resolve(0)), + count: vi.fn(() => Promise.resolve(defaultData?.count || 0)), }; } // Then update the createMockDrizzleClient function to accept defaults: export function createMockDrizzleClient(defaultData?: Record<string, { single?: Record<string, unknown>; many?: Record<string, unknown>[]; count?: number; }>) { const queryTables = Object.keys(drizzleSchema).reduce<QueryTables>( (acc, tableName) => { - acc[tableName as keyof typeof drizzleSchema] = createTableMethods(); + acc[tableName as keyof typeof drizzleSchema] = createTableMethods( + defaultData?.[tableName] + ); return acc; }, {} as QueryTables, ); // ... }test/_Mocks_/mockMinioClient.ts (2)
48-59: Enhance getObject mock for more flexible testingThe current implementation always returns the same content. Consider enhancing it to allow tests to specify different content for different object names.
export const createMockMinioClient = ( + fileContents: Record<string, string> = {} ): MockMinioClient => { // ... mockClient.getObject = vi.fn(async (bucketName, objectName) => { if (bucketName !== mockMinioConfig.bucketName || !objectName) { throw new Error("Object not found"); } const stream = new Readable({ read() { - this.push("mock file content"); + this.push(fileContents[objectName] || "mock file content"); this.push(null); }, }); return stream; }); // ... };
8-12: Consider making bucket name configurableThe bucket name is currently hardcoded to "talawa". Consider making this configurable to support more test scenarios.
- const mockMinioConfig = { + const createMockMinioConfig = (overrides?: { + bucketName?: string; + endPoint?: string; + port?: number; + }) => ({ endPoint: "localhost", port: 9000, - bucketName: "talawa" as const, + bucketName: (overrides?.bucketName || "talawa") as string, - }; + }); export const createMockMinioClient = ( fileContents: Record<string, string> = {}, + configOverrides?: Parameters<typeof createMockMinioConfig>[0] ): MockMinioClient => { + const mockMinioConfig = createMockMinioConfig(configOverrides); // ... rest of implementation using mockMinioConfig };docs/docs/docs/developer-resources/testing.md (1)
74-76: Fix the import path in documentationThe import path should be
test/_Mocks_/mockContextCreator/mockContextCreatorto match the actual implementation seen in the test files.-import { createMockGraphQLContext } from "test/MockContext/mockContextCreator"; +import { createMockGraphQLContext } from "test/_Mocks_/mockContextCreator/mockContextCreator";test/graphql/types/Event/creator.test.ts (3)
163-169: Consider simplifying the mock function assignment.The current code assigns the mock function reference across multiple lines with extra whitespace. Consider simplifying this for consistency with other test files.
- const findFirst = - mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue( - mockCurrentUser, - ); - + const findFirst = mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue( + mockCurrentUser, + );
190-195: Consider simplifying this mock function assignment too.Similar to the previous comment, this code can be simplified for better readability and consistency.
- const findFirst = - mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue( - mockCurrentUser, - ); + const findFirst = mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue( + mockCurrentUser, + );
313-316: Remove unnecessary empty lines.There are extra empty lines here that can be removed for code cleanliness.
- - mocks.drizzleClient.query.usersTable.findFirst + mocks.drizzleClient.query.usersTable.findFirst
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/docs/docs/developer-resources/testing.md(1 hunks)test/_Mocks_/drizzleClientMock.ts(1 hunks)test/_Mocks_/mockContextCreator/mockContextCreator.ts(1 hunks)test/_Mocks_/mockMinioClient.ts(1 hunks)test/_Mocks_/pubsubMock.ts(1 hunks)test/graphql/types/Advertisement/updatedAt.test.ts(12 hunks)test/graphql/types/Chat/updatedAt.test.ts(7 hunks)test/graphql/types/Chat/updater.test.ts(9 hunks)test/graphql/types/Event/creator.test.ts(17 hunks)test/graphql/types/Event/updatedAt.test.ts(11 hunks)test/graphql/types/Event/updater.test.ts(11 hunks)test/graphql/types/Fund/creator.test.ts(14 hunks)test/graphql/types/FundCampaign/updatedAt.test.ts(9 hunks)test/graphql/types/Organization/creator.test.ts(16 hunks)test/graphql/types/Post/updater.test.ts(11 hunks)test/graphql/types/Tag/creator.test.ts(18 hunks)
🧰 Additional context used
🧠 Learnings (13)
test/_Mocks_/mockContextCreator/mockContextCreator.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Tag/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/updater.test.ts (3)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Post/updater.test.ts (3)
Learnt from: PratapRathi
PR: PalisadoesFoundation/talawa-api#3310
File: test/graphql/types/Post/updater.test.ts:57-73
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the talawa-api project, the preferred approach for test data organization is to use shared mock objects initialized in beforeEach blocks to prevent code duplication, with specific test cases creating their own mock data only when necessary for that particular test scenario.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
docs/docs/docs/developer-resources/testing.md (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/updatedAt.test.ts (3)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Advertisement/updatedAt.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Chat/updater.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Chat/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Fund/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Organization/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/FundCampaign/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
🔇 Additional comments (41)
test/_Mocks_/pubsubMock.ts (1)
5-17: Well-implemented PubSub mock!The mock implementation correctly implements the PubSub interface with both required methods. The
publishmethod is appropriately mocked for tracking calls, and thesubscribemethod returns a proper Readable stream with an async iterator, simulating the expected behavior.test/_Mocks_/mockContextCreator/mockContextCreator.ts (3)
34-82: Excellent centralized mock context implementation!This factory function effectively centralizes the creation of GraphQL contexts for tests, which aligns perfectly with the PR objectives. The approach of returning both the context and exposed mocks makes it easy for tests to both use the context and verify interactions.
54-54: Type assertion is appropriate hereThe type assertion
mockDrizzleClient as unknown as FastifyInstance["drizzleClient"]is a good approach for handling the mock Drizzle client. As noted in the retrieved learnings, this centralizes the type assertion in one place rather than requiring it in every test.
73-81: Great pattern for exposing mock instancesReturning both the context and the mocks as separate properties is an excellent pattern that allows tests to both use the context and verify interactions with the mocked components.
test/_Mocks_/drizzleClientMock.ts (2)
4-12: Well-defined TableMethods interfaceThe TableMethods interface properly captures the essential database operations that tests will need to mock. Using the Mock type from vitest ensures that returned values maintain proper typing.
31-43: Good implementation of createMockDrizzleClientThe function correctly builds a mock drizzle client that matches the expected structure with table methods for each table in the schema.
test/_Mocks_/mockMinioClient.ts (1)
26-67: Comprehensive MinIO client mockThe mock implementation covers all the essential MinIO operations and provides reasonable default behaviors. The error handling in
getObjectis particularly good, simulating the real behavior when bucket or object names don't match.test/graphql/types/Organization/creator.test.ts (4)
1-1: Good use of centralized mock context factoryThe import of
createMockGraphQLContextfrom the correct path is a good implementation of the centralized approach outlined in the PR objectives.
19-28: Clean type definitions for context and mocksGood use of TypeScript's
ReturnTypeutility to correctly type the context and mocks. This approach ensures type safety while keeping the code DRY.
22-36: Simplified context initializationThe context setup looks clean and concise. Using the structured approach with destructuring to get both context and mocks makes the code more readable.
65-67: Consistent mock usageCorrectly using the mocks object from the centralized factory to mock the drizzle client's behavior. This is more maintainable than creating mock implementations directly in each test file.
docs/docs/docs/developer-resources/testing.md (3)
62-166: Well-structured and comprehensive documentationThe new section on Mock GraphQL Context Factory is well-organized and provides clear guidance on its purpose, usage, and benefits. The examples are practical and the tables explaining components and return values are informative.
80-95: User authentication examples are clear and usefulThe examples for creating contexts for both authenticated and unauthenticated users are well-structured and demonstrate the different usage patterns effectively.
171-180: Good explanation of simplified call signatureThe documentation clearly explains both the legacy style and the new object-based style for creating mock contexts, which helps with migration and understanding the API.
test/graphql/types/FundCampaign/updatedAt.test.ts (3)
1-3: Proper imports for mock context factoryThe import statements are correctly updated to use the centralized mock context factory, which aligns with the PR objectives.
26-33: Clean context setup in beforeEachThe context setup in the beforeEach block is concise and clearly separates the context from the mocks, making it easy to understand how the test environment is being prepared.
43-43: Consistent mock usage across testsGood use of the mock drizzle client for simulating database responses, which is cleaner than creating custom mock implementations in each test file.
test/graphql/types/Chat/updatedAt.test.ts (3)
1-5: Good import structureThe imports are well-organized, with the mock context creator imported first, followed by testing utilities and the component under test.
19-36: Clear separation of authenticated and unauthenticated contextsGood approach to creating both authenticated and unauthenticated contexts for testing different scenarios. The type definitions using ReturnType ensure type safety.
49-49: Consider using mockImplementation for better type safetyAccording to the project's best practices (from the retrieved learnings), consider using
mockImplementation(() => Promise.resolve(undefined))instead ofmockResolvedValuewhen dealing with complex types to ensure type safety.-mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue(undefined); +mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => Promise.resolve(undefined));test/graphql/types/Post/updater.test.ts (3)
1-1: Great implementation of the centralized mock context factory!The import and setup of the
createMockGraphQLContextfunction are well implemented, making the test setup more consistent and maintainable.Also applies to: 10-11, 14-19
35-40: Good use of the factory for unauthenticated testingThe test correctly uses
createMockGraphQLContext(false)to generate an unauthenticated context, which makes the test more readable and maintainable.
48-48: Consistent use of the mocks object throughout the test suiteThe test cases have been properly updated to use
mocks.drizzleClientfor all database queries, maintaining consistency throughout the test file and separating the mocking concerns from the context object itself.Also applies to: 65-67, 83-85, 101-103, 121-123, 135-137, 156-157, 170-172, 191-192
test/graphql/types/Chat/updater.test.ts (2)
53-69: Well-structured mock context setupThe implementation correctly defines typed variables for the context and mocks, and initializes them in the beforeEach block using the new factory function. This makes the setup more maintainable and consistent with other tests.
144-145: Good consistency in user IDsThe updaterId has been properly updated to match the user ID used in the context setup, ensuring consistency between the test context and the test data.
test/graphql/types/Event/updater.test.ts (2)
29-45: Well-structured mock context setupThe implementation correctly defines typed variables for both authenticated and unauthenticated contexts, and initializes them using the new factory function. This makes the setup more maintainable and consistent with other tests.
77-78: Good consistency in user IDsThe user IDs have been properly updated across test cases to match the user ID used in the context setup, ensuring consistency between the test context and the test data.
Also applies to: 97-98, 117-119, 126-127
test/graphql/types/Event/updatedAt.test.ts (3)
1-8: Well-implemented changes to use the centralized mock contextThe imports and setup correctly implement the new
createMockGraphQLContextfactory function, eliminating the need for theCurrentClienttype import and simplifying the test setup.Also applies to: 22-31
53-57: Consistent update to use the mocks objectAll mock implementations for user queries have been properly updated to use the
mocks.drizzleClient.query.usersTable.findFirstmethod, maintaining the original test behavior while following the new centralized approach.Also applies to: 71-73, 89-91, 107-109, 130-132, 148-150, 159-161, 178-180, 195-197, 211-213
215-219: Good implementation of error handling test caseThe mock implementation for the error handling test case has been properly updated to use the mocks object, maintaining the original test behavior while following the new centralized approach.
test/graphql/types/Advertisement/updatedAt.test.ts (3)
1-3: LGTM! Import changes look good.The import changes correctly replace the custom mock context with the centralized factory and remove the unnecessary CurrentClient type.
19-28: Structure of mock context initialization is clean and consistent.The new approach with destructuring the result of
createMockGraphQLContextinto context and mocks is well-structured. This pattern properly separates the context object from the mock implementations.
49-60: References to mock implementations updated correctly.All references to
ctx.drizzleClient.query.usersTable.findFirsthave been correctly updated to use the mocks object, maintaining the test functionality while adopting the new pattern.test/graphql/types/Tag/creator.test.ts (3)
1-3: Import changes align with the centralized context factory approach.The changes correctly import the new mock context creator while removing the unnecessary CurrentClient type.
20-29: Context initialization follows the standardized pattern.The refactoring properly initializes the context and mocks variables using the destructured result from
createMockGraphQLContext, which is consistent with the approach used in other test files.
133-135: Clean extraction of mock function reference.The code cleanly extracts the mock function for repeated use rather than accessing it repeatedly, which is a good practice.
test/graphql/types/Fund/creator.test.ts (3)
1-8: Import and spacing look good.The import for the centralized mock context creator is properly added, and the unnecessary CurrentClient import is removed.
22-28: Context initialization formatting is correct.The context initialization is properly formatted with appropriate spacing between parameters, addressing a previously noted formatting issue.
176-179: Mock spy assignment is correctly updated.The reference to the mock function is updated to use the new mocks object structure.
test/graphql/types/Event/creator.test.ts (2)
23-29: Context initialization follows the standardized pattern.The refactored code correctly initializes the context and mocks using the new centralized factory function.
93-108: Note about commented-out test code.This commented-out test case references the old mocking style (
ReturnType<typeof vi.fn>). While it's not affecting anything since it's commented out, it might be worth revisiting this test case later to update it to the new pattern if you decide to use it.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/docs/docs/developer-resources/testing.md (2)
75-76: Import path in documentation does not match actual implementationThe import path in this example is inconsistent with how it's used in the actual test files.
-import { createMockGraphQLContext } from "test/_Mocks_/mockContextCreator"; +import { createMockGraphQLContext } from "test/_Mocks_/mockContextCreator/mockContextCreator";
182-188: Duplicate "Future Considerations" sectionThe "Future Considerations" section (lines 182-184) is duplicated with an identical section at lines 186-188.
Consider removing one of these duplicate sections to maintain clean documentation.
🧰 Tools
🪛 LanguageTool
[typographical] ~188-~188: After an expression of time at the start of a sentence, a comma is usually used.
Context: ...dea. ### Future Considerations In the future there might be a requirement to run som...(IN_THE_PAST_COMMA)
[uncategorized] ~188-~188: A comma might be missing here.
Context: ...me tests sequentially. When that moment arrives separating sequential and parallel test...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
test/graphql/types/Event/updater.test.ts (2)
15-26: Inconsistent user ID format in MockEvent objectThe MockEvent object uses IDs with underscores (
user_1,updater_1), while the tests use hyphenated IDs (user-123). This inconsistency could cause confusion.const MockEvent = { createdAt: new Date(), - creatorId: "user_1", + creatorId: "user-123", description: "chat description", endAt: new Date(), id: "event_1", name: "fo", organizationId: "organization_1", startAt: new Date(), updatedAt: new Date(), - updaterId: "updater_1", + updaterId: "another-user-id", };
200-205: Inconsistent user ID in test caseThis test case still uses the old format for the current user ID (
user_1) while previous tests have been updated to use the hyphenated format (user-123).const mockCurrentUser: UserObject = { - id: "user_1", + id: "user-123", role: "administrator", organizationMembershipsWhereMember: [{ role: "administrator" }], };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/docs/docs/developer-resources/testing.md(1 hunks)test/graphql/types/Chat/updater.test.ts(9 hunks)test/graphql/types/Event/updatedAt.test.ts(11 hunks)test/graphql/types/Event/updater.test.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/graphql/types/Chat/updater.test.ts
🧰 Additional context used
🧠 Learnings (3)
test/graphql/types/Event/updater.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
docs/docs/docs/developer-resources/testing.md (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/updatedAt.test.ts (3)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (5)
docs/docs/docs/developer-resources/testing.md (1)
62-166: Well-structured and comprehensive documentation for the new mock context factoryThe new documentation section effectively explains the purpose, usage, and benefits of the mock GraphQL context factory. It provides clear guidance for developers on how to implement testing with the new centralized approach.
test/graphql/types/Event/updater.test.ts (2)
1-45: Good implementation of the centralized mock context approachThe test setup has been properly refactored to use the new
createMockGraphQLContextfunction, which improves code reusability and consistency across tests. ThebeforeEachhook effectively sets up both authenticated and unauthenticated contexts for testing.
126-127: Updated updaterId to match test contextThe event's updaterId has been properly updated to match the authenticated user's ID in this test case, which helps maintain consistency.
test/graphql/types/Event/updatedAt.test.ts (2)
7-31: Proper implementation of the centralized mock contextThe test has been successfully updated to use the new
createMockGraphQLContextfunction, providing a consistent approach to mocking across the test suite.
53-62: Improved error handling test with clear mock implementationThe test for handling unauthenticated errors when the user doesn't exist is well structured and uses the mock approach correctly.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
test/graphql/types/Event/updater.test.ts (1)
200-205:⚠️ Potential issueInconsistent user ID format in mock data
There's an inconsistency in the user ID format. The new mock context is created with "user-123", but this test case uses "user_1", which could lead to test failures. Update the ID to match the format used elsewhere.
const mockCurrentUser: UserObject = { - id: "user_1", + id: "user-123", role: "administrator", organizationMembershipsWhereMember: [{ role: "administrator" }], };
🧹 Nitpick comments (8)
test/_Mocks_/mockContextCreator/mockContextCreator.ts (1)
72-82: Consider adding TypeScript return type annotationThe function implementation is correct, but explicitly defining the return type would improve the clarity and type safety.
export function createMockGraphQLContext( isAuthenticated = true, userId = "user123", -) { +): { + context: GraphQLContext; + mocks: { + drizzleClient: ReturnType<typeof createMockDrizzleClient>; + minioClient: ReturnType<typeof createMockMinioClient>; + pubsub: ReturnType<typeof createMockPubSub>; + jwtSign: MockInstance; + }; +} {test/graphql/types/Chat/updater.test.ts (1)
180-180: Verify logger interaction through mocksThis test correctly verifies that the error was logged, but now that the logger is part of the context, consider verifying it through the mocks object for consistency with other mock verifications.
-expect(authenticatedContext.log.error).toHaveBeenCalled(); +expect(authenticatedContext.log.error).toHaveBeenCalled(); // Or use mocks.logger if exposed in the futuretest/graphql/types/Post/updater.test.ts (2)
48-48: Consider using mockImplementation for better type safety.According to the project's best practices, use
mockImplementation(() => Promise.resolve(undefined))instead ofmockResolvedValuewhen dealing with complex types to ensure type safety.-mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue(undefined); +mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => Promise.resolve(undefined));
65-67: Consider using mockImplementation for better type safety across all mocks.For consistency and better type safety, consider using
mockImplementation()instead ofmockResolvedValueOnce()throughout the file. This is consistent with the project's preferred approach for mocking.-mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValueOnce( - mockUser, -); +mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => + Promise.resolve(mockUser) +);Make similar changes for all instances of
mockResolvedValueOnce()in the file.Also applies to: 83-85, 101-103, 121-123, 135-137, 156-158, 170-172
test/graphql/types/Event/updater.test.ts (1)
62-64: Consider using mockImplementation for better type safety.-mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => - Promise.resolve(undefined), -); +mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => + Promise.resolve(undefined) +);This is a stylistic suggestion for consistency and code style, not a functional issue.
test/graphql/types/Event/updatedAt.test.ts (2)
53-53: Fix typo in test descriptionThere's a typo in the test description: "is userId exists" should be "if userId exists".
-it("should throw unauthenticated error is userId exists but current user doesn't exists", async () => { +it("should throw unauthenticated error if userId exists but current user doesn't exist", async () => {
55-57: Consider using mockImplementation for better type safety across all mocks.For consistency and better type safety, consider using
mockImplementation()instead ofmockResolvedValue()/mockRejectedValue()throughout the file. This is consistent with the project's preferred approach for mocking.-mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue( - currentUser, -); +mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => + Promise.resolve(currentUser) +);Make similar changes for all instances in the file.
Also applies to: 71-73, 89-91, 107-109, 130-132, 148-150, 159-161, 178-180, 195-197
test/graphql/types/Event/creator.test.ts (1)
94-108: Consider removing or enabling commented test case.There's a commented test case that might be causing confusion. Consider either removing it completely or enabling it with the new mock context pattern if it's meant to be part of the test suite.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/docs/docs/developer-resources/testing.md(1 hunks)test/_Mocks_/drizzleClientMock.ts(1 hunks)test/_Mocks_/mockContextCreator/mockContextCreator.ts(1 hunks)test/_Mocks_/mockMinioClient.ts(1 hunks)test/_Mocks_/pubsubMock.ts(1 hunks)test/graphql/types/Advertisement/updatedAt.test.ts(12 hunks)test/graphql/types/Chat/updatedAt.test.ts(7 hunks)test/graphql/types/Chat/updater.test.ts(9 hunks)test/graphql/types/Event/creator.test.ts(17 hunks)test/graphql/types/Event/updatedAt.test.ts(11 hunks)test/graphql/types/Event/updater.test.ts(11 hunks)test/graphql/types/Fund/creator.test.ts(14 hunks)test/graphql/types/FundCampaign/updatedAt.test.ts(9 hunks)test/graphql/types/Organization/creator.test.ts(16 hunks)test/graphql/types/Post/updater.test.ts(11 hunks)test/graphql/types/Tag/creator.test.ts(18 hunks)
🧰 Additional context used
🧠 Learnings (13)
test/graphql/types/FundCampaign/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Organization/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Chat/updater.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/updatedAt.test.ts (3)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/_Mocks_/mockContextCreator/mockContextCreator.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Tag/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Chat/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T13:38:42.175Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/updater.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Fund/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Advertisement/updatedAt.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
docs/docs/docs/developer-resources/testing.md (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Post/updater.test.ts (3)
Learnt from: PratapRathi
PR: PalisadoesFoundation/talawa-api#3310
File: test/graphql/types/Post/updater.test.ts:57-73
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the talawa-api project, the preferred approach for test data organization is to use shared mock objects initialized in beforeEach blocks to prevent code duplication, with specific test cases creating their own mock data only when necessary for that particular test scenario.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
🔇 Additional comments (57)
test/_Mocks_/pubsubMock.ts (1)
1-17: Well-structured mock for PubSub interfaceThe implementation creates a properly typed mock that follows the PubSub interface contract, allowing for tracking method calls during tests. Good use of the Readable stream to simulate real event streams and the Symbol.asyncIterator implementation for async iteration support.
This mock will be useful for testing components that use PubSub without needing real event streaming.
test/_Mocks_/mockContextCreator/mockContextCreator.ts (2)
13-26: Good approach for client mocks with clear documentationThe client mocks are well-defined with JSDoc comments clearly describing their purpose. The separation between unauthenticated and authenticated clients provides flexibility for different test scenarios.
28-71: Excellent implementation of context factory with exposed mocksThe implementation successfully centralizes the creation of GraphQL context for tests, which aligns with the PR objectives. Exposing both the context and individual mocks through the return value is particularly useful for testing - it allows tests to both use the context and verify interactions with specific mock instances.
The type assertion for drizzleClient follows the pattern identified in the retrieved learnings, centralizing it in one place rather than requiring casts in every test.
test/graphql/types/Chat/updater.test.ts (5)
1-1: Good integration of centralized mock contextThe import of the centralized mock context creator aligns with the PR objectives to standardize the approach to mock context creation.
52-52: Fix the inconsistent describe titleThe describe title has been changed to "Chat.updatedAt resolver" but this file is testing the
resolveUpdaterfunction. The title should reflect the actual resolver being tested.-describe("Chat.updater resolver", () => { +describe("Chat.updater resolver", () => {
53-69: Clean refactoring of context setupThe context setup has been refactored to use the centralized mock context factory, which simplifies the setup code and makes it more consistent with other tests. The type definitions using ReturnType ensure type safety.
82-83: Proper use of mock implementationThe refactoring correctly uses the mock drizzle client from the centralized context. This maintains the test's behavior while using the new structure.
144-144: User ID consistency with context setupThe updaterId has been updated to match the userId used in the context setup ("user-123"), which ensures test consistency.
test/_Mocks_/drizzleClientMock.ts (2)
1-29: Well-designed mock structure with default implementationsThe implementation defines a comprehensive set of table methods with appropriate defaults, which will make testing easier. Good use of TypeScript to ensure type safety with the generic
TableMethodstype.
31-43: Effective dynamic creation of mock client based on schemaThe
createMockDrizzleClientfunction efficiently generates mock methods for all tables in the schema, ensuring that the mock structure matches the real client. This approach makes the mock both comprehensive and maintainable as the schema evolves.test/_Mocks_/mockMinioClient.ts (3)
8-12: Well-defined mock configuration with appropriate type safetyThe
mockMinioConfigobject is well-structured with proper type safety using theas constassertion for the bucket name, which ensures type consistency throughout the mock implementation.
17-21: Good type definition with clear property typesThe
MockMinioClienttype properly defines the structure of the mock client, with appropriate references to the previously defined config. This ensures type safety when working with the mock client.
26-67: Well-implemented mock MinIO client with comprehensive method coverageThe
createMockMinioClientfunction provides a complete mock implementation with all necessary methods required for testing. The mock includes:
- Proper initialization with mock credentials
- Mocked method implementations using Vitest's
vi.fn()- Realistic error handling for
getObjectmethod- Simulated file content stream for testing file operations
- Clean return structure with all required properties
This implementation will help maintain consistency across tests involving MinIO operations.
test/graphql/types/Organization/creator.test.ts (4)
1-4: Improved imports with centralized mock context factoryGood replacement of the custom mock context with a centralized mock context factory. This helps standardize the testing approach across the codebase.
19-20: Improved type declaration with direct access to mocksThe explicit typing for
mocksusingReturnType<typeof createMockGraphQLContext>["mocks"]provides better type safety and IDE assistance when working with the mock objects.
23-28: Streamlined context creation using the centralized factoryThe implementation now uses the centralized mock context factory, which aligns with the PR objectives to standardize the approach to mock context creation. This reduces code duplication and improves maintainability.
65-67: Consistent mock implementation using the new context structureAll mock implementations have been updated to use the new mocks structure provided by
createMockGraphQLContext. This ensures a consistent approach to mocking throughout the test file.Also applies to: 85-87, 103-105, 124-126, 148-150, 183-185, 225-227, 267-269, 280-280, 313-313, 345-349
docs/docs/docs/developer-resources/testing.md (10)
62-66: Clear documentation of the mock context factory location and purposeThe documentation clearly identifies where to find the mock context factory and its purpose. The section is properly formatted with appropriate headings.
68-69: Well-explained purpose with key benefits highlightedThe purpose section effectively explains the benefits of using a centralized mock context, emphasizing testing without real dependencies. This helps developers understand why they should use this approach.
72-76: Correct import path shown for the mock context factoryThe documentation provides the correct import path for the mock context factory, making it easy for developers to adopt this approach in their tests.
80-87: Clear example for creating an unauthenticated mock contextThe example for creating an unauthenticated mock context is well-structured and includes a helpful note about the
currentClient.isAuthenticatedflag.
88-98: Clear example for creating an authenticated mock contextThe example for creating an authenticated mock context is well-structured and shows how to set a user ID, which is a common requirement in resolver tests.
101-114: Comprehensive components table with clear descriptionsThe components table provides a thorough overview of all the mock components available in the context, with clear descriptions of each component's purpose. This helps developers understand what's available for mocking in their tests.
117-125: Well-documented return value structureThe return value section clearly explains the two properties returned by the factory function, helping developers understand how to access both the context and the individual mocks.
128-157: Practical example for testing resolvers with mock verificationThe example for testing resolvers is comprehensive and shows best practices:
- Creating the context with proper authentication
- Configuring mock behavior as needed
- Calling the resolver with the mock context
- Verifying both the results and the interactions with dependencies
This provides an excellent template for developers to follow.
161-166: Clear explanation of key benefitsThe key benefits section effectively summarizes why developers should use this approach, highlighting the exposed mocks, type safety, and scalability benefits.
169-180: Helpful documentation of both legacy and new parameter stylesThe documentation of both parameter styles (legacy and object-based) is helpful for maintaining backwards compatibility while encouraging the new approach. The code examples make the differences clear.
test/graphql/types/FundCampaign/updatedAt.test.ts (3)
1-3: Improved imports with centralized mock context factoryThe imports have been updated to use the centralized mock context factory, removing the unnecessary
CurrentClienttype import. This aligns with the PR objectives to standardize mock context creation.
24-33: Streamlined context setup with the standard factory functionThe context initialization now uses the centralized mock context factory, providing a consistent approach across test files. This makes the test more maintainable and aligned with other tests in the codebase.
43-43: Consistent mock usage across all test casesAll test cases have been updated to use the standardized
mocksstructure provided by the mock context factory. This ensures a consistent approach to mocking throughout the test file and improves maintainability.Also applies to: 50-53, 69-70, 85-86, 100-101, 107-109, 117-119, 127-129, 137-139, 147-149, 158-160, 173-175, 191-192
test/graphql/types/Chat/updatedAt.test.ts (4)
49-49: Consider using mockImplementation for better type safety.According to the project's best practices, use
mockImplementation(() => Promise.resolve(undefined))instead ofmockResolvedValuewhen dealing with complex types to ensure type safety.-mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue(undefined); +mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => Promise.resolve(undefined));
1-4: Sort import statements to fix potential pipeline failure.The import statements should be sorted alphabetically to prevent pipeline failures:
-import { createMockGraphQLContext } from "test/_Mocks_/mockContextCreator/mockContextCreator"; -import { beforeEach, describe, expect, it } from "vitest"; -import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError"; -import { resolveUpdatedAt } from "../../../../src/graphql/types/Chat/updatedAt"; +import { beforeEach, describe, expect, it } from "vitest"; +import { createMockGraphQLContext } from "test/_Mocks_/mockContextCreator/mockContextCreator"; +import { resolveUpdatedAt } from "../../../../src/graphql/types/Chat/updatedAt"; +import { TalawaGraphQLError } from "~/src/utilities/TalawaGraphQLError";
19-36: LGTM: Clean context setup using the new mock factoryThe context setup using the centralized
createMockGraphQLContextfunction is clean and correctly typed. This aligns with the PR objective of standardizing the approach to mock context creation.
1-1:✅ Verification successful
Fix import path inconsistency.
The import path for
createMockGraphQLContextappears to be using a different directory structure (test/_Mocks_/mockContextCreator/mockContextCreator) compared to the path used in previous commits (test/MockContext/mockContextCreator). Ensure this is the correct path and consistent across the codebase.
🏁 Script executed:
#!/bin/bash # Check for consistency in the import paths rg -A 1 "import.*createMockGraphQLContext" --type tsLength of output: 2674
Import path is consistent across the codebase.
All affected test files—includingtest/graphql/types/Chat/updatedAt.test.ts—now uniformly import from"test/_Mocks_/mockContextCreator/mockContextCreator". The previous inconsistency with the old path (test/MockContext/mockContextCreator) has been resolved, so no further changes are required.test/graphql/types/Post/updater.test.ts (2)
13-20: LGTM: Proper context initialization with the centralized factoryThe context initialization using the new
createMockGraphQLContextfunction is well-implemented. This improves maintainability and aligns with the PR objective of standardizing the approach to mock context creation.
35-37: LGTM: Clean handling of unauthenticated contextThe creation of an unauthenticated context for testing is well-implemented using the new centralized factory function.
test/graphql/types/Event/updater.test.ts (2)
28-28: Descriptive test suite title is appropriate.The updated test suite title "Event updater resolver - Tests" clearly describes what's being tested and is more specific than the previous title.
37-45: LGTM: Clean context setup with the centralized factoryThe context setup using the centralized
createMockGraphQLContextfunction is clean and well-implemented, improving code maintainability.test/graphql/types/Event/updatedAt.test.ts (2)
211-219: Conflicting mock implementations in the same testThis test has two conflicting mock implementations for the same method. The first mock at line 211 returns mockUserData, but the second at line 215 throws an error. This could cause unpredictable test behavior.
// First keep the mock that returns data mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue( mockUserData, ); // Then remove this conflicting mock implementation or use a different approach -mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => { - throw new TalawaGraphQLError({ - extensions: { code: "unauthorized_action" }, - }); -}); // Consider a different approach to test this error case, such as: const result = await eventUpdatedAtResolver(mockEvent, {}, ctx).catch(error => error); expect(result).toBeInstanceOf(TalawaGraphQLError); expect(result.extensions.code).toBe("unauthorized_action");
19-36: LGTM: Good implementation of the centralized mock contextThe test setup using the new centralized
createMockGraphQLContextis well-implemented and properly initialized. This improves consistency and maintainability of test code.test/graphql/types/Advertisement/updatedAt.test.ts (5)
1-3: Approve centralized mock context import.The import of the centralized mock context factory is correctly implemented, replacing the previous custom mock context creation approach. This aligns with the PR objective to standardize context creation across tests.
20-20: Approve adding mocks variable.Adding a dedicated variable to hold mock implementations from the mock context factory improves code organization and readability.
23-28: Approve context initialization using factory function.The context initialization approach properly destructures both the context and mocks from the factory function result, making the test setup more standardized and maintainable.
50-52: Approve updated mock implementation references.Mock implementation references have been correctly updated to use the new
mocksobject instead of directly accessing them throughctx. This follows the new pattern established by the mock context factory.Also applies to: 59-59
69-71: Approve remaining mock implementation updates.All remaining mock implementation references have been consistently updated to use the
mocksobject, maintaining a uniform approach throughout the test file.Also applies to: 87-89, 106-108, 129-131, 144-146, 158-161, 172-172, 183-185, 198-200
test/graphql/types/Tag/creator.test.ts (4)
1-3: Approve import of centralized mock context factory.The import statement correctly brings in the centralized mock context factory, removing the need for custom context creation in this test file.
20-21: Approve type declaration for context and mocks.The type declaration for the
mocksvariable properly leverages TypeScript's ReturnType utility to ensure type safety with the mock implementations.
24-29: Approve context initialization using factory function.The context initialization correctly destructures both context and mocks from the factory function result, providing a clean and standardized approach.
57-59: Approve consistent usage of mock implementations.All mock implementation references throughout the test file have been properly updated to use the
mocksobject, maintaining consistency with the new approach.Also applies to: 75-77, 91-93, 107-109, 116-118, 134-135, 162-164, 178-180, 223-223, 246-246, 260-262, 276-277, 308-309, 340-342
test/graphql/types/Fund/creator.test.ts (4)
1-4: Approve import changes for centralized mock context.The import statement correctly brings in the centralized mock context factory while maintaining only the necessary imports for the test file.
Also applies to: 8-8
20-20: Approve type declaration for mocks variable.The type declaration for the
mocksvariable properly leverages TypeScript's ReturnType utility to ensure type safety.
23-28: Approve context initialization using factory function.The context initialization correctly implements the new approach with proper spacing between parameters.
54-56: Approve consistent mock implementation references.All mock implementation references have been properly updated to use the
mocksobject returned by the factory function, maintaining consistency throughout the test file.Also applies to: 71-73, 86-88, 102-105, 121-123, 136-138, 148-150, 161-163, 178-178, 202-204, 212-212, 245-245, 276-280
test/graphql/types/Event/creator.test.ts (4)
1-3: Approve import of centralized mock context factory.The import statement correctly brings in the centralized mock context factory, removing the need for custom context creation in this test file.
21-21: Approve type declaration for mocks variable.The type declaration for the
mocksvariable properly leverages TypeScript's ReturnType utility to ensure type safety.
24-29: Approve context initialization approach.The context initialization correctly uses the factory function with proper parameter spacing.
54-56: Approve consistent usage of mock implementations.All mock implementation references have been properly updated to use the
mocksobject, maintaining consistency with the new approach.Also applies to: 69-71, 85-87, 121-123, 137-139, 164-168, 191-194, 207-209, 223-225, 239-241, 255-257, 284-287, 314-320, 340-342, 357-359
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/graphql/types/Event/creator.test.ts (1)
297-299: Unnecessary empty lineThere's an empty line that doesn't seem to serve any purpose. Consider removing line 298 for better readability.
- - mocks.drizzleClient.query.usersTable.findFirst
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/graphql/types/Event/creator.test.ts(17 hunks)test/graphql/types/Event/updatedAt.test.ts(11 hunks)test/graphql/types/Event/updater.test.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/graphql/types/Event/updatedAt.test.ts
- test/graphql/types/Event/updater.test.ts
🧰 Additional context used
🧠 Learnings (1)
test/graphql/types/Event/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T13:38:54.486Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (6)
test/graphql/types/Event/creator.test.ts (6)
1-3: Import refactoring looks clean and well-structuredThese import changes effectively implement the centralized context factory approach. The
GraphQLContexttype import ensures proper typing while using the new factory function.
21-29: Context creation implementation aligns well with PR goalsThe implementation correctly uses the centralized mock context factory with explicit parameters for authentication and user ID, making the test setup cleaner and more maintainable. The destructuring of both context and mocks is particularly elegant, as it provides clear access to both components.
54-56: Mock implementation updated correctlyThe refactoring properly uses the mocks object returned from the factory function, maintaining the same functionality while improving consistency across tests.
148-152: Improved variable assignment patternGood usage of the mocks object with an intermediate variable assignment for improved readability when chaining multiple mock implementations.
175-178: Consistent implementation of mock patternsThe consistent use of the mocks object throughout the test suite demonstrates the value of this centralized approach, making the tests more maintainable and easier to understand.
267-269:✅ Verification successful
Verify mock chaining implementation is correct
While the implementation is functionally correct, you might want to verify that this mocking pattern with chained calls works as expected. The approach is slightly different from earlier instances where you first assigned to a variable before chaining.
🏁 Script executed:
#!/bin/bash # Check for any similar patterns in the codebase to confirm consistency rg -A 3 --no-heading "mockResolvedValue.*\n.*mockResolvedValueOnce" --type ts test/Length of output: 277
Below is an updated verification command that uses multiline mode so that the chained mock calls (like the one on line 267 in
test/graphql/types/Event/creator.test.ts) can be correctly captured. Please run the following shell script to confirm that similar chaining patterns are consistently used across the tests:Next steps:
- Run this command to inspect similar usage of chained mocks.
- Verify that the approach in
creator.test.tsis consistent with other tests in the repository.Once you’ve confirmed the results, please update or confirm this mocking pattern’s consistency.
🏁 Script executed:
#!/bin/bash # Re-verify the usage of chained mock calls with multiline mode enabled. rg -U -A 3 --no-heading "mocks\.drizzleClient\.query\..*\n\s*\.mockResolvedValueOnce" --type ts test/Length of output: 7140
Mock chaining pattern verified
After examining the codebase, we found that the chained mock implementation used in
test/graphql/types/Event/creator.test.tsis consistent with the approach applied in other test files such as those for Post, Fund, Organization, and Tag. The pattern—using chained calls to.mockResolvedValueOnce—is uniformly applied and appears to work as expected.No changes are needed; please maintain this pattern for consistency.
Please provide some substantive feedback for improvement as I am eager to improve and contribute to other issues too , thanks! |
Piyushk8
force-pushed
the
mockFactoryFunction
branch
from
4329de5 to
34929bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
test/graphql/types/Post/updater.test.ts (1)
214-234: 🧹 Nitpick (assertive)Verify mocking implementation keeps original functionality.
The test still directly accesses
ctx.drizzleClient.query.usersTable.findFirstinstead of usingmocks. This might be intentional for this specific verification, but consider updating it for consistency with the rest of the file.Consider replacing:
- ( - ctx.drizzleClient.query.usersTable.findFirst as ReturnType<typeof vi.fn> - ).mockImplementation(({ with: withClause }) => { + mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(({ with: withClause }) => {
♻️ Duplicate comments (1)
test/graphql/types/Chat/updatedAt.test.ts (1)
49-49: 🧹 Nitpick (assertive)Consider using mockImplementation for better type safety.
According to the project's best practices, use
mockImplementation(() => Promise.resolve(undefined))instead ofmockResolvedValuewhen dealing with complex types to ensure type safety.-mocks.drizzleClient.query.usersTable.findFirst.mockResolvedValue(undefined); +mocks.drizzleClient.query.usersTable.findFirst.mockImplementation(() => Promise.resolve(undefined));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/docs/docs/developer-resources/testing.mdis excluded by!**/docs/docs/**
📒 Files selected for processing (15)
test/_Mocks_/drizzleClientMock.ts(1 hunks)test/_Mocks_/mockContextCreator/mockContextCreator.ts(1 hunks)test/_Mocks_/mockMinioClient.ts(1 hunks)test/_Mocks_/pubsubMock.ts(1 hunks)test/graphql/types/Advertisement/updatedAt.test.ts(12 hunks)test/graphql/types/Chat/updatedAt.test.ts(7 hunks)test/graphql/types/Chat/updater.test.ts(9 hunks)test/graphql/types/Event/creator.test.ts(17 hunks)test/graphql/types/Event/updatedAt.test.ts(11 hunks)test/graphql/types/Event/updater.test.ts(11 hunks)test/graphql/types/Fund/creator.test.ts(14 hunks)test/graphql/types/FundCampaign/updatedAt.test.ts(9 hunks)test/graphql/types/Organization/creator.test.ts(16 hunks)test/graphql/types/Post/updater.test.ts(11 hunks)test/graphql/types/Tag/creator.test.ts(18 hunks)
🧰 Additional context used
🧠 Learnings (12)
test/_Mocks_/mockContextCreator/mockContextCreator.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/FundCampaign/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T20:18:47.320Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Tag/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Chat/updater.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T20:18:47.320Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Organization/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/updater.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Advertisement/updatedAt.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Chat/updatedAt.test.ts (2)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T20:18:47.320Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Post/updater.test.ts (3)
Learnt from: PratapRathi
PR: PalisadoesFoundation/talawa-api#3310
File: test/graphql/types/Post/updater.test.ts:57-73
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the talawa-api project, the preferred approach for test data organization is to use shared mock objects initialized in beforeEach blocks to prevent code duplication, with specific test cases creating their own mock data only when necessary for that particular test scenario.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/updatedAt.test.ts (3)
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Event/updater.test.ts:139-162
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In test files, prefer keeping mock data separate for each test case instead of extracting common mock objects, as it provides better test isolation and makes future modifications easier without affecting other tests.
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3253
File: test/graphql/types/Chat/updatedAt.test.ts:48-54
Timestamp: 2025-03-12T20:18:47.320Z
Learning: When mocking functions with specific return types in Vitest, prefer using `mockImplementation(() => Promise.resolve(value))` over `mockResolvedValue(value)` when dealing with complex types to ensure type safety.
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Event/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
test/graphql/types/Fund/creator.test.ts (1)
Learnt from: Piyushk8
PR: PalisadoesFoundation/talawa-api#3319
File: test/MockContext/mockContextCreator.ts:46-48
Timestamp: 2025-03-12T20:19:03.355Z
Learning: In the Talawa API project, the mock context factory for GraphQL resolver tests uses a type assertion (`as unknown as FastifyInstance["drizzleClient"]`) to handle the drizzle client mocking. This was a deliberate design choice to centralize the type assertion in one place rather than creating a custom TestContext type that would need casting in every test.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (59)
test/_Mocks_/pubsubMock.ts (1)
5-17: Well-designed PubSub mock implementationThis implementation provides a clean and comprehensive mock for the PubSub interface. The approach of creating both
publishandsubscribemethods as mock functions is appropriate, and the implementation of the subscribe method with a Readable stream and async iterator correctly simulates the behavior of a real event stream.test/_Mocks_/mockContextCreator/mockContextCreator.ts (2)
16-26: Good separation of authenticated and unauthenticated client mocksThe clear distinction between unauthenticated and authenticated clients with proper documentation is a good design choice that makes the usage intention clear to other developers.
34-82: Well-structured centralized context factoryThis implementation successfully centralizes the mock context creation, which aligns with the PR objectives. The function:
- Creates all necessary mocks with proper typing
- Constructs both explicit and implicit contexts
- Returns both the context and exposed mocks for easier testing
- Uses type assertion centrally as mentioned in the retrieved learning
I particularly appreciate the use of default parameters for isAuthenticated and userId, which makes the function more convenient to use.
test/graphql/types/Chat/updater.test.ts (3)
52-52: Ensure the describe title matches the test contentThe describe title is "Chat.updater resolver", which correctly matches the file name and the resolver being tested.
53-69: Good refactoring to use the centralized mock contextThe test setup has been properly refactored to use the new
createMockGraphQLContextfunction. The types for context and mocks are correctly defined, and the beforeEach setup is clean and concise.
144-144: Test data aligned with mock contextThe updaterId has been correctly updated to "user-123" to align with the user ID used in the context setup.
test/_Mocks_/drizzleClientMock.ts (3)
5-17: Well-designed table method typesThe type definitions for TableMethods are comprehensive and correctly model the Drizzle client interface. Using Mock types from Vitest ensures that the mock functions have the correct typing for testing.
19-29: Good default mock implementationsThe
createTableMethodsfunction provides sensible default implementations for all table methods. This approach makes it easy to use the mocks without having to configure every method manually.
31-43: Elegant implementation of the mock Drizzle clientThe
createMockDrizzleClientfunction dynamically creates mock methods for each table in the schema. This approach ensures that the mock client structure stays in sync with the actual schema, which will reduce maintenance overhead.test/graphql/types/Organization/creator.test.ts (5)
1-1: Good addition of centralized mock context import.Using the centralized mock context creator aligns with the PR objectives to standardize the approach for GraphQL resolver tests.
4-4: Clean import refactoring.Correctly removed the
CurrentClienttype import while keeping the necessaryGraphQLContexttype.
19-20: Good type definition for context and mocks variables.Using
ReturnType<typeof createMockGraphQLContext>["mocks"]ensures type safety without manual type definitions.
23-28: Well-structured context initialization.The implementation correctly initializes both the context and mocks objects from the factory function, maintaining proper separation of concerns.
65-67: Consistent usage of mock objects across tests.The refactoring consistently replaces direct context mocking with the structured mocks object returned by the factory function, improving maintenance and readability.
Also applies to: 85-87, 103-104, 124-125, 148-150, 168-169, 183-185, 199-201, 225-226, 243-243, 267-269, 280-281, 313-313, 345-346
test/_Mocks_/mockMinioClient.ts (6)
1-4: Good imports for MinIO client mock.The imports include necessary modules for stream handling and MinIO client integration with Vitest.
5-12: Well-defined mock configuration.The configuration object has appropriate values for local testing with clear type definitions, especially the use of
as constfor the bucket name to ensure type safety.
14-21: Good type definition for MockMinioClient.The type definition properly structures the mock client with references to the configuration, ensuring consistency throughout the tests.
23-34: Well-implemented mock client initialization.The implementation creates a proper MinIO client instance with mock credentials and appropriate configuration values.
36-60: Comprehensive mocking of MinIO methods.The implementation mocks all necessary MinIO methods with appropriate behavior, including error handling in the
getObjectmethod and stream creation.
62-67: Clean return structure.Returns an object with all necessary components for testing: bucket name, configuration, and client instance.
test/graphql/types/FundCampaign/updatedAt.test.ts (5)
1-1: Good adoption of centralized mock context.The import aligns with the PR objectives to standardize mock context creation across tests.
3-3: Clean type import.Correctly removed the
CurrentClienttype import while keeping the necessaryGraphQLContexttype.
24-24: Well-typed mocks variable.Using
ReturnType<typeof createMockGraphQLContext>["mocks"]ensures type safety for the mock objects.
27-32: Proper context initialization.The implementation correctly sets up both the context and mocks objects from the factory function.
43-43: Consistent usage of mocks across tests.All test cases consistently use the mocks object for drizzle client queries, maintaining a uniform approach throughout the file.
Also applies to: 50-52, 69-70, 85-86, 100-101, 107-109, 117-119, 127-129, 137-139, 147-149, 158-159, 173-175, 191-192
test/graphql/types/Post/updater.test.ts (5)
1-1: Good adoption of centralized mock context.The import aligns with the PR objectives to standardize mock context creation across tests.
10-11: Well-structured variable declarations.Properly declared context and mocks variables with appropriate types.
14-19: Well-implemented context initialization.Correctly sets up both the context and mocks objects using the factory function.
36-39: Good approach for unauthenticated test.Creates a separate unauthenticated context for testing, which is a clean pattern for testing authentication failures.
48-48: Consistent usage of mocks across tests.All test cases consistently use the mocks object for database queries, maintaining a uniform approach across the file.
Also applies to: 65-67, 83-85, 101-103, 121-123, 135-137, 156-158, 170-172, 191-192
test/graphql/types/Chat/updatedAt.test.ts (3)
1-1: Correctly importing the new mock context creator.The import statement correctly references the centralized mock context creator, which is in line with the PR's objective to standardize the approach to mock context creation.
20-26: Good use of TypeScript type inference.The type definitions correctly leverage TypeScript's
ReturnTypeutility to ensure type safety without duplicating the return type ofcreateMockGraphQLContext.
28-36: Well-structured beforeEach setup with the new mock factory.The setup now uses the centralized mock context factory, which creates both authenticated and unauthenticated contexts. This approach is cleaner and more maintainable than the previous implementation.
test/graphql/types/Event/updater.test.ts (5)
1-2: Imports are organized appropriately.The import for the mock context creator has been added correctly, and the import statements are organized properly with the Vitest imports.
28-28: Improved test suite description.The updated describe block title "Event updater resolver - Tests" is more descriptive and consistent with the module being tested.
29-45: Well-structured mock context setup.The initialization of authenticated and unauthenticated contexts using the centralized mock factory is well-implemented. This approach follows the PR's objective to standardize mock context creation.
62-64: Proper use of mockImplementation.This test correctly uses
mockImplementation(() => Promise.resolve(undefined))for type safety, which aligns with the project's best practices.
76-80: Consistent user ID updates across mock objects.The user IDs have been consistently updated from "user_1" and "updater_1" to "user-123" throughout the file, matching the ID used in the mock context factory. This ensures consistency between the test data and the mock setup.
Also applies to: 97-101, 117-121, 141-146, 159-163, 175-186, 200-205
test/graphql/types/Event/updatedAt.test.ts (6)
8-8: Correctly importing the centralized mock context creator.The import for
createMockGraphQLContextreplaces the custom context creation logic, aligning with the PR's goal of standardizing mock context creation.
19-19: Appropriate test name and structure.The describe block "Event UpdatedAt Resolver Tests" clearly indicates what functionality is being tested.
22-22: Good type definition for mocks.Using TypeScript's
ReturnTypeutility to type the mocks ensures type safety without duplication.
25-30: Well-structured context initialization.The setup correctly initializes both the context and mocks using the centralized factory.
53-58: Improved test description and implementation.The test case description is now more descriptive, and the implementation properly uses the new mock structure.
206-211: No more conflicting mock implementations.This test now properly isolates the mock implementation that throws an error in its own test case, fixing the previous issue of conflicting mock implementations within the same test.
test/graphql/types/Advertisement/updatedAt.test.ts (4)
1-1: Correctly importing the centralized mock context creator.The import for
createMockGraphQLContextreplaces the custom context creation logic, aligning with the PR's goal of standardizing mock context creation.
20-20: Good type definition for mocks.Using TypeScript's
ReturnTypeutility to type the mocks ensures type safety without duplication.
23-28: Well-structured context initialization.The setup correctly initializes both the context and mocks using the centralized factory, improving maintainability.
59-59: Updated assertion to use mocks.The assertion has been properly updated to use
mocks.drizzleClient.query.usersTable.findFirstinstead ofctx.drizzleClient.query.usersTable.findFirst.test/graphql/types/Tag/creator.test.ts (4)
1-7: LGTM: Imports properly updated for centralized mock context.The changes appropriately replace the custom mock context implementation with the new centralized mock context factory, maintaining the necessary imports for GraphQLContext type.
20-21: LGTM: Context and mocks variables properly declared.The context variable type is properly updated to GraphQLContext, and a new mocks variable is added to store the mock implementations returned by createMockGraphQLContext.
23-29: LGTM: Context initialization refactored properly.The context initialization now correctly uses the centralized createMockGraphQLContext function with appropriate parameters (authentication status and user ID).
57-59: LGTM: Mock implementation reference updated correctly.All mock implementations now correctly reference the mocks object instead of directly accessing the context's drizzleClient.
test/graphql/types/Fund/creator.test.ts (4)
1-8: LGTM: Import statements properly updated.The imports have been correctly updated to use the centralized mock context creator and maintain the GraphQLContext type.
20-20: LGTM: Mocks variable declaration added.Properly added the mocks variable with the correct type derived from the createMockGraphQLContext return type.
23-28: LGTM: Context initialization refactored correctly.The context initialization has been properly updated to use the centralized mock context factory with appropriate parameters and structure.
54-56: LGTM: Mock implementation references updated.Mock implementation references have been correctly updated to use the mocks object returned by the factory function.
test/graphql/types/Event/creator.test.ts (3)
1-7: LGTM: Import statements properly updated.The imports have been correctly updated to use the centralized mock context creator and maintain the GraphQLContext type.
21-21: LGTM: Mocks variable declaration added.Properly added the mocks variable with the correct type derived from the createMockGraphQLContext return type.
24-29: LGTM: Context initialization refactored correctly.The context initialization has been properly updated to use the centralized mock context factory with appropriate parameters.
palisadoes
merged commit e95f472
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
This PR introduces a centralized mock GraphQL context factory function for unit tests, ensuring consistency across all resolver tests. It replaces scattered mock context implementations with a single, structured
GraphQLContext, reducing code duplication and improving maintainability.Issue Number:
Fixes #3292
Summary
Previously, unit tests used individually created mock contexts for resolver testing, leading to redundancy and inconsistency. This PR standardizes the approach by implementing a unified
GraphQLContextfactory, serving as the single source of truth for constructing resolver contexts in tests.This approach ensures all tests consistently simulate various authentication states, database interactions, logging, and other GraphQL dependencies in a controlled manner.
Key Updates:
createMockGraphQLContext.isAuthenticateddynamically.drizzleClient.query).ReturnType<typeof vi.fn>) to bridge the gap between real and mocked dependencies.Does this PR introduce a breaking change?
NO
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit