feat: supplement integration tests #2190
Conversation
jihun
changed the title
WalkthroughAdds many new integration tests across admin controllers; tightens DTO validation for webhooks and roles; changes CategoryNotFound to return 404; introduces WebhookNotFoundException and error-code; updates WebhookService to throw not-found errors on update/delete. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API
participant Validator as ValidationPipe
participant WebhookSvc as WebhookService
rect rgb(245,255,245)
note right of API: Create webhook — validation enforced
Client->>API: POST /admin/projects/:projectId/webhooks {name,url,events}
API->>Validator: validate(CreateWebhookRequestDto)
alt validation fails
Validator-->>API: 400 Bad Request
API-->>Client: 400
else
Validator-->>API: OK
API->>WebhookSvc: create(projectId, dto)
WebhookSvc-->>API: created
API-->>Client: 201 Created
end
end
sequenceDiagram
autonumber
participant Client
participant API
participant WebhookSvc as WebhookService
rect rgb(255,250,250)
note right of API: Update/Delete now return explicit 404 when missing
Client->>API: PUT/DELETE /admin/projects/:projectId/webhooks/:id
API->>WebhookSvc: findById(projectId, id)
WebhookSvc-->>API: null
API-->>Client: 404 Not Found (WebhookNotFoundException)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
apps/api/integration-test/test-specs/role.integration-spec.ts (1)
350-357: Consider replacing arbitrary delay with explicit async completion checks.The 500ms delay before closing the app is a workaround to ensure async operations complete. While common in integration tests, it can lead to flaky tests (too short) or unnecessarily slow tests (too long). If specific async operations need completion (e.g., database connection cleanup), consider waiting for those explicitly or using test framework hooks that handle this automatically.
apps/api/integration-test/test-specs/webhook.integration-spec.ts (1)
552-559: Consider replacing arbitrary delay with explicit async completion checks.The 500ms delay before closing the app is a workaround to ensure async operations complete. While common in integration tests, it can lead to flaky tests (too short) or unnecessarily slow tests (too long). If specific async operations need completion (e.g., database connection cleanup), consider waiting for those explicitly or using test framework hooks that handle this automatically.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/api/integration-test/test-specs/api-key.integration-spec.ts(1 hunks)apps/api/integration-test/test-specs/auth.integration-spec.ts(1 hunks)apps/api/integration-test/test-specs/category.integration-spec.ts(1 hunks)apps/api/integration-test/test-specs/channel.integration-spec.ts(1 hunks)apps/api/integration-test/test-specs/issue.integration-spec.ts(2 hunks)apps/api/integration-test/test-specs/member.integration-spec.ts(1 hunks)apps/api/integration-test/test-specs/role.integration-spec.ts(1 hunks)apps/api/integration-test/test-specs/webhook.integration-spec.ts(1 hunks)apps/api/src/domains/admin/project/category/exceptions/category-not-found.exception.ts(1 hunks)apps/api/src/domains/admin/project/webhook/dtos/requests/create-webhook-request.dto.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apps/api/integration-test/test-specs/member.integration-spec.ts (2)
apps/api/src/test-utils/util-functions.ts (2)
clearAllEntities(154-186)signInTestUser(196-208)apps/api/src/domains/admin/project/member/dtos/responses/get-all-members.dto.ts (1)
GetAllMemberResponseDto(76-87)
apps/api/integration-test/test-specs/channel.integration-spec.ts (1)
apps/api/src/domains/admin/channel/channel/dtos/requests/index.ts (3)
CreateChannelRequestDto(16-16)CreateChannelRequestFieldDto(18-18)UpdateChannelRequestDto(20-20)
apps/api/integration-test/test-specs/auth.integration-spec.ts (2)
apps/api/src/test-utils/util-functions.ts (1)
clearAllEntities(154-186)apps/api/src/domains/admin/auth/dtos/requests/email-user-sign-in-request.dto.ts (1)
EmailUserSignInRequestDto(21-29)
apps/api/integration-test/test-specs/api-key.integration-spec.ts (1)
apps/api/src/test-utils/util-functions.ts (2)
clearAllEntities(154-186)signInTestUser(196-208)
apps/api/integration-test/test-specs/role.integration-spec.ts (3)
apps/api/src/test-utils/util-functions.ts (2)
clearAllEntities(154-186)signInTestUser(196-208)apps/api/src/domains/admin/project/role/dtos/requests/create-role-request.dto.ts (1)
CreateRoleRequestDto(22-31)apps/api/src/domains/admin/project/role/dtos/responses/get-all-roles-response.dto.ts (2)
GetAllRolesResponseDto(39-54)GetAllRolesResponseRoleDto(21-37)
apps/api/integration-test/test-specs/webhook.integration-spec.ts (3)
apps/api/src/test-utils/util-functions.ts (2)
clearAllEntities(154-186)signInTestUser(196-208)apps/api/src/domains/admin/project/webhook/dtos/requests/create-webhook-request.dto.ts (1)
CreateWebhookRequestDto(24-47)apps/api/src/domains/admin/project/webhook/dtos/responses/get-webhook-by-id-response.dto.ts (1)
GetWebhookByIdResponseDto(48-80)
apps/api/integration-test/test-specs/category.integration-spec.ts (1)
apps/api/src/test-utils/util-functions.ts (2)
clearAllEntities(154-186)signInTestUser(196-208)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-test
🔇 Additional comments (8)
apps/api/integration-test/test-specs/role.integration-spec.ts (4)
58-97: LGTM!The test setup properly initializes the transactional context, bootstraps the NestJS application, clears existing data, and sets up the required tenant/project fixtures with authenticated user credentials.
173-213: LGTM!The GET endpoint tests appropriately verify listing functionality with search/pagination and unauthorized access scenarios.
215-294: LGTM!The PUT endpoint tests properly verify update functionality, validation constraints, and authorization requirements.
296-348: LGTM!The DELETE endpoint tests appropriately verify deletion functionality and unauthorized access scenarios.
apps/api/integration-test/test-specs/webhook.integration-spec.ts (4)
62-100: LGTM!The test setup properly initializes the testing infrastructure, clears data, and creates the necessary tenant/project fixtures with authenticated user credentials.
102-207: LGTM!The POST endpoint tests comprehensively verify webhook creation with proper validation for empty names, invalid URLs, empty events arrays, and unauthorized access. The expectations align with the validation decorators in CreateWebhookRequestDto (@isnotempty for name, @isurl for url).
209-260: LGTM!The GET list endpoint tests appropriately verify webhook listing with search/pagination and unauthorized access handling.
481-491: LGTM!The successful deletion test properly verifies that the webhook is removed and subsequent GET requests no longer find it.
apps/api/integration-test/test-specs/member.integration-spec.ts
Outdated
Show resolved
Hide resolved
apps/api/integration-test/test-specs/webhook.integration-spec.ts
Outdated
Show resolved
Hide resolved
apps/api/integration-test/test-specs/webhook.integration-spec.ts
Outdated
Show resolved
Hide resolved
apps/api/integration-test/test-specs/webhook.integration-spec.ts
Outdated
Show resolved
Hide resolved
apps/api/integration-test/test-specs/webhook.integration-spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/integration-test/test-specs/channel.integration-spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-test
🔇 Additional comments (1)
apps/api/integration-test/test-specs/channel.integration-spec.ts (1)
247-251: Authentication is applied before resource lookup via RequirePermission guard. Unauthorized DELETE requests will always return 401 as expected.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/domains/admin/project/role/dtos/requests/create-role-request.dto.ts (1)
17-31:@IsNotEmpty()does not validate array length—use@ArrayMinSize(1)instead.The
@IsNotEmpty()decorator on line 30 only checks that the value is notnull,undefined, or an empty string. An empty array[]will pass this validation, which likely contradicts the intent of requiring at least one permission when creating a role.Apply this diff to enforce a minimum array size:
-import { IsEnum, IsNotEmpty, IsString } from 'class-validator'; +import { ArrayMinSize, IsEnum, IsString } from 'class-validator';@ApiProperty() @IsEnum(PermissionEnum, { each: true }) @ArrayDistinct() - @IsNotEmpty() + @ArrayMinSize(1) permissions: PermissionEnum[];
♻️ Duplicate comments (3)
apps/api/integration-test/test-specs/webhook.integration-spec.ts (3)
286-300: GET by ID should return a single object, not an array.Align the test to expect a single DTO (no [0] indexing).
- const body = response.body as GetWebhookByIdResponseDto[]; - expect(response.body).toBeDefined(); - expect(body[0].id).toBe(webhookId); - expect(body[0].name).toBe('TestWebhookForGet'); - expect(body[0].url).toBe('https://example.com/webhook'); - expect(body[0].events).toHaveLength(1); - expect(body[0].status).toBe(WebhookStatusEnum.ACTIVE); - expect(body[0].createdAt).toBeDefined(); + const body = response.body as GetWebhookByIdResponseDto; + expect(body).toBeDefined(); + expect(body.id).toBe(webhookId); + expect(body.name).toBe('TestWebhookForGet'); + expect(body.url).toBe('https://example.com/webhook'); + expect(body.events).toHaveLength(1); + expect(body.status).toBe(WebhookStatusEnum.ACTIVE); + expect(body.createdAt).toBeDefined();- const body = response.body as GetWebhookByIdResponseDto[]; - expect(body[0].name).toBe('UpdatedTestWebhook'); - expect(body[0].url).toBe('https://updated-example.com/webhook'); - expect(body[0].events).toHaveLength(1); - expect(body[0].events[0].type).toBe(EventTypeEnum.FEEDBACK_CREATION); - expect(body[0].status).toBe(WebhookStatusEnum.ACTIVE); + const body = response.body as GetWebhookByIdResponseDto; + expect(body.name).toBe('UpdatedTestWebhook'); + expect(body.url).toBe('https://updated-example.com/webhook'); + expect(body.events).toHaveLength(1); + expect(body.events[0].type).toBe(EventTypeEnum.FEEDBACK_CREATION); + expect(body.status).toBe(WebhookStatusEnum.ACTIVE);Also applies to: 358-365
503-521: Unauthorized on test endpoint should be 401 (once implemented).The unauthorized case should assert 401, not 404.
- it('should return 404 when unauthorized for test', async () => { + it('should return 401 when unauthorized for test', async () => { return request(app.getHttpServer() as Server) .post(`/admin/projects/${project.id}/webhooks/${webhookId}/test`) - .expect(404); + .expect(401); });
381-386: Test expectation should be 400, not 200, for empty webhook name.The
UpdateWebhookRequestDtoextendsCreateWebhookRequestDto, which has@IsNotEmpty()on thenamefield. When the test sendsdto.name = ''and the global ValidationPipe is active, NestJS will reject the request with a 400 validation error, not 200.- .expect(200); + .expect(400);
🧹 Nitpick comments (6)
apps/api/integration-test/test-specs/member.integration-spec.ts (3)
313-316: Consider returning member ID from the create endpoint.The tests retrieve the member ID using raw SQL queries after creation. This approach is fragile because it assumes the last inserted record is the one just created. Consider having the POST endpoint return the created member with its ID, or use the response body from the create request to get the member ID.
Refactor to use the response from the create endpoint:
- await request(app.getHttpServer() as Server) + const createResponse = await request(app.getHttpServer() as Server) .post(`/admin/projects/${project.id}/members`) .set('Authorization', `Bearer ${accessToken}`) .send(dto) .expect(201); - const allMembers: { id: number }[] = await dataSource.query( - 'SELECT id FROM members ORDER BY id DESC LIMIT 1', - ); - memberId = allMembers.length > 0 ? allMembers[0].id : 1; + memberId = createResponse.body.id;Also applies to: 386-389
327-337: Consider verifying the actual role update.The test only checks the status code but doesn't verify that the member's role was actually updated in the database. While status code validation is useful, adding a follow-up query or GET request to confirm the role change would make the test more robust.
Add verification of the actual update:
expect(response.status).toBe(200); + + const updatedMember = await request(app.getHttpServer() as Server) + .get(`/admin/projects/${project.id}/members/${memberId}`) + .set('Authorization', `Bearer ${accessToken}`) + .expect(200); + + expect(updatedMember.body.role.id).toBe(newRole.id);
350-358: Align PUT error code with GET by returning 404
MemberService.update currently throws MemberNotFoundException (extends BadRequestException → HTTP 400) for missing members, while GET returns 404. To maintain REST semantics, have MemberNotFoundException extend NotFoundException (HTTP 404) or map it to 404.apps/api/src/domains/admin/project/webhook/webhook.service.ts (1)
122-129: Update: throw 404 when webhook missing — good; verify event orphan removal.Logic is correct. Ensure the WebhookEntity->events relation is configured to remove orphans when replacing the collection (cascade + orphanedRowAction), otherwise old events may linger after updates.
apps/api/integration-test/test-specs/webhook.integration-spec.ts (2)
122-126: Drop body on GET.
.send(dto)on a GET is ignored; remove for clarity.- .get(`/admin/projects/${project.id}/webhooks`) - .set('Authorization', `Bearer ${accessToken}`) - .send(dto) + .get(`/admin/projects/${project.id}/webhooks`) + .set('Authorization', `Bearer ${accessToken}`)
175-187: Ensure DTO enforces non-empty events array.This test expects 400; verify CreateWebhookRequestDto uses @ArrayMinSize(1), @ValidateNested({ each: true }) and @type(() => EventDto) so validation triggers reliably. Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/integration-test/test-specs/auth.integration-spec.ts(1 hunks)apps/api/integration-test/test-specs/issue.integration-spec.ts(2 hunks)apps/api/integration-test/test-specs/member.integration-spec.ts(1 hunks)apps/api/integration-test/test-specs/webhook.integration-spec.ts(1 hunks)apps/api/src/domains/admin/project/role/dtos/requests/create-role-request.dto.ts(2 hunks)apps/api/src/domains/admin/project/webhook/exceptions/index.ts(1 hunks)apps/api/src/domains/admin/project/webhook/exceptions/webhook-not-found.exception.ts(1 hunks)apps/api/src/domains/admin/project/webhook/webhook.service.ts(3 hunks)packages/ufb-shared/src/error-code.enum.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/integration-test/test-specs/issue.integration-spec.ts
- apps/api/integration-test/test-specs/auth.integration-spec.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api/src/domains/admin/project/webhook/exceptions/webhook-not-found.exception.ts (2)
apps/api/src/domains/admin/project/webhook/exceptions/index.ts (1)
WebhookNotFoundException(17-17)packages/ufb-shared/src/error-code.enum.ts (2)
ErrorCode(108-125)ErrorCode(127-127)
apps/api/integration-test/test-specs/member.integration-spec.ts (2)
apps/api/src/test-utils/util-functions.ts (2)
clearAllEntities(154-186)signInTestUser(196-208)apps/api/src/domains/admin/project/member/dtos/responses/get-all-members.dto.ts (1)
GetAllMemberResponseDto(76-87)
apps/api/integration-test/test-specs/webhook.integration-spec.ts (3)
apps/api/src/test-utils/util-functions.ts (2)
clearAllEntities(154-186)signInTestUser(196-208)apps/api/src/domains/admin/project/webhook/dtos/requests/create-webhook-request.dto.ts (1)
CreateWebhookRequestDto(24-47)apps/api/src/domains/admin/project/webhook/dtos/responses/get-webhook-by-id-response.dto.ts (1)
GetWebhookByIdResponseDto(48-80)
apps/api/src/domains/admin/project/webhook/webhook.service.ts (2)
apps/api/src/domains/admin/project/webhook/exceptions/index.ts (1)
WebhookNotFoundException(17-17)apps/api/src/domains/admin/project/webhook/exceptions/webhook-not-found.exception.ts (1)
WebhookNotFoundException(20-27)
🪛 GitHub Actions: CI
apps/api/src/domains/admin/project/webhook/webhook.service.ts
[error] 169-169: WebhookNotFoundException thrown in delete() when webhook does not exist.
[error] 127-127: WebhookNotFoundException thrown in update() when webhook does not exist.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-test
🔇 Additional comments (6)
apps/api/integration-test/test-specs/member.integration-spec.ts (1)
66-122: LGTM! Excellent test setup.The test setup is well-structured with proper initialization of transactional context, comprehensive service injection, conditional OpenSearch cleanup, and thorough test data seeding including tenant, project, role, and users. The separation of a SUPER user for authorization and a GENERAL user for member operations demonstrates good test design.
packages/ufb-shared/src/error-code.enum.ts (1)
103-106: Adds WebhookNotFound error code — LGTM.Consistent naming and grouping; aligns with new exception usage.
apps/api/src/domains/admin/project/webhook/exceptions/index.ts (1)
16-17: Export wired correctly — LGTM.New WebhookNotFoundException is properly re-exported.
apps/api/src/domains/admin/project/webhook/webhook.service.ts (2)
166-170: Delete: throw 404 when webhook missing — LGTM.Matches REST semantics; update any tests still expecting 200/500.
26-29: Explicit not-found exception imported — LGTM. No tests found expecting 200/500 on missing webhooks; ensure tests and consumers handle 404 for absent resources.apps/api/src/domains/admin/project/webhook/exceptions/webhook-not-found.exception.ts (1)
20-27: Standardized 404 exception — LGTM.Correct status type and error payload with ErrorCode.Webhook.WebhookNotFound.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/src/domains/admin/project/webhook/webhook.service.spec.ts (1)
643-655: Simplify test mocking to match actual service behavior.The test mocks
findOnethree times and also mockssave, but when the webhook doesn't exist, the service throwsWebhookNotFoundExceptionimmediately after the firstfindOnereturnsnull(seewebhook.service.tsline 127). The additional mocks are never reached and add unnecessary complexity.Apply this diff to simplify the test:
it('should throw WebhookNotFoundException when updating non-existent webhook', async () => { const dto: UpdateWebhookDto = createUpdateWebhookDto({ id: faker.number.int(), }); - jest.spyOn(webhookRepo, 'findOne').mockResolvedValueOnce(null); - jest.spyOn(webhookRepo, 'findOne').mockResolvedValueOnce(null); - jest.spyOn(webhookRepo, 'findOne').mockResolvedValueOnce(null); - jest.spyOn(webhookRepo, 'save').mockResolvedValue(webhookFixture); + jest.spyOn(webhookRepo, 'findOne').mockResolvedValue(null); await expect(webhookService.update(dto)).rejects.toThrow( new WebhookNotFoundException(), ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/domains/admin/project/webhook/webhook.service.spec.ts(4 hunks)apps/api/src/domains/admin/project/webhook/webhook.service.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/src/domains/admin/project/webhook/webhook.service.ts (2)
apps/api/src/domains/admin/project/webhook/exceptions/webhook-not-found.exception.ts (1)
WebhookNotFoundException(20-27)apps/api/src/domains/admin/project/webhook/exceptions/index.ts (1)
WebhookNotFoundException(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-test
🔇 Additional comments (5)
apps/api/src/domains/admin/project/webhook/webhook.service.spec.ts (2)
33-36: LGTM!The import of
WebhookNotFoundExceptionis correctly added to support the new error handling tests.
459-470: LGTM!The test correctly validates that
WebhookNotFoundExceptionis thrown when attempting to delete a non-existent webhook. The test setup and assertions are appropriate.apps/api/src/domains/admin/project/webhook/webhook.service.ts (3)
26-29: LGTM!The import of
WebhookNotFoundExceptionis correctly added to support the new error handling in theupdateanddeletemethods.
122-127: LGTM!The addition of proper error handling for non-existent webhooks is a good improvement. The method now explicitly fetches the webhook and throws
WebhookNotFoundExceptionif it doesn't exist, preventing potential null reference errors downstream.
166-169: LGTM!The addition of proper error handling for non-existent webhooks in the delete operation is consistent with the update method and provides clear error feedback to callers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/api/integration-test/test-specs/webhook.integration-spec.ts (4)
292-300: GET by ID should return a single object, not an arrayUpdate typing and assertions to use the object directly.
- const body = response.body as GetWebhookByIdResponseDto[]; + const body = response.body as GetWebhookByIdResponseDto; expect(response.body).toBeDefined(); - expect(body[0].id).toBe(webhookId); - expect(body[0].name).toBe('TestWebhookForGet'); - expect(body[0].url).toBe('https://example.com/webhook'); - expect(body[0].events).toHaveLength(1); - expect(body[0].status).toBe(WebhookStatusEnum.ACTIVE); - expect(body[0].createdAt).toBeDefined(); + expect(body.id).toBe(webhookId); + expect(body.name).toBe('TestWebhookForGet'); + expect(body.url).toBe('https://example.com/webhook'); + expect(body.events).toHaveLength(1); + expect(body.status).toBe(WebhookStatusEnum.ACTIVE); + expect(body.createdAt).toBeDefined();
358-365: Use object (not array) for GET by ID after updateSame issue here; fix typing and property access.
expect(response.body).toBeDefined(); - const body = response.body as GetWebhookByIdResponseDto[]; - expect(body[0].name).toBe('UpdatedTestWebhook'); - expect(body[0].url).toBe('https://updated-example.com/webhook'); - expect(body[0].events).toHaveLength(1); - expect(body[0].events[0].type).toBe(EventTypeEnum.FEEDBACK_CREATION); - expect(body[0].status).toBe(WebhookStatusEnum.ACTIVE); + const body = response.body as GetWebhookByIdResponseDto; + expect(body.name).toBe('UpdatedTestWebhook'); + expect(body.url).toBe('https://updated-example.com/webhook'); + expect(body.events).toHaveLength(1); + expect(body.events[0].type).toBe(EventTypeEnum.FEEDBACK_CREATION); + expect(body.status).toBe(WebhookStatusEnum.ACTIVE);
381-385: Invalid update payload should fail validation (expect 400)Empty name should be rejected by ValidationPipe.
return request(app.getHttpServer() as Server) .put(`/admin/projects/${project.id}/webhooks/${webhookId}`) .set('Authorization', `Bearer ${accessToken}`) .send(dto) - .expect(200); + .expect(400);
459-463: After delete, GET by ID should return 404Deleted resource must not be retrievable.
return request(app.getHttpServer() as Server) .get(`/admin/projects/${project.id}/webhooks/${webhookId}`) .set('Authorization', `Bearer ${accessToken}`) - .expect(200); + .expect(404);
🧹 Nitpick comments (5)
apps/api/integration-test/test-specs/webhook.integration-spec.ts (5)
25-26: Register DataSource with typeorm-transactionalInitialize ALS context is not enough; register the DataSource to ensure transactional propagation works in tests.
Apply:
-import { initializeTransactionalContext } from 'typeorm-transactional'; +import { initializeTransactionalContext, addTransactionalDataSource } from 'typeorm-transactional';And after acquiring the DataSource:
dataSource = module.get(getDataSourceToken()); authService = module.get(AuthService); tenantService = module.get(TenantService); projectService = module.get(ProjectService); configService = module.get(ConfigService); opensearchRepository = module.get(OpensearchRepository); + + // Register DataSource for transactional context + addTransactionalDataSource(dataSource);Based on learnings
Also applies to: 71-77
62-66: Seed faker for deterministic testsSeed once to make test data stable across runs.
beforeAll(async () => { initializeTransactionalContext(); + faker.seed(123); const module: TestingModule = await Test.createTestingModule({ imports: [AppModule], }).compile();Based on learnings
122-127: Drop body on GET requestGET should not send a body; supertest ignores it. Remove
.send(dto).return request(app.getHttpServer() as Server) .get(`/admin/projects/${project.id}/webhooks`) .set('Authorization', `Bearer ${accessToken}`) - .send(dto) .expect(200)
104-115: Prefer plain objects over DTO instances in testsSending plain JSON objects decouples tests from DTO classes and avoids serialization quirks. Pattern applies throughout.
479-485: Avoid arbitrary sleep before app.closeThe fixed 500ms delay can be flaky. Prefer awaiting actual async cleanup (e.g., pending jobs/promises) or remove if unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/integration-test/test-specs/webhook.integration-spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/integration-test/test-specs/webhook.integration-spec.ts (3)
apps/api/src/test-utils/util-functions.ts (2)
clearAllEntities(154-186)signInTestUser(196-208)apps/api/src/domains/admin/project/webhook/dtos/requests/create-webhook-request.dto.ts (1)
CreateWebhookRequestDto(24-47)apps/api/src/domains/admin/project/webhook/dtos/responses/get-webhook-by-id-response.dto.ts (1)
GetWebhookByIdResponseDto(48-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-test
1 participant
Summary by CodeRabbit
Bug Fixes
Tests