Code coverage for /src/graphql/types/Query/fund.ts #3166
Conversation
WalkthroughThis pull request introduces a comprehensive suite of tests for the GraphQL fund query. The tests cover multiple error scenarios—including unauthenticated access, non-existent funds, and unauthorized users—as well as successful data retrieval for admins and organization members. In addition, new GraphQL operations (a query and several mutations) have been added in the document nodes and cache declaration files to support fund queries and the creation of organizations, funds, and organization memberships. Changes
Assessment against linked issues
Possibly related issues
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3166 +/- ##
====================================================
+ Coverage 39.71% 42.08% +2.37%
====================================================
Files 455 455
Lines 33553 33647 +94
Branches 407 563 +156
====================================================
+ Hits 13324 14161 +837
+ Misses 20229 19486 -743 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
test/routes/graphql/Query/fund.test.ts(1 hunks)test/routes/graphql/documentNodes.ts(1 hunks)test/routes/graphql/gql.tada-cache.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 (4)
test/routes/graphql/documentNodes.ts (1)
311-342: LGTM! Well-structured GraphQL operations.The new operations are well-defined and follow consistent patterns with existing operations in the file.
test/routes/graphql/Query/fund.test.ts (2)
22-128: LGTM! Comprehensive error handling test cases.The error handling tests cover key scenarios:
- Unauthenticated access
- Non-existent fund
- Unauthorized access for non-members
193-356: LGTM! Thorough schema validation tests.The validation tests cover:
- Name length constraints
- Unique name constraints
- Required fields
- UUID validation
- Field type constraints
test/routes/graphql/gql.tada-cache.d.ts (1)
33-40: LGTM! Well-defined TypeScript types for GraphQL operations.The type definitions provide proper type safety and are consistent with the GraphQL schema.
| test("returns fund data if user is an admin", async () => { | ||
| const adminSignInResult = await mercuriusClient.query(Query_signIn, { | ||
| variables: { | ||
| input: { | ||
| emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, | ||
| password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); | ||
|
|
||
| const { fundId } = await createFund(); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.errors).toBeUndefined(); | ||
| expect(fundResult.data.fund).toEqual( | ||
| expect.objectContaining({ | ||
| id: fundId, | ||
| isTaxDeductible: expect.any(Boolean), | ||
| name: expect.any(String), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| test("returns fund data if user is organization member", async () => { | ||
| const regularUserResult = await createRegularUser(); | ||
| const { fundId, orgId } = await createFund(); | ||
|
|
||
| await addUserToOrg(regularUserResult.userId, orgId); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${regularUserResult.authToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.errors).toBeUndefined(); | ||
| expect(fundResult.data.fund).toEqual( | ||
| expect.objectContaining({ | ||
| id: fundId, | ||
| isTaxDeductible: expect.any(Boolean), | ||
| name: expect.any(String), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding edge cases for successful fund retrieval.
While the basic success cases are covered, consider adding tests for:
- Funds with null/empty fields
- Funds with maximum length names
- Multiple funds in the same organization
| }, | ||
| variables: { | ||
| input: { | ||
| emailAddress: `email${faker.string.uuid()}@test.com`, | ||
| password: "password123", | ||
| role: "regular", | ||
| name: "Test User", | ||
| isEmailAddressVerified: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(userResult.data?.createUser?.authenticationToken); | ||
| assertToBeNonNullish(userResult.data?.createUser?.user?.id); | ||
|
|
||
| return { | ||
| authToken: userResult.data.createUser.authenticationToken, | ||
| userId: userResult.data.createUser.user.id, | ||
| }; | ||
| } | ||
|
|
||
| async function createFund() { | ||
| // First sign in as admin to create org and fund | ||
| const adminSignInResult = await mercuriusClient.query(Query_signIn, { | ||
| variables: { | ||
| input: { | ||
| emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, | ||
| password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); | ||
|
|
||
| // Create organization | ||
| const createOrgResult = await mercuriusClient.mutate( | ||
| Mutation_createOrganization, | ||
| { | ||
| headers: { | ||
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| name: `Org ${faker.string.uuid()}`, | ||
| countryCode: "us", | ||
| }, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| assertToBeNonNullish(createOrgResult.data?.createOrganization?.id); | ||
| const orgId = createOrgResult.data.createOrganization.id; | ||
|
|
||
| // Create fund | ||
| const createFundResult = await mercuriusClient.mutate(Mutation_createFund, { | ||
| headers: { | ||
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| name: `Fund ${faker.string.uuid()}`, | ||
| organizationId: orgId, | ||
| isTaxDeductible: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(createFundResult.data?.createFund?.id); | ||
|
|
||
| return { | ||
| fundId: createFundResult.data.createFund.id, | ||
| orgId, | ||
| }; | ||
| } | ||
|
|
||
| async function addUserToOrg(userId: string, orgId: string) { | ||
| const adminSignInResult = await mercuriusClient.query(Query_signIn, { | ||
| variables: { | ||
| input: { | ||
| emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, | ||
| password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); | ||
|
|
||
| await mercuriusClient.mutate(Mutation_createOrganizationMembership, { | ||
| headers: { | ||
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| memberId: userId, | ||
| organizationId: orgId, | ||
| role: "regular", | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing helper functions for better reusability.
The helper functions could be improved by:
- Adding error handling for failed operations
- Supporting custom input parameters
- Adding cleanup functions to remove test data
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
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/routes/graphql/Query/fund.test.ts (1)
516-632:⚠️ Potential issueRemove duplicate helper functions and enhance error handling.
The helper functions are duplicated:
createRegularUser: Lines 161-187 and 516-552createFund: Lines 189-231 and 554-606addUserToOrg: Lines 233-248 and 608-632Consider enhancing the helper functions with:
- Error handling for failed operations
- Cleanup functions to remove test data
- Retry logic for flaky operations
- Logging for debugging
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/routes/graphql/Query/fund.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
| test("returns fund data if user is an admin", async () => { | ||
| const adminSignInResult = await mercuriusClient.query(Query_signIn, { | ||
| variables: { | ||
| input: { | ||
| emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, | ||
| password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); | ||
|
|
||
| const { fundId } = await createFund(); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.errors).toBeUndefined(); | ||
| expect(fundResult.data.fund).toEqual( | ||
| expect.objectContaining({ | ||
| id: fundId, | ||
| isTaxDeductible: expect.any(Boolean), | ||
| name: expect.any(String), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test case.
This test case is a duplicate of the one at lines 107-130. Consider removing this redundant test to maintain a cleaner test suite.
| test("returns fund data if user is organization member", async () => { | ||
| const regularUserResult = await createRegularUser(); | ||
| const { fundId, orgId } = await createFund(); | ||
|
|
||
| await addUserToOrg(regularUserResult.userId, orgId); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${regularUserResult.authToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.errors).toBeUndefined(); | ||
| expect(fundResult.data.fund).toEqual( | ||
| expect.objectContaining({ | ||
| id: fundId, | ||
| isTaxDeductible: expect.any(Boolean), | ||
| name: expect.any(String), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test case.
This test case is a duplicate of the one at lines 132-157. Consider removing this redundant test to maintain a cleaner test suite.
| input: { | ||
| name: fundName, | ||
| organizationId: orgId, | ||
| isTaxDeductible: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(fund1Result.data?.createFund?.id); | ||
|
|
||
| const fund2Result = await mercuriusClient.mutate(Mutation_createFund, { | ||
| headers: { | ||
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| name: fundName, | ||
| organizationId: orgId, | ||
| isTaxDeductible: true, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fund2Result.errors).toBeDefined(); | ||
| expect(fund2Result.errors?.[0]?.extensions?.code).toBe( | ||
| "forbidden_action_on_arguments_associated_resources", | ||
| ); | ||
| }); | ||
|
|
||
| test("verifies UUID generation for fund id", async () => { | ||
| const { fundId } = await createFund(); | ||
| expect(fundId).toMatch( | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| test("validates required fund fields", () => { | ||
| const validInput = { | ||
| name: "Test Fund", | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }; | ||
|
|
||
| // Test missing name | ||
| const missingName = { ...validInput, name: undefined }; | ||
| expect(fundsTableInsertSchema.safeParse(missingName).success).toBe(false); | ||
|
|
||
| // Test missing isTaxDeductible | ||
| const missingTaxStatus = { ...validInput, isTaxDeductible: undefined }; | ||
| expect(fundsTableInsertSchema.safeParse(missingTaxStatus).success).toBe( | ||
| false, | ||
| ); | ||
|
|
||
| // Test missing organizationId | ||
| const missingOrgId = { ...validInput, organizationId: undefined }; | ||
| expect(fundsTableInsertSchema.safeParse(missingOrgId).success).toBe(false); | ||
|
|
||
| // Valid data should pass | ||
| expect(fundsTableInsertSchema.safeParse(validInput).success).toBe(true); | ||
| }); | ||
|
|
||
| test("validates fund field constraints", () => { | ||
| const validInput = { | ||
| name: "Test Fund", | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }; | ||
|
|
||
| // Test name constraints | ||
| expect( | ||
| fundsTableInsertSchema.safeParse({ | ||
| ...validInput, | ||
| name: "", | ||
| }).success, | ||
| ).toBe(false); | ||
|
|
||
| expect( | ||
| fundsTableInsertSchema.safeParse({ | ||
| ...validInput, | ||
| name: "a".repeat(257), | ||
| }).success, | ||
| ).toBe(false); | ||
|
|
||
| // Test isTaxDeductible must be boolean | ||
| expect( | ||
| fundsTableInsertSchema.safeParse({ | ||
| ...validInput, | ||
| isTaxDeductible: "true", | ||
| }).success, | ||
| ).toBe(false); | ||
|
|
||
| // Test organizationId must be UUID | ||
| expect( | ||
| fundsTableInsertSchema.safeParse({ | ||
| ...validInput, | ||
| organizationId: "invalid-uuid", | ||
| }).success, | ||
| ).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding more edge cases for fund name validation.
While the current validation tests are good, consider adding test cases for:
- Fund names with special characters
- Fund names with leading/trailing spaces
- Fund names with only whitespace characters
- Unicode characters in fund names
test("validates fund name length constraints", () => {
const tooShortName = "";
const tooLongName = "a".repeat(257);
const validName = "Test Fund";
+ const specialCharsName = "Fund #1 @Special!";
+ const whitespaceOnlyName = " ";
+ const unicodeName = "Fund 🚀 测试";
+ const spacePaddedName = " Test Fund ";
// ... existing tests ...
+ // Test special characters
+ const specialCharsResult = fundsTableInsertSchema.safeParse({
+ name: specialCharsName,
+ isTaxDeductible: false,
+ organizationId: faker.string.uuid(),
+ });
+ expect(specialCharsResult.success).toBe(true);
+ // Test whitespace-only name
+ const whitespaceOnlyResult = fundsTableInsertSchema.safeParse({
+ name: whitespaceOnlyName,
+ isTaxDeductible: false,
+ organizationId: faker.string.uuid(),
+ });
+ expect(whitespaceOnlyResult.success).toBe(false);
+ // Test unicode characters
+ const unicodeResult = fundsTableInsertSchema.safeParse({
+ name: unicodeName,
+ isTaxDeductible: false,
+ organizationId: faker.string.uuid(),
+ });
+ expect(unicodeResult.success).toBe(true);
+ // Test space-padded name
+ const spacePaddedResult = fundsTableInsertSchema.safeParse({
+ name: spacePaddedName,
+ isTaxDeductible: false,
+ organizationId: faker.string.uuid(),
+ });
+ expect(spacePaddedResult.success).toBe(true);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suite("Funds schema validation and field behavior", () => { | |
| test("validates fund name length constraints", () => { | |
| const tooShortName = ""; | |
| const tooLongName = "a".repeat(257); | |
| const validName = "Test Fund"; | |
| const tooShortResult = fundsTableInsertSchema.safeParse({ | |
| name: tooShortName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(tooShortResult.success).toBe(false); | |
| const tooLongResult = fundsTableInsertSchema.safeParse({ | |
| name: tooLongName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(tooLongResult.success).toBe(false); | |
| const validResult = fundsTableInsertSchema.safeParse({ | |
| name: validName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(validResult.success).toBe(true); | |
| }); | |
| test("verifies unique constraint on fund name within organization", async () => { | |
| const adminSignInResult = await mercuriusClient.query(Query_signIn, { | |
| variables: { | |
| input: { | |
| emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, | |
| password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, | |
| }, | |
| }, | |
| }); | |
| assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); | |
| const createOrgResult = await mercuriusClient.mutate( | |
| Mutation_createOrganization, | |
| { | |
| headers: { | |
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| name: `Org ${faker.string.uuid()}`, | |
| countryCode: "us", | |
| }, | |
| }, | |
| }, | |
| ); | |
| assertToBeNonNullish(createOrgResult.data?.createOrganization?.id); | |
| const orgId = createOrgResult.data.createOrganization.id; | |
| const fundName = `Test Fund ${faker.string.uuid()}`; | |
| const fund1Result = await mercuriusClient.mutate(Mutation_createFund, { | |
| headers: { | |
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| name: fundName, | |
| organizationId: orgId, | |
| isTaxDeductible: false, | |
| }, | |
| }, | |
| }); | |
| assertToBeNonNullish(fund1Result.data?.createFund?.id); | |
| const fund2Result = await mercuriusClient.mutate(Mutation_createFund, { | |
| headers: { | |
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| name: fundName, | |
| organizationId: orgId, | |
| isTaxDeductible: true, | |
| }, | |
| }, | |
| }); | |
| expect(fund2Result.errors).toBeDefined(); | |
| expect(fund2Result.errors?.[0]?.extensions?.code).toBe( | |
| "forbidden_action_on_arguments_associated_resources", | |
| ); | |
| }); | |
| test("verifies UUID generation for fund id", async () => { | |
| const { fundId } = await createFund(); | |
| expect(fundId).toMatch( | |
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, | |
| ); | |
| }); | |
| }); | |
| test("validates required fund fields", () => { | |
| const validInput = { | |
| name: "Test Fund", | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }; | |
| // Test missing name | |
| const missingName = { ...validInput, name: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingName).success).toBe(false); | |
| // Test missing isTaxDeductible | |
| const missingTaxStatus = { ...validInput, isTaxDeductible: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingTaxStatus).success).toBe( | |
| false, | |
| ); | |
| // Test missing organizationId | |
| const missingOrgId = { ...validInput, organizationId: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingOrgId).success).toBe(false); | |
| // Valid data should pass | |
| expect(fundsTableInsertSchema.safeParse(validInput).success).toBe(true); | |
| }); | |
| test("validates fund field constraints", () => { | |
| const validInput = { | |
| name: "Test Fund", | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }; | |
| // Test name constraints | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| name: "", | |
| }).success, | |
| ).toBe(false); | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| name: "a".repeat(257), | |
| }).success, | |
| ).toBe(false); | |
| // Test isTaxDeductible must be boolean | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| isTaxDeductible: "true", | |
| }).success, | |
| ).toBe(false); | |
| // Test organizationId must be UUID | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| organizationId: "invalid-uuid", | |
| }).success, | |
| ).toBe(false); | |
| }); | |
| suite("Funds schema validation and field behavior", () => { | |
| test("validates fund name length constraints", () => { | |
| const tooShortName = ""; | |
| const tooLongName = "a".repeat(257); | |
| const validName = "Test Fund"; | |
| const specialCharsName = "Fund #1 @Special!"; | |
| const whitespaceOnlyName = " "; | |
| const unicodeName = "Fund 🚀 测试"; | |
| const spacePaddedName = " Test Fund "; | |
| const tooShortResult = fundsTableInsertSchema.safeParse({ | |
| name: tooShortName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(tooShortResult.success).toBe(false); | |
| const tooLongResult = fundsTableInsertSchema.safeParse({ | |
| name: tooLongName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(tooLongResult.success).toBe(false); | |
| const validResult = fundsTableInsertSchema.safeParse({ | |
| name: validName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(validResult.success).toBe(true); | |
| // Test special characters | |
| const specialCharsResult = fundsTableInsertSchema.safeParse({ | |
| name: specialCharsName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(specialCharsResult.success).toBe(true); | |
| // Test whitespace-only name | |
| const whitespaceOnlyResult = fundsTableInsertSchema.safeParse({ | |
| name: whitespaceOnlyName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(whitespaceOnlyResult.success).toBe(false); | |
| // Test unicode characters | |
| const unicodeResult = fundsTableInsertSchema.safeParse({ | |
| name: unicodeName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(unicodeResult.success).toBe(true); | |
| // Test space-padded name | |
| const spacePaddedResult = fundsTableInsertSchema.safeParse({ | |
| name: spacePaddedName, | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }); | |
| expect(spacePaddedResult.success).toBe(true); | |
| }); | |
| test("verifies unique constraint on fund name within organization", async () => { | |
| const adminSignInResult = await mercuriusClient.query(Query_signIn, { | |
| variables: { | |
| input: { | |
| emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, | |
| password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, | |
| }, | |
| }, | |
| }); | |
| assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); | |
| const createOrgResult = await mercuriusClient.mutate( | |
| Mutation_createOrganization, | |
| { | |
| headers: { | |
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| name: `Org ${faker.string.uuid()}`, | |
| countryCode: "us", | |
| }, | |
| }, | |
| }, | |
| ); | |
| assertToBeNonNullish(createOrgResult.data?.createOrganization?.id); | |
| const orgId = createOrgResult.data.createOrganization.id; | |
| const fundName = `Test Fund ${faker.string.uuid()}`; | |
| const fund1Result = await mercuriusClient.mutate(Mutation_createFund, { | |
| headers: { | |
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| name: fundName, | |
| organizationId: orgId, | |
| isTaxDeductible: false, | |
| }, | |
| }, | |
| }); | |
| assertToBeNonNullish(fund1Result.data?.createFund?.id); | |
| const fund2Result = await mercuriusClient.mutate(Mutation_createFund, { | |
| headers: { | |
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| name: fundName, | |
| organizationId: orgId, | |
| isTaxDeductible: true, | |
| }, | |
| }, | |
| }); | |
| expect(fund2Result.errors).toBeDefined(); | |
| expect(fund2Result.errors?.[0]?.extensions?.code).toBe( | |
| "forbidden_action_on_arguments_associated_resources", | |
| ); | |
| }); | |
| test("verifies UUID generation for fund id", async () => { | |
| const { fundId } = await createFund(); | |
| expect(fundId).toMatch( | |
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, | |
| ); | |
| }); | |
| }); | |
| test("validates required fund fields", () => { | |
| const validInput = { | |
| name: "Test Fund", | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }; | |
| // Test missing name | |
| const missingName = { ...validInput, name: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingName).success).toBe(false); | |
| // Test missing isTaxDeductible | |
| const missingTaxStatus = { ...validInput, isTaxDeductible: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingTaxStatus).success).toBe(false); | |
| // Test missing organizationId | |
| const missingOrgId = { ...validInput, organizationId: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingOrgId).success).toBe(false); | |
| // Valid data should pass | |
| expect(fundsTableInsertSchema.safeParse(validInput).success).toBe(true); | |
| }); | |
| test("validates fund field constraints", () => { | |
| const validInput = { | |
| name: "Test Fund", | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }; | |
| // Test name constraints | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| name: "", | |
| }).success, | |
| ).toBe(false); | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| name: "a".repeat(257), | |
| }).success, | |
| ).toBe(false); | |
| // Test isTaxDeductible must be boolean | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| isTaxDeductible: "true", | |
| }).success, | |
| ).toBe(false); | |
| // Test organizationId must be UUID | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| organizationId: "invalid-uuid", | |
| }).success, | |
| ).toBe(false); | |
| }); |
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
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/routes/graphql/Query/fund.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (3)
test/routes/graphql/Query/fund.test.ts (3)
263-296: Remove duplicate test cases.These test cases are duplicates of the tests at lines 107-132. Consider removing them to maintain a cleaner test suite.
Also applies to: 298-323
1-24: LGTM! Well-structured test file setup.The file has a clear structure with proper imports and helper function organization.
531-647: 🧹 Nitpick (assertive)Enhance helper functions with error handling and cleanup.
The helper functions could be improved by:
- Adding error handling for failed operations
- Supporting custom input parameters
- Adding cleanup functions to remove test data
Here's how to enhance the
createRegularUserfunction:async function createRegularUser(options?: { + name?: string; + role?: string; + emailAddress?: string; + password?: string; }) { try { const adminSignInResult = await mercuriusClient.query(Query_signIn, { variables: { input: { emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, }, }, }); if (!adminSignInResult.data?.signIn?.authenticationToken) { + throw new Error('Failed to get admin auth token'); } const userResult = await mercuriusClient.mutate(Mutation_createUser, { headers: { authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, }, variables: { input: { - emailAddress: `email${faker.string.uuid()}@test.com`, - password: "password123", - role: "regular", - name: "Test User", + emailAddress: options?.emailAddress ?? `email${faker.string.uuid()}@test.com`, + password: options?.password ?? "password123", + role: options?.role ?? "regular", + name: options?.name ?? "Test User", isEmailAddressVerified: false, }, }, }); if (!userResult.data?.createUser?.authenticationToken || !userResult.data?.createUser?.user?.id) { + throw new Error('Failed to create user'); } return { authToken: userResult.data.createUser.authenticationToken, userId: userResult.data.createUser.user.id, + cleanup: async () => { + // Add cleanup logic here + // e.g., delete the user + } }; + } catch (error) { + throw new Error(`Failed to create regular user: ${error.message}`); } }Likely invalid or redundant comment.
| suite("Funds schema validation and field behavior", () => { | ||
| test("validates fund name constraints", () => { | ||
| // Test length constraints | ||
| const tooShortName = ""; | ||
| const tooLongName = "a".repeat(257); | ||
| const validName = "Test Fund"; | ||
|
|
||
| // Test special cases | ||
| const specialCharsName = "Fund #1 @Special!"; | ||
| const whitespaceOnlyName = " "; | ||
| const unicodeName = "Fund 🚀 测试"; | ||
| const spacePaddedName = " Test Fund "; | ||
|
|
||
| // Test length constraints | ||
| const tooShortResult = fundsTableInsertSchema.safeParse({ | ||
| name: tooShortName, | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }); | ||
| expect(tooShortResult.success).toBe(false); | ||
|
|
||
| const tooLongResult = fundsTableInsertSchema.safeParse({ | ||
| name: tooLongName, | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }); | ||
| expect(tooLongResult.success).toBe(false); | ||
|
|
||
| const validResult = fundsTableInsertSchema.safeParse({ | ||
| name: validName, | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }); | ||
| expect(validResult.success).toBe(true); | ||
|
|
||
| // Test special characters | ||
| const specialCharsResult = fundsTableInsertSchema.safeParse({ | ||
| name: specialCharsName, | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }); | ||
| expect(specialCharsResult.success).toBe(true); | ||
|
|
||
| // Test whitespace-only name | ||
| const whitespaceOnlyResult = fundsTableInsertSchema.safeParse({ | ||
| name: whitespaceOnlyName, | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }); | ||
| expect(whitespaceOnlyResult.success).toBe(true); | ||
|
|
||
| // Test unicode characters | ||
| const unicodeResult = fundsTableInsertSchema.safeParse({ | ||
| name: unicodeName, | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }); | ||
| expect(unicodeResult.success).toBe(true); | ||
|
|
||
| // Test space-padded name | ||
| const spacePaddedResult = fundsTableInsertSchema.safeParse({ | ||
| name: spacePaddedName, | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }); | ||
| expect(spacePaddedResult.success).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding validation for trimmed fund names.
While the test cases cover various scenarios including special characters and whitespace, they don't verify if the schema trims whitespace from fund names. This could lead to duplicate funds with the same name but different spacing.
Add a test case to verify trimming behavior:
// Test space-padded name
const spacePaddedResult = fundsTableInsertSchema.safeParse({
name: spacePaddedName,
isTaxDeductible: false,
organizationId: faker.string.uuid(),
});
expect(spacePaddedResult.success).toBe(true);
+
+// Test trimming behavior if supported
+if (validResult.success) {
+ const trimmedResult = validResult.data;
+ expect(trimmedResult.name).toBe(validName.trim());
+}Committable suggestion skipped: line range outside the PR's diff.
| test("verifies UUID generation for fund id", async () => { | ||
| const { fundId } = await createFund(); | ||
| expect(fundId).toMatch( | ||
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding more UUID validation test cases.
The UUID validation test only checks the format. Consider adding tests for:
- Invalid UUID formats
- Case sensitivity
- UUID version validation
test("verifies UUID generation for fund id", async () => {
const { fundId } = await createFund();
expect(fundId).toMatch(
/^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i,
);
+
+ // Test invalid UUID formats
+ const invalidFormats = [
+ 'not-a-uuid',
+ '123e4567-e89b-12d3-a456-426614174000', // Invalid version
+ '123e4567-e89b-7-a456-426614174000', // Missing digits
+ ];
+
+ for (const invalidId of invalidFormats) {
+ const fundResult = await mercuriusClient.query(Query_fund, {
+ variables: { input: { id: invalidId } },
+ });
+ expect(fundResult.errors).toBeDefined();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("verifies UUID generation for fund id", async () => { | |
| const { fundId } = await createFund(); | |
| expect(fundId).toMatch( | |
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, | |
| ); | |
| }); | |
| test("verifies UUID generation for fund id", async () => { | |
| const { fundId } = await createFund(); | |
| expect(fundId).toMatch( | |
| /^[0-9a-f]{8}-[0-9a-f]{4}-7[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i, | |
| ); | |
| // Test invalid UUID formats | |
| const invalidFormats = [ | |
| 'not-a-uuid', | |
| '123e4567-e89b-12d3-a456-426614174000', // Invalid version | |
| '123e4567-e89b-7-a456-426614174000', // Missing digits | |
| ]; | |
| for (const invalidId of invalidFormats) { | |
| const fundResult = await mercuriusClient.query(Query_fund, { | |
| variables: { input: { id: invalidId } }, | |
| }); | |
| expect(fundResult.errors).toBeDefined(); | |
| } | |
| }); |
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
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/routes/graphql/Query/fund.test.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 (3)
test/routes/graphql/Query/fund.test.ts (3)
263-296: Remove duplicate test cases.These test cases are duplicates of the tests at lines 107-132. Remove them to maintain a cleaner test suite and reduce test execution time.
Also applies to: 298-323
609-645: Remove duplicate helper function.This
createRegularUserfunction is a duplicate of the one at lines 136-162. Remove this duplicate and use the existing function.
37-567: Well-structured test suites with comprehensive coverage!The test organization and coverage are excellent, including:
- Clear test descriptions
- Thorough validation of edge cases
- Good separation of concerns in test suites
| async function createRegularUser() { | ||
| const adminAuthToken = await getAdminAuthToken(); | ||
|
|
||
| // Create regular user as admin | ||
| const userResult = await mercuriusClient.mutate(Mutation_createUser, { | ||
| headers: { | ||
| authorization: `bearer ${adminAuthToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| emailAddress: `email${faker.string.uuid()}@test.com`, | ||
| password: "password123", | ||
| role: "regular", | ||
| name: "Test User", | ||
| isEmailAddressVerified: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(userResult.data?.createUser?.authenticationToken); | ||
| assertToBeNonNullish(userResult.data?.createUser?.user?.id); | ||
|
|
||
| return { | ||
| authToken: userResult.data.createUser.authenticationToken, | ||
| userId: userResult.data.createUser.user.id, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance helper functions with error handling and cleanup.
The helper functions could be improved by:
- Adding error handling for failed operations
- Adding cleanup functions to remove test data
- Adding retry logic for flaky operations
Here's an example improvement for the createRegularUser function:
async function createRegularUser() {
+ try {
const adminAuthToken = await getAdminAuthToken();
// Create regular user as admin
const userResult = await mercuriusClient.mutate(Mutation_createUser, {
headers: {
authorization: `bearer ${adminAuthToken}`,
},
variables: {
input: {
emailAddress: `email${faker.string.uuid()}@test.com`,
password: "password123",
role: "regular",
name: "Test User",
isEmailAddressVerified: false,
},
},
});
assertToBeNonNullish(userResult.data?.createUser?.authenticationToken);
assertToBeNonNullish(userResult.data?.createUser?.user?.id);
return {
authToken: userResult.data.createUser.authenticationToken,
userId: userResult.data.createUser.user.id,
+ cleanup: async () => {
+ // Add cleanup logic here
+ // e.g., delete the created user
+ }
};
+ } catch (error) {
+ console.error('Failed to create regular user:', error);
+ throw error;
+ }
}Also applies to: 164-206, 208-223
| }); | ||
| }); | ||
|
|
||
| suite("Required Feilds", () => { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix typo in suite name.
Correct the suite name from "Required Feilds" to "Required Fields".
-suite("Required Feilds", () => {
+suite("Required Fields", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suite("Required Feilds", () => { | |
| suite("Required Fields", () => { |
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
|
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
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/routes/graphql/Query/fund.test.ts (1)
298-358: Move duplicate test cases into their respective suites.These test cases are duplicates of tests within the "Query field fund" suite. Consider moving them into the appropriate suite to maintain better organization and avoid redundancy.
Apply this diff to reorganize the tests:
-test("returns fund data if user is an admin", async () => { - // ... test implementation ... -}); - -test("returns fund data if user is organization member", async () => { - // ... test implementation ... -}); +suite("Query field fund", () => { + // ... existing tests ... + + test("returns fund data if user is an admin", async () => { + // ... test implementation ... + }); + + test("returns fund data if user is organization member", async () => { + // ... test implementation ... + }); +});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.suite("Query field fund", () => { // ... existing tests ... test("returns fund data if user is an admin", async () => { const adminSignInResult = await mercuriusClient.query(Query_signIn, { variables: { input: { emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, }, }, }); assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); const { fundId } = await createFund(); const fundResult = await mercuriusClient.query(Query_fund, { headers: { authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, }, variables: { input: { id: fundId, }, }, }); expect(fundResult.errors).toBeUndefined(); expect(fundResult.data.fund).toEqual( expect.objectContaining({ id: fundId, isTaxDeductible: expect.any(Boolean), name: expect.any(String), }), ); }); test("returns fund data if user is organization member", async () => { const regularUserResult = await createRegularUser(); const { fundId, orgId } = await createFund(); await addUserToOrg(regularUserResult.userId, orgId); const fundResult = await mercuriusClient.query(Query_fund, { headers: { authorization: `bearer ${regularUserResult.authToken}`, }, variables: { input: { id: fundId, }, }, }); expect(fundResult.errors).toBeUndefined(); expect(fundResult.data.fund).toEqual( expect.objectContaining({ id: fundId, isTaxDeductible: expect.any(Boolean), name: expect.any(String), }), ); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/routes/graphql/Query/fund.test.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 (1)
test/routes/graphql/Query/fund.test.ts (1)
404-409: Verify if whitespace-only fund names should be allowed.The test expects whitespace-only names to be valid, but this might lead to confusion and display issues. Consider if such names should be rejected or trimmed.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for any validation or display logic related to fund names ast-grep --pattern 'name: $name'Length of output: 34
I'm not seeing any evidence of additional validation logic for trimming or rejecting whitespace-only fund names in the codebase. However, it's not clear whether this behavior is intentional. It would be helpful to verify the definition of
fundsTableInsertSchema(likely defined using a schema validation library like zod) to see if any trimming or non-empty checks are applied to the "name" field.Could you please confirm whether the business requirements allow whitespace-only names or if they should be rejected/trimmed? If whitespace-only names are undesired, you might need to add trimming or a custom validation rule to the schema.
| try { | ||
| // Add cleanup mutation here when available | ||
| // await mercuriusClient.mutate(Mutation_deleteUser, { | ||
| // headers: { authorization: `bearer ${adminAuthToken}` }, | ||
| // variables: { input: { id: userId } } | ||
| // }); | ||
| console.log(`Cleanup: User ${userId} would be deleted here`); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implement cleanup functionality.
The cleanup functions are defined but not implemented, which could lead to test data accumulation. Consider implementing the cleanup mutations or adding a note about when they will be implemented.
Would you like me to help implement the cleanup mutations or create an issue to track this task?
| const RETRY_OPTIONS = { | ||
| maxRetries: 3, | ||
| delay: 1000, // ms | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider implementing exponential backoff for retries.
The current retry mechanism uses a fixed delay. Consider implementing exponential backoff to better handle transient failures.
Apply this diff to implement exponential backoff:
const RETRY_OPTIONS = {
maxRetries: 3,
- delay: 1000, // ms
+ initialDelay: 1000, // ms
+ maxDelay: 5000, // ms
};Then update the retry function:
async function retry<T>(
operation: () => Promise<T>,
options = RETRY_OPTIONS,
): Promise<T> {
let lastError: Error | undefined;
for (let attempt = 1; attempt <= options.maxRetries; attempt++) {
try {
return await operation();
} catch (error) {
lastError = error as Error;
if (attempt < options.maxRetries) {
- await new Promise((resolve) => setTimeout(resolve, options.delay));
+ const delay = Math.min(
+ options.initialDelay * Math.pow(2, attempt - 1),
+ options.maxDelay
+ );
+ await new Promise((resolve) => setTimeout(resolve, delay));
console.warn(`Retry attempt ${attempt} after error:`, error);
}
}
}
throw lastError;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const RETRY_OPTIONS = { | |
| maxRetries: 3, | |
| delay: 1000, // ms | |
| }; | |
| const RETRY_OPTIONS = { | |
| maxRetries: 3, | |
| initialDelay: 1000, // ms | |
| maxDelay: 5000, // ms | |
| }; | |
| async function retry<T>( | |
| operation: () => Promise<T>, | |
| options = RETRY_OPTIONS, | |
| ): Promise<T> { | |
| let lastError: Error | undefined; | |
| for (let attempt = 1; attempt <= options.maxRetries; attempt++) { | |
| try { | |
| return await operation(); | |
| } catch (error) { | |
| lastError = error as Error; | |
| if (attempt < options.maxRetries) { | |
| const delay = Math.min( | |
| options.initialDelay * Math.pow(2, attempt - 1), | |
| options.maxDelay | |
| ); | |
| await new Promise((resolve) => setTimeout(resolve, delay)); | |
| console.warn(`Retry attempt ${attempt} after error:`, error); | |
| } | |
| } | |
| } | |
| throw lastError; | |
| } |
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
|
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 (1)
test/routes/graphql/Query/fund.test.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 (2)
test/routes/graphql/Query/fund.test.ts (2)
39-41: Well-organized test structure!The test suites are logically organized with clear separation of concerns:
- Error cases for unauthorized access
- Success cases for authorized users
- Schema validation
- UUID validation
- Required fields validation
Also applies to: 180-181, 243-244, 383-384, 460-461
243-524: Excellent validation test coverage!The validation tests thoroughly cover:
- Name constraints (length, special chars, Unicode)
- UUID format and version validation
- Required fields and their constraints
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.data.fund).toEqual(null); | ||
| expect(fundResult.errors).toEqual( | ||
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | ||
| expect.objectContaining<TalawaGraphQLFormattedError>({ | ||
| extensions: expect.objectContaining<UnauthenticatedExtensions>({ | ||
| code: "unauthenticated", | ||
| }), | ||
| message: expect.any(String), | ||
| path: ["fund"], | ||
| }), | ||
| ]), | ||
| ); | ||
| }); | ||
|
|
||
| test("with 'arguments_associated_resources_not_found' extensions code if fund not found", async () => { | ||
| const adminSignInResult = await mercuriusClient.query(Query_signIn, { | ||
| variables: { | ||
| input: { | ||
| emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, | ||
| password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: faker.string.uuid(), | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.data.fund).toEqual(null); | ||
| expect(fundResult.errors).toEqual( | ||
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | ||
| expect.objectContaining<TalawaGraphQLFormattedError>({ | ||
| extensions: | ||
| expect.objectContaining<ArgumentsAssociatedResourcesNotFoundExtensions>( | ||
| { | ||
| code: "arguments_associated_resources_not_found", | ||
| issues: [ | ||
| { | ||
| argumentPath: ["input", "id"], | ||
| }, | ||
| ], | ||
| }, | ||
| ), | ||
| message: expect.any(String), | ||
| path: ["fund"], | ||
| }), | ||
| ]), | ||
| ); | ||
| }); | ||
|
|
||
| test("with 'unauthorized_action_on_arguments_associated_resources' if non-admin user is not a member of fund's organization", async () => { | ||
| const regularUserResult = await createRegularUser(); | ||
| const { fundId } = await createFund(); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${regularUserResult.authToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.data.fund).toEqual(null); | ||
| expect(fundResult.errors).toEqual( | ||
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | ||
| expect.objectContaining<TalawaGraphQLFormattedError>({ | ||
| extensions: | ||
| expect.objectContaining<UnauthorizedActionOnArgumentsAssociatedResourcesExtensions>( | ||
| { | ||
| code: "unauthorized_action_on_arguments_associated_resources", | ||
| issues: [ | ||
| { | ||
| argumentPath: ["input", "id"], | ||
| }, | ||
| ], | ||
| }, | ||
| ), | ||
| message: expect.any(String), | ||
| path: ["fund"], | ||
| }), | ||
| ]), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding more error scenarios.
While the error handling is comprehensive, consider adding these edge cases:
- Malformed authorization token
- Expired authorization token
- Rate limiting scenarios
| test("returns fund data if user is organization member", async () => { | ||
| const regularUserResult = await createRegularUser(); | ||
| const { fundId, orgId } = await createFund(); | ||
|
|
||
| await addUserToOrg(regularUserResult.userId, orgId); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${regularUserResult.authToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.errors).toBeUndefined(); | ||
| expect(fundResult.data.fund).toEqual( | ||
| expect.objectContaining({ | ||
| id: fundId, | ||
| isTaxDeductible: expect.any(Boolean), | ||
| name: expect.any(String), | ||
| }), | ||
| ); | ||
| }); | ||
|
|
||
| test("returns fund data if user is an admin", async () => { | ||
| const adminSignInResult = await mercuriusClient.query(Query_signIn, { | ||
| variables: { | ||
| input: { | ||
| emailAddress: server.envConfig.API_ADMINISTRATOR_USER_EMAIL_ADDRESS, | ||
| password: server.envConfig.API_ADMINISTRATOR_USER_PASSWORD, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| assertToBeNonNullish(adminSignInResult.data.signIn?.authenticationToken); | ||
|
|
||
| const { fundId } = await createFund(); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${adminSignInResult.data.signIn.authenticationToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.errors).toBeUndefined(); | ||
| expect(fundResult.data.fund).toEqual( | ||
| expect.objectContaining({ | ||
| id: fundId, | ||
| isTaxDeductible: expect.any(Boolean), | ||
| name: expect.any(String), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding more success scenarios.
While the basic success cases are covered, consider adding tests for:
- Funds with null/empty optional fields
- Funds with maximum length names
- Multiple funds in the same organization
| const orgId = createOrgResult.data.createOrganization.id; | ||
|
|
||
| // Create fund | ||
| const createFundResult = await mercuriusClient.mutate( | ||
| Mutation_createFund, | ||
| { | ||
| headers: { | ||
| authorization: `bearer ${adminAuthToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| name: `Fund ${faker.string.uuid()}`, | ||
| organizationId: orgId, | ||
| isTaxDeductible: false, | ||
| }, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (!createFundResult.data?.createFund?.id) { | ||
| throw new Error("Failed to create fund: Missing fund ID"); | ||
| } | ||
|
|
||
| const fundId = createFundResult.data.createFund.id; | ||
|
|
||
| return { | ||
| fundId, | ||
| orgId, | ||
| cleanup: async () => { | ||
| try { | ||
| // Add cleanup mutations here when available | ||
| // First delete fund, then organization | ||
| // await mercuriusClient.mutate(Mutation_deleteFund, { | ||
| // headers: { authorization: `bearer ${adminAuthToken}` }, | ||
| // variables: { input: { id: fundId } } | ||
| // }); | ||
| // await mercuriusClient.mutate(Mutation_deleteOrganization, { | ||
| // headers: { authorization: `bearer ${adminAuthToken}` }, | ||
| // variables: { input: { id: orgId } } | ||
| // }); | ||
| console.log( | ||
| `Cleanup: Fund ${fundId} and Org ${orgId} would be deleted here`, | ||
| ); | ||
| } catch (error) { | ||
| console.error("Failed to cleanup fund and organization:", error); | ||
| throw error; | ||
| } | ||
| }, | ||
| }; | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to create fund:", error); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| async function addUserToOrg( | ||
| userId: string, | ||
| orgId: string, | ||
| ): Promise<{ cleanup: () => Promise<void> }> { | ||
| try { | ||
| return await retry(async () => { | ||
| const adminAuthToken = await getAdminAuthToken(); | ||
|
|
||
| await mercuriusClient.mutate(Mutation_createOrganizationMembership, { | ||
| headers: { | ||
| authorization: `bearer ${adminAuthToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| memberId: userId, | ||
| organizationId: orgId, | ||
| role: "regular", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| return { | ||
| cleanup: async () => { | ||
| try { | ||
| // Add cleanup mutation here when available | ||
| // await mercuriusClient.mutate(Mutation_deleteOrganizationMembership, { | ||
| // headers: { authorization: `bearer ${adminAuthToken}` }, | ||
| // variables: { input: { memberId: userId, organizationId: orgId } } | ||
| // }); | ||
| console.log( | ||
| `Cleanup: Membership for user ${userId} in org ${orgId} would be deleted here`, | ||
| ); | ||
| } catch (error) { | ||
| console.error("Failed to cleanup organization membership:", error); | ||
| throw error; | ||
| } | ||
| }, | ||
| }; | ||
| }); | ||
| } catch (error) { | ||
| console.error("Failed to add user to organization:", error); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implement cleanup functionality.
The cleanup functions are defined but not implemented. This could lead to test data accumulation. Consider implementing the cleanup mutations or adding a note about when they will be implemented.
Would you like me to help implement the cleanup mutations or create an issue to track this task?
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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
test/routes/graphql/Query/fund.test.ts (3)
749-757: Implement cleanup functionality.The cleanup functions are defined but not implemented. This could lead to test data accumulation.
Would you like me to help implement the cleanup mutations or create an issue to track this task?
683-712: Consider implementing exponential backoff for retries.The current retry mechanism uses a fixed delay. Consider implementing exponential backoff to better handle transient failures.
Apply this diff to implement exponential backoff:
const RETRY_OPTIONS = { maxRetries: 3, - initialDelay: 1000, // ms - maxDelay: 5000, // ms + initialDelay: 100, // ms + maxDelay: 1000, // ms + backoffFactor: 2, };
603-603: Fix typo in suite name.Correct the suite name from "Required Fields" to "Required Field Validation".
-suite("Required Fields", () => { +suite("Required Field Validation", () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/routes/graphql/Query/fund.test.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
test/routes/graphql/Query/fund.test.ts
209-210: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
210-210: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 GitHub Check: CodeQL
test/routes/graphql/Query/fund.test.ts
[failure] 184-184: Hard-coded credentials
The hard-coded value "malformed_token" is used as authorization header.
[failure] 210-210: Hard-coded credentials
The hard-coded value "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9.DlOeaAiW9JJthDkhcLvMoJqFWZkiKnYAg4gXxHl6HN4" is used as authorization header.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CodeQL
- GitHub Check: Analyse Code With CodeQL (typescript)
| test("validates required fund fields", () => { | ||
| const validInput = { | ||
| name: "Test Fund", | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }; | ||
|
|
||
| // Test missing name | ||
| const missingName = { ...validInput, name: undefined }; | ||
| expect(fundsTableInsertSchema.safeParse(missingName).success).toBe(false); | ||
|
|
||
| // Test missing isTaxDeductible | ||
| const missingTaxStatus = { ...validInput, isTaxDeductible: undefined }; | ||
| expect(fundsTableInsertSchema.safeParse(missingTaxStatus).success).toBe( | ||
| false, | ||
| ); | ||
|
|
||
| // Test missing organizationId | ||
| const missingOrgId = { ...validInput, organizationId: undefined }; | ||
| expect(fundsTableInsertSchema.safeParse(missingOrgId).success).toBe(false); | ||
|
|
||
| // Valid data should pass | ||
| expect(fundsTableInsertSchema.safeParse(validInput).success).toBe(true); | ||
| }); | ||
|
|
||
| test("validates fund field constraints", () => { | ||
| const validInput = { | ||
| name: "Test Fund", | ||
| isTaxDeductible: false, | ||
| organizationId: faker.string.uuid(), | ||
| }; | ||
|
|
||
| // Test name constraints | ||
| expect( | ||
| fundsTableInsertSchema.safeParse({ | ||
| ...validInput, | ||
| name: "", | ||
| }).success, | ||
| ).toBe(false); | ||
|
|
||
| expect( | ||
| fundsTableInsertSchema.safeParse({ | ||
| ...validInput, | ||
| name: "a".repeat(257), | ||
| }).success, | ||
| ).toBe(false); | ||
|
|
||
| // Test isTaxDeductible must be boolean | ||
| expect( | ||
| fundsTableInsertSchema.safeParse({ | ||
| ...validInput, | ||
| isTaxDeductible: "true", | ||
| }).success, | ||
| ).toBe(false); | ||
|
|
||
| // Test organizationId must be UUID | ||
| expect( | ||
| fundsTableInsertSchema.safeParse({ | ||
| ...validInput, | ||
| organizationId: "invalid-uuid", | ||
| }).success, | ||
| ).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance test descriptions for clarity.
The test descriptions could be more descriptive to better communicate their purpose.
-test("validates required fund fields", () => {
+test("validates that all required fund fields (name, isTaxDeductible, organizationId) are present", () => {
-test("validates fund field constraints", () => {
+test("validates field constraints: name length, isTaxDeductible type, and organizationId format", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("validates required fund fields", () => { | |
| const validInput = { | |
| name: "Test Fund", | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }; | |
| // Test missing name | |
| const missingName = { ...validInput, name: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingName).success).toBe(false); | |
| // Test missing isTaxDeductible | |
| const missingTaxStatus = { ...validInput, isTaxDeductible: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingTaxStatus).success).toBe( | |
| false, | |
| ); | |
| // Test missing organizationId | |
| const missingOrgId = { ...validInput, organizationId: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingOrgId).success).toBe(false); | |
| // Valid data should pass | |
| expect(fundsTableInsertSchema.safeParse(validInput).success).toBe(true); | |
| }); | |
| test("validates fund field constraints", () => { | |
| const validInput = { | |
| name: "Test Fund", | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }; | |
| // Test name constraints | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| name: "", | |
| }).success, | |
| ).toBe(false); | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| name: "a".repeat(257), | |
| }).success, | |
| ).toBe(false); | |
| // Test isTaxDeductible must be boolean | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| isTaxDeductible: "true", | |
| }).success, | |
| ).toBe(false); | |
| // Test organizationId must be UUID | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| organizationId: "invalid-uuid", | |
| }).success, | |
| ).toBe(false); | |
| }); | |
| }); | |
| test("validates that all required fund fields (name, isTaxDeductible, organizationId) are present", () => { | |
| const validInput = { | |
| name: "Test Fund", | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }; | |
| // Test missing name | |
| const missingName = { ...validInput, name: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingName).success).toBe(false); | |
| // Test missing isTaxDeductible | |
| const missingTaxStatus = { ...validInput, isTaxDeductible: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingTaxStatus).success).toBe(false); | |
| // Test missing organizationId | |
| const missingOrgId = { ...validInput, organizationId: undefined }; | |
| expect(fundsTableInsertSchema.safeParse(missingOrgId).success).toBe(false); | |
| // Valid data should pass | |
| expect(fundsTableInsertSchema.safeParse(validInput).success).toBe(true); | |
| }); | |
| test("validates field constraints: name length, isTaxDeductible type, and organizationId format", () => { | |
| const validInput = { | |
| name: "Test Fund", | |
| isTaxDeductible: false, | |
| organizationId: faker.string.uuid(), | |
| }; | |
| // Test name constraints | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| name: "", | |
| }).success, | |
| ).toBe(false); | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| name: "a".repeat(257), | |
| }).success, | |
| ).toBe(false); | |
| // Test isTaxDeductible must be boolean | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| isTaxDeductible: "true", | |
| }).success, | |
| ).toBe(false); | |
| // Test organizationId must be UUID | |
| expect( | |
| fundsTableInsertSchema.safeParse({ | |
| ...validInput, | |
| organizationId: "invalid-uuid", | |
| }).success, | |
| ).toBe(false); | |
| }); | |
| }); |
| test("with 'unauthenticated' extensions code if authorization token is malformed", async () => { | ||
| const fundId = faker.string.uuid(); | ||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: "malformed_token", | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.data.fund).toEqual(null); | ||
| expect(fundResult.errors).toEqual( | ||
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | ||
| expect.objectContaining<TalawaGraphQLFormattedError>({ | ||
| extensions: expect.objectContaining<UnauthenticatedExtensions>({ | ||
| code: "unauthenticated", | ||
| }), | ||
| message: expect.any(String), | ||
| path: ["fund"], | ||
| }), | ||
| ]), | ||
| ); | ||
| }); | ||
|
|
||
| test("with 'unauthenticated' extensions code if authorization token is expired", async () => { | ||
| const fundId = faker.string.uuid(); | ||
| const expiredToken = | ||
| "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9.DlOeaAiW9JJthDkhcLvMoJqFWZkiKnYAg4gXxHl6HN4"; | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${expiredToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.data.fund).toEqual(null); | ||
| expect(fundResult.errors).toEqual( | ||
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | ||
| expect.objectContaining<TalawaGraphQLFormattedError>({ | ||
| extensions: expect.objectContaining<UnauthenticatedExtensions>({ | ||
| code: "unauthenticated", | ||
| }), | ||
| message: expect.any(String), | ||
| path: ["fund"], | ||
| }), | ||
| ]), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document test tokens and their purpose.
The test uses hard-coded tokens for testing error cases. While this is acceptable for tests, it's good practice to document their purpose.
Add comments to explain the test tokens:
test("with 'unauthenticated' extensions code if authorization token is malformed", async () => {
const fundId = faker.string.uuid();
const fundResult = await mercuriusClient.query(Query_fund, {
headers: {
+ // Using a malformed token to test error handling
authorization: "malformed_token",
},
variables: {
input: {
id: fundId,
},
},
});
// ... rest of the test
});
test("with 'unauthenticated' extensions code if authorization token is expired", async () => {
const fundId = faker.string.uuid();
+ // Using an expired JWT for testing (exp: 1)
const expiredToken =
"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9.DlOeaAiW9JJthDkhcLvMoJqFWZkiKnYAg4gXxHl6HN4";
// ... rest of the test
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("with 'unauthenticated' extensions code if authorization token is malformed", async () => { | |
| const fundId = faker.string.uuid(); | |
| const fundResult = await mercuriusClient.query(Query_fund, { | |
| headers: { | |
| authorization: "malformed_token", | |
| }, | |
| variables: { | |
| input: { | |
| id: fundId, | |
| }, | |
| }, | |
| }); | |
| expect(fundResult.data.fund).toEqual(null); | |
| expect(fundResult.errors).toEqual( | |
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | |
| expect.objectContaining<TalawaGraphQLFormattedError>({ | |
| extensions: expect.objectContaining<UnauthenticatedExtensions>({ | |
| code: "unauthenticated", | |
| }), | |
| message: expect.any(String), | |
| path: ["fund"], | |
| }), | |
| ]), | |
| ); | |
| }); | |
| test("with 'unauthenticated' extensions code if authorization token is expired", async () => { | |
| const fundId = faker.string.uuid(); | |
| const expiredToken = | |
| "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9.DlOeaAiW9JJthDkhcLvMoJqFWZkiKnYAg4gXxHl6HN4"; | |
| const fundResult = await mercuriusClient.query(Query_fund, { | |
| headers: { | |
| authorization: `bearer ${expiredToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| id: fundId, | |
| }, | |
| }, | |
| }); | |
| expect(fundResult.data.fund).toEqual(null); | |
| expect(fundResult.errors).toEqual( | |
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | |
| expect.objectContaining<TalawaGraphQLFormattedError>({ | |
| extensions: expect.objectContaining<UnauthenticatedExtensions>({ | |
| code: "unauthenticated", | |
| }), | |
| message: expect.any(String), | |
| path: ["fund"], | |
| }), | |
| ]), | |
| ); | |
| }); | |
| test("with 'unauthenticated' extensions code if authorization token is malformed", async () => { | |
| const fundId = faker.string.uuid(); | |
| const fundResult = await mercuriusClient.query(Query_fund, { | |
| headers: { | |
| // Using a malformed token to test error handling | |
| authorization: "malformed_token", | |
| }, | |
| variables: { | |
| input: { | |
| id: fundId, | |
| }, | |
| }, | |
| }); | |
| expect(fundResult.data.fund).toEqual(null); | |
| expect(fundResult.errors).toEqual( | |
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | |
| expect.objectContaining<TalawaGraphQLFormattedError>({ | |
| extensions: expect.objectContaining<UnauthenticatedExtensions>({ | |
| code: "unauthenticated", | |
| }), | |
| message: expect.any(String), | |
| path: ["fund"], | |
| }), | |
| ]), | |
| ); | |
| }); | |
| test("with 'unauthenticated' extensions code if authorization token is expired", async () => { | |
| const fundId = faker.string.uuid(); | |
| // Using an expired JWT for testing (exp: 1) | |
| const expiredToken = | |
| "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9.DlOeaAiW9JJthDkhcLvMoJqFWZkiKnYAg4gXxHl6HN4"; | |
| const fundResult = await mercuriusClient.query(Query_fund, { | |
| headers: { | |
| authorization: `bearer ${expiredToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| id: fundId, | |
| }, | |
| }, | |
| }); | |
| expect(fundResult.data.fund).toEqual(null); | |
| expect(fundResult.errors).toEqual( | |
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | |
| expect.objectContaining<TalawaGraphQLFormattedError>({ | |
| extensions: expect.objectContaining<UnauthenticatedExtensions>({ | |
| code: "unauthenticated", | |
| }), | |
| message: expect.any(String), | |
| path: ["fund"], | |
| }), | |
| ]), | |
| ); | |
| }); |
🧰 Tools
🪛 Gitleaks (8.21.2)
209-210: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
210-210: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 GitHub Check: CodeQL
[failure] 184-184: Hard-coded credentials
The hard-coded value "malformed_token" is used as authorization header.
[failure] 210-210: Hard-coded credentials
The hard-coded value "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwiaWF0IjoxNTE2MjM5MDIyLCJleHAiOjF9.DlOeaAiW9JJthDkhcLvMoJqFWZkiKnYAg4gXxHl6HN4" is used as authorization header.
| test("returns fund with expected fields", async () => { | ||
| const adminAuthToken = await getAdminAuthToken(); | ||
| const { fundId } = await createFund(); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { authorization: `bearer ${adminAuthToken}` }, | ||
| variables: { input: { id: fundId } }, | ||
| }); | ||
|
|
||
| expect(fundResult.errors).toBeUndefined(); | ||
| expect(fundResult.data?.fund).toMatchObject({ | ||
| id: fundId, | ||
| isTaxDeductible: false, | ||
| name: expect.any(String), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance fund fields test coverage.
The current test only verifies basic fields. Consider adding tests for:
- Null/empty optional fields
- Fields with special characters
- Fields with maximum length values
test("returns fund with expected fields", async () => {
const adminAuthToken = await getAdminAuthToken();
- const { fundId } = await createFund();
+
+ // Create funds with various field values
+ const funds = await Promise.all([
+ createFund(), // Default fund
+ createFundWithMaxLengthName(),
+ createFundWithSpecialChars(),
+ ]);
- const fundResult = await mercuriusClient.query(Query_fund, {
- headers: { authorization: `bearer ${adminAuthToken}` },
- variables: { input: { id: fundId } },
- });
+ // Test each fund
+ for (const { fundId } of funds) {
+ const fundResult = await mercuriusClient.query(Query_fund, {
+ headers: { authorization: `bearer ${adminAuthToken}` },
+ variables: { input: { id: fundId } },
+ });
- expect(fundResult.errors).toBeUndefined();
- expect(fundResult.data?.fund).toMatchObject({
- id: fundId,
- isTaxDeductible: false,
- name: expect.any(String),
- });
+ expect(fundResult.errors).toBeUndefined();
+ expect(fundResult.data?.fund).toMatchObject({
+ id: expect.any(String),
+ isTaxDeductible: expect.any(Boolean),
+ name: expect.any(String),
+ });
+ }
});Committable suggestion skipped: line range outside the PR's diff.
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
|
|
Please fix the failing tests |
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
|
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
test/routes/graphql/Query/fund.test.ts(1 hunks)test/routes/graphql/documentNodes.ts(1 hunks)test/routes/graphql/gql.tada-cache.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 (25)
test/routes/graphql/gql.tada-cache.d.ts (5)
33-34: Good addition of Query_fund.
Nicely typed return object for the query. Just ensure that the consumer of this query properly handles the possibility ofisTaxDeductibleornamebeingnull.
35-36: Mutation_createOrganization looks consistent.
The returned fields accurately reflect the creation response. Verify whether additional organization fields (like address or metadata) need to be returned for your use case.
37-38: Mutation_createFund is aligned, but double-check nullability.
Be sure to confirm whetherisTaxDeductiblecan benullin every case or if it should always be a boolean. This helps avoid confusion in front-end handling.
41-42: Deletion mutation for Fund looks good.
The typed structure lines up with the rest of the schema for fund deletion.
45-46: Deletion mutation for OrganizationMembership is appropriate.
The typed structure for returningid,name, andcountryCodeis consistent with the GraphQL definitions.test/routes/graphql/documentNodes.ts (5)
310-317: New Query_fund definition is consistent.
Fetchingid,isTaxDeductible, andnamealigns with the usage in tests.
328-336: Mutation_createFund is coherent with the fund structure.
Ensure consumers of this mutation handle partial or null values gracefully.
344-352: Mutation_deleteFund is appropriately typed.
No issues spotted. The name, id, andisTaxDeductiblefields match the new fund schema.
353-361: Mutation_deleteOrganization is straightforward.
Looks good for confirming which organization was removed.
362-370: Mutation_deleteOrganizationMembership.
The returned fields (id, name, countryCode) appear to align with your membership object shape.test/routes/graphql/Query/fund.test.ts (15)
1-28: Imports and type references are well-organized.
They provide good modularity without extraneous references.
29-41: getAdminAuthToken function ensures admin credentials are fetched just-in-time.
This is a good approach to reduce repeated code, but verify that environment variables exist and are validated.
43-182: Robust negative test coverage in 'Query field fund' suite.
You're checking multiple error states (unauthenticated, resource not found, unauthorized, etc.). This thoroughly ensures correct GraphQL error handling.
184-214: Malformed authorization token test.
Generating a random invalid token is a secure practice to avoid storing real secrets in tests.
216-253: Expired token test logic is clear.
Constructing an expired JWT payload in-line is a neat approach to test exact expiration times.
281-306: Fund retrieval tests validate membership-based access.
They confirm correct fund data for properly authorized regular users. Nicely done.
308-341: Admin-based fund retrieval is similarly validated.
Ensures that the admin role bypasses membership constraints.
343-358: Fund fields verification test.
Checks essential properties of the fund object. This complements earlier negative tests nicely.
360-401: Maximum length fund name test.
Great approach to validate boundary conditions around name length.
404-476: Fund name constraints and trimming checks.
Good coverage of short, long, special chars, whitespace, and trimming. This ensures robust validation logic.
478-541: Unique constraint on fund name.
Properly tests duplicates within the same organization. Confirm that theforbidden_action_on_arguments_associated_resourceserror aligns with your desired semantic for duplicates.
544-619: UUID Validation suite thoroughly checks multiple scenarios.
Covers invalid formats, case sensitivity, and incorrect versions. Excellent coverage for your new v7 approach.
621-646: Required fields test.
Validates that all fields are present and flags any omissions. This is consistent with your schema-level requirements.
647-684: Additional field constraints test.
Ensures invalid types and constraints are rejected. This pairs well with the earlier name constraint checks.
687-933: Helper functions for test setup and cleanup.
Exponential backoff inretryis a nice addition. ThecreateRegularUser,createFund, andaddUserToOrgpatterns promote DRY code.
| "mutation Mutation_createOrganizationMembership($input: MutationCreateOrganizationMembershipInput!) {\n createOrganizationMembership(input: $input) {\n id\n }\n }": | ||
| TadaDocumentNode<{ createOrganizationMembership: { id: string; } | null; }, { input: { role?: "administrator" | "regular" | null | undefined; organizationId: string; memberId: string; }; }, void>; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider returning more membership fields if needed.
Currently, the membership payload only includes id. If your tests or UX require more membership info (like roles, invitation status, etc.), you might expand this response.
| "mutation Mutation_deleteOrganization($input: MutationDeleteOrganizationInput!) {\n deleteOrganization(input: $input) {\n id\n name\n countryCode\n }\n}": | ||
| TadaDocumentNode<{ deleteOrganization: { id: string; name: string | null; countryCode: "at" | "pg" | "ad" | "ae" | "af" | "ag" | "ai" | "al" | "am" | "ao" | "aq" | "ar" | "as" | "au" | "aw" | "ax" | "az" | "ba" | "bb" | "bd" | "be" | "bf" | "bg" | "bh" | "bi" | "bj" | "bl" | "bm" | "bn" | "bo" | "bq" | "br" | "bs" | "bt" | "bv" | "bw" | "by" | "bz" | "ca" | "cc" | "cd" | "cf" | "cg" | "ch" | "ci" | "ck" | "cl" | "cm" | "cn" | "co" | "cr" | "cu" | "cv" | "cw" | "cx" | "cy" | "cz" | "de" | "dj" | "dk" | "dm" | "do" | "dz" | "ec" | "ee" | "eg" | "eh" | "er" | "es" | "et" | "fi" | "fj" | "fk" | "fm" | "fo" | "fr" | "ga" | "gb" | "gd" | "ge" | "gf" | "gg" | "gh" | "gi" | "gl" | "gm" | "gn" | "gp" | "gq" | "gr" | "gs" | "gt" | "gu" | "gw" | "gy" | "hk" | "hm" | "hn" | "hr" | "ht" | "hu" | "id" | "ie" | "il" | "im" | "in" | "io" | "iq" | "ir" | "is" | "it" | "je" | "jm" | "jo" | "jp" | "ke" | "kg" | "kh" | "ki" | "km" | "kn" | "kp" | "kr" | "kw" | "ky" | "kz" | "la" | "lb" | "lc" | "li" | "lk" | "lr" | "ls" | "lt" | "lu" | "lv" | "ly" | "ma" | "mc" | "md" | "me" | "mf" | "mg" | "mh" | "mk" | "ml" | "mm" | "mn" | "mo" | "mp" | "mq" | "mr" | "ms" | "mt" | "mu" | "mv" | "mw" | "mx" | "my" | "mz" | "na" | "nc" | "ne" | "nf" | "ng" | "ni" | "nl" | "no" | "np" | "nr" | "nu" | "nz" | "om" | "pa" | "pe" | "pf" | "ph" | "pk" | "pl" | "pm" | "pn" | "pr" | "ps" | "pt" | "pw" | "py" | "qa" | "re" | "ro" | "rs" | "ru" | "rw" | "sa" | "sb" | "sc" | "sd" | "se" | "sg" | "sh" | "si" | "sj" | "sk" | "sl" | "sm" | "sn" | "so" | "sr" | "ss" | "st" | "sv" | "sx" | "sy" | "sz" | "tc" | "td" | "tf" | "tg" | "th" | "tj" | "tk" | "tl" | "tm" | "tn" | "to" | "tr" | "tt" | "tv" | "tw" | "tz" | "ua" | "ug" | "um" | "us" | "uy" | "uz" | "va" | "vc" | "ve" | "vg" | "vi" | "vn" | "vu" | "wf" | "ws" | "ye" | "yt" | "za" | "zm" | "zw" | null; } | null; }, { input: { id: string; }; }, void>; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Verify that only these fields are needed after deleting the Organization.
Sometimes, you may want to confirm success or keep track of timestamps. As it stands, this is fine if minimal info is sufficient.
| export const Mutation_createOrganization = | ||
| gql(`mutation Mutation_createOrganization($input: MutationCreateOrganizationInput!) { | ||
| createOrganization(input: $input) { | ||
| id | ||
| name | ||
| countryCode | ||
| } | ||
| }`); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mutation_createOrganization is well-defined.
The new organization creation fields match typical organization data. Consider whether you need additional organization metadata (like addressLine1, addressLine2) returned.
| export const Mutation_createOrganizationMembership = | ||
| gql(`mutation Mutation_createOrganizationMembership($input: MutationCreateOrganizationMembershipInput!) { | ||
| createOrganizationMembership(input: $input) { | ||
| id | ||
| } | ||
| }`); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mutation_createOrganizationMembership has a minimal response.
You may eventually want to return the organization name or membership role for clarity, but this is sufficient for basic creation flow.
| test("with 'too_many_requests' extensions code when rate limit is exceeded", async () => { | ||
| const fundId = faker.string.uuid(); | ||
| const adminAuthToken = await getAdminAuthToken(); | ||
|
|
||
| const results = await Promise.all( | ||
| Array.from({ length: 10 }, () => | ||
| mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${adminAuthToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }), | ||
| ), | ||
| ); | ||
|
|
||
| const lastResult = results.at(-1); | ||
| expect(lastResult?.errors).toBeDefined(); | ||
| expect(lastResult?.errors?.[0]?.extensions?.code).toBe( | ||
| "arguments_associated_resources_not_found", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Mismatch between test description and expected error code.
You describe expecting "too_many_requests", but the assertion checks for "arguments_associated_resources_not_found". This is contradictory.
Proposed fix:
- expect(lastResult?.errors?.[0]?.extensions?.code).toBe(
- "arguments_associated_resources_not_found",
- );
+ expect(lastResult?.errors?.[0]?.extensions?.code).toBe(
+ "too_many_requests",
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("with 'too_many_requests' extensions code when rate limit is exceeded", async () => { | |
| const fundId = faker.string.uuid(); | |
| const adminAuthToken = await getAdminAuthToken(); | |
| const results = await Promise.all( | |
| Array.from({ length: 10 }, () => | |
| mercuriusClient.query(Query_fund, { | |
| headers: { | |
| authorization: `bearer ${adminAuthToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| id: fundId, | |
| }, | |
| }, | |
| }), | |
| ), | |
| ); | |
| const lastResult = results.at(-1); | |
| expect(lastResult?.errors).toBeDefined(); | |
| expect(lastResult?.errors?.[0]?.extensions?.code).toBe( | |
| "arguments_associated_resources_not_found", | |
| ); | |
| }); | |
| test("with 'too_many_requests' extensions code when rate limit is exceeded", async () => { | |
| const fundId = faker.string.uuid(); | |
| const adminAuthToken = await getAdminAuthToken(); | |
| const results = await Promise.all( | |
| Array.from({ length: 10 }, () => | |
| mercuriusClient.query(Query_fund, { | |
| headers: { | |
| authorization: `bearer ${adminAuthToken}`, | |
| }, | |
| variables: { | |
| input: { | |
| id: fundId, | |
| }, | |
| }, | |
| }), | |
| ), | |
| ); | |
| const lastResult = results.at(-1); | |
| expect(lastResult?.errors).toBeDefined(); | |
| expect(lastResult?.errors?.[0]?.extensions?.code).toBe( | |
| "too_many_requests", | |
| ); | |
| }); |
There was a problem hiding this comment.
This seems like a valid request
There was a problem hiding this comment.
@palisadoes Thank you for confirming! Yes, updating the assertion to expect "too_many_requests" will make the test consistent with its description.
(｀°ω°)⊃━☆ﾟ.*･｡ﾟ
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/routes/graphql/Query/fund.test.ts (1)
255-279:⚠️ Potential issueFix mismatch between test description and expected error code.
The test description suggests checking for rate limiting ("when rate limit is exceeded"), but the assertion checks for "arguments_associated_resources_not_found". This is contradictory.
Apply this diff to fix the assertion:
-expect(lastResult?.errors?.[0]?.extensions?.code).toBe( - "arguments_associated_resources_not_found", -); +expect(lastResult?.errors?.[0]?.extensions?.code).toBe( + "too_many_requests", +);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/routes/graphql/Query/fund.test.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 (8)
test/routes/graphql/Query/fund.test.ts (8)
1-41: LGTM! Well-structured setup with proper imports and helper functions.The setup section is well-organized with clear type imports and a focused helper function for admin authentication.
43-182: Consider adding more error scenarios.While the error handling is comprehensive, consider adding these edge cases:
- Malformed authorization token
- Expired authorization token
- Rate limiting scenarios
216-253: Improve expired token generation.The test generates an expired token correctly using a timestamp-based approach, which is better than using a hard-coded token.
281-358: Consider adding more success scenarios.While the basic success cases are covered, consider adding tests for:
- Funds with null/empty optional fields
- Funds with maximum length names
- Multiple funds in the same organization
404-542: LGTM! Comprehensive schema validation tests.The validation tests cover a wide range of scenarios including:
- Name length constraints
- Special characters and whitespace handling
- Unique constraint validation within organizations
544-619: LGTM! Thorough UUID validation tests.The tests comprehensively cover:
- UUID format validation
- Version constraints (v7)
- Case sensitivity
- Invalid formats and versions
621-685: LGTM! Well-structured required field validation tests.The tests thoroughly validate:
- Required fields presence
- Field type constraints
- Field format validation
687-932: LGTM! Well-implemented helper functions with robust error handling.The helper functions include:
- Retry mechanism with exponential backoff
- Proper error handling and logging
- Cleanup capabilities for test data
| test("with 'unauthenticated' extensions code if authorization token is malformed", async () => { | ||
| const fundId = faker.string.uuid(); | ||
| // Generate a random invalid token instead of using a hard-coded value | ||
| const invalidToken = Buffer.from(faker.string.alphanumeric(32)).toString( | ||
| "base64", | ||
| ); | ||
|
|
||
| const fundResult = await mercuriusClient.query(Query_fund, { | ||
| headers: { | ||
| authorization: `bearer ${invalidToken}`, | ||
| }, | ||
| variables: { | ||
| input: { | ||
| id: fundId, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(fundResult.data.fund).toEqual(null); | ||
| expect(fundResult.errors).toEqual( | ||
| expect.arrayContaining<TalawaGraphQLFormattedError>([ | ||
| expect.objectContaining<TalawaGraphQLFormattedError>({ | ||
| extensions: expect.objectContaining<UnauthenticatedExtensions>({ | ||
| code: "unauthenticated", | ||
| }), | ||
| message: expect.any(String), | ||
| path: ["fund"], | ||
| }), | ||
| ]), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Replace hard-coded token with dynamically generated one.
The test uses a hard-coded malformed token which was flagged by security analysis. Consider using a dynamically generated token instead.
The current implementation has been improved to use a randomly generated token, which is good practice.
There was a problem hiding this comment.
This is a random generated invalid token, won't be a problem. However, if you still need changes please recommend some good practices
There was a problem hiding this comment.
@yugal07 Your current implementation using faker.string.alphanumeric(32) with base64 encoding is already following good practices! However, if you'd like to make it even more robust, here are some additional suggestions:
// Option 1: Using crypto for stronger randomness
const cryptoToken = crypto.randomBytes(32).toString('base64');
// Option 2: Test suite covering various malformed scenarios
describe('malformed tokens', () => {
const cases = [
['empty token', ''],
['very long token', 'a'.repeat(1000)],
['non-base64', '!@#$%^&*()'],
['invalid JWT format', 'header.invalid'],
['random bytes', crypto.randomBytes(32).toString('base64')]
];
test.each(cases)('%s', async (_, token) => {
// Your test here using the token
});
});These are optional enhancements - your current implementation is already secure and follows testing best practices.
✏️ Learnings added
Learnt from: yugal07
PR: PalisadoesFoundation/talawa-api#3166
File: test/routes/graphql/Query/fund.test.ts:184-214
Timestamp: 2025-02-11T11:02:01.149Z
Learning: When testing invalid authentication tokens in tests, it's recommended to:
1. Use dynamically generated tokens instead of hard-coded values
2. Consider using crypto-safe random generation
3. Test various malformed scenarios (empty, very long, non-base64, invalid JWT format)
4. Use reasonable token lengths (e.g., 32 characters)
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@palisadoes I have implemented all of the coderaabbit suggestions and the remaining suggestion is not valid. Please have a look and let me know if there's need for more changes |
palisadoes
merged commit da81d6e
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Code coverage
Issue Number:
Fixes #3052
Summary
Added test cases for the file fund.ts
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
New Features
Tests