feat: enhance unit tests #2117
feat: enhance unit tests #2117
Conversation
|
Warning Rate limit exceeded@jihun has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds ESLint/Jest config tweaks, exposes WebhookService.validateEvent as public, adds IssueController/IssueService methods for update/delete by category ID, extends CodeRepositoryStub API, and introduces or expands many Jest test suites across API domains covering controllers, services, utils, and integrations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant IC as IssueController
participant IS as IssueService
participant DB as DB/Repos
rect rgb(240,250,255)
note over C,IS: updateByCategoryId flow
C->>IC: PATCH /issues/:issueId/categories/:categoryId
IC->>IS: updateByCategoryId({ issueId, categoryId })
IS->>DB: Update issue record
DB-->>IS: Result
IS-->>IC: Updated issue
IC-->>C: 200 OK
end
rect rgb(255,245,245)
note over C,IS: deleteByCategoryId flow
C->>IC: DELETE /issues/:issueId/categories/:categoryId
IC->>IS: deleteByCategoryId({ issueId, categoryId })
IS->>DB: Remove/update relation
DB-->>IS: Success
IS-->>IC: void
IC-->>C: 200 OK
end
sequenceDiagram
autonumber
participant A as Admin/API
participant WS as WebhookService
participant V as validateEvent (public)
participant Repo as WebhookRepo
A->>WS: create/update webhook (events[])
WS->>V: validateEvent(event)
alt valid
V-->>WS: true
WS->>Repo: save webhook
Repo-->>WS: saved
WS-->>A: 200 OK
else invalid
V-->>WS: false
WS-->>A: 400/validation error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 49
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
apps/api/src/domains/admin/project/api-key/api-key.service.spec.ts (1)
141-149: Stop sharing mutable DTO fixtures between tests
beforeEach(() => { dtos = apiKeys; });reuses the same array/object instances across every spec, so mutations in one test (e.g., overwritingprojectIdorvalue) silently bleed into the next. That means later tests are no longer exercising the intended inputs—e.g., after the “invalid project id” case, subsequent “happy path” specs still receive the tamperedprojectId, yet pass only because the repository mock ignores its argument. Please build a fresh DTO array per test run.Apply:
describe('createMany', () => { const projectId = faker.number.int(); const apiKeyCount = faker.number.int({ min: 2, max: 10 }); - const apiKeys = Array.from({ length: apiKeyCount }).map(() => ({ - projectId, - })) as CreateApiKeyDto[]; + const buildApiKeyDtos = () => + Array.from({ length: apiKeyCount }).map( + () => + ({ + projectId, + }) as CreateApiKeyDto, + ); let dtos: CreateApiKeyDto[]; beforeEach(() => { - dtos = apiKeys; + dtos = buildApiKeyDtos(); });apps/api/src/domains/admin/channel/channel/channel.controller.spec.ts (1)
52-62: Reset shared mocks before each test
MockChannelServiceis declared once and reused, so the call counts accumulate between specs. EverytoHaveBeenCalledTimes(1)assertion will start failing as soon as a previous test has already hit the mock (or the suite will become order-dependent) unless your Jest config forcibly clears mocks between tests. Add an explicit reset inside the existingbeforeEach.beforeEach(async () => { + jest.clearAllMocks(); + const module = await Test.createTestingModule({ controllers: [ChannelController], providers: [apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.spec.ts (1)
323-336: Lock acquisition should be asserted.Since we now depend on
SchedulerLockService, also assert thatacquireLockandreleaseLockwere called exactly once and in the right order; this guards against future regressions in lock handling.Suggested diff:
+expect(schedulerLockService.acquireLock).toHaveBeenCalledWith( + `feedback-statistics-${projectId}`, + expect.any(Number), +); +expect(schedulerLockService.releaseLock).toHaveBeenCalledWith( + `feedback-statistics-${projectId}`, +);apps/api/src/domains/admin/project/member/member.service.spec.ts (1)
48-59: Override RoleService/UserService with mock providers
MemberServiceProviders currently spreads real RoleService and UserService classes. Replace these with jest-mock providers (e.g. via getMockProvider or useValue with jest.fn()) to prevent instantiating real services in tests. [apps/api/src/test-utils/providers/member.service.providers.ts]apps/api/src/domains/admin/channel/option/option.service.spec.ts (3)
51-114: Mock repository responses forsave.In the success-path tests (
create/createwith inactive input) we spy onoptionRepo.savebut never provide a resolved value. Unless the provider replacessavewith a preconfigured Jest mock that already returns the entity,optionService.createwill awaitundefined, sooption.key/option.nameassertions will throw. Please explicitlymockResolvedValue(ormockImplementation) to the entity you expect.Apply a diff like:
- jest.spyOn(optionRepo, 'save'); + jest.spyOn(optionRepo, 'save').mockResolvedValue({ + key: dto.key, + name: dto.name, + } as OptionEntity);
118-168: Ensure deterministicsaveoutput forcreateMany.Same concern here:
optionService.createManylikely resolves to whateveroptionRepo.savereturns. WithoutmockResolvedValue, the returnedoptionsarray isundefined, so the indexed assertions fail. Please mock the save call to return the DTO payload (or a matching shape).A minimal fix:
- jest.spyOn(optionRepo, 'save'); + jest.spyOn(optionRepo, 'save').mockResolvedValue( + dto.options as unknown as OptionEntity[], + );
170-304: Guard against undefined save/query mocks.Every success-path in
replaceManyassumesoptionRepo.save/queryresolve cleanly, yet no mock implementation is supplied. If the provider’s stub returnsundefined,optionService.replaceManycould reject, making the tests flaky. Please mock them (mockResolvedValuefor promises,mockImplementationfor sync) to keep the expectations deterministic.Example fix:
- jest.spyOn(optionRepo, 'query'); - jest.spyOn(optionRepo, 'save'); + jest.spyOn(optionRepo, 'query').mockResolvedValue(undefined as never); + jest.spyOn(optionRepo, 'save').mockResolvedValue( + dto.options as unknown as OptionEntity[], + );apps/api/src/domains/admin/user/create-user.service.spec.ts (1)
165-166: Inconsistent mocking: assert against MemberService instead of repository.save.Elsewhere you mock memberService.create to simulate failure, which implies CreateUserService uses MemberService for member creation. These two tests should spy/expect on memberService.create, not memberRepo.save.
- jest.spyOn(memberRepo, 'save'); + jest.spyOn(memberService, 'create'); ... - expect(memberRepo.save).toHaveBeenCalledTimes(1); + expect(memberService.create).toHaveBeenCalledTimes(1);Apply the same change to the "super user having a role" test.
Also applies to: 173-173, 185-186, 194-195
🧹 Nitpick comments (111)
apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts (1)
889-898: Transaction executes once per day/issue.The service wraps each day/issue insert in its own transaction; with a single day and issue, the spy should record one call. Incrementing
transactionCallCountand asserting it equals 1 matches the implementation today, but if an extra async invocation ever occurred the test would pass incorrectly. Please convert this toexpect(feedbackIssueStatsRepo.manager.transaction).toHaveBeenCalledTimes(dayToCreate * issues.length)so we assert on the actual spy and avoid manual counters.apps/api/src/shared/code/code.service.spec.ts (1)
240-268: Restore repository stubs after spying onfindOne.
jest.spyOn(codeRepo, 'findOne')overrides the stubbed implementation, but the test never restores it. The next tests in this suite rely on the defaultfindOnebehavior (from the stub), so the spy leaks across specs and makes them depend on the spy’s mock instead of the real stub. Please callmockRestore()(or usemockResolvedValueOnce) so subsequent cases run against the intended repository stub implementation.Apply the following fix:
- jest.spyOn(codeRepo, 'findOne').mockResolvedValue(mockEntity as any); + const findOneSpy = jest + .spyOn(codeRepo, 'findOne') + .mockResolvedValue(mockEntity as any); @@ - expect(saveSpy).toHaveBeenCalledWith( + expect(saveSpy).toHaveBeenCalledWith( expect.objectContaining({ tryCount: initialTryCount + 1, }), ); + findOneSpy.mockRestore();apps/api/eslint.config.mjs (1)
25-32: Quadruple-check we really want to blanket-disable these rules.Turning off
no-unsafe-*andno-unnecessary-type-assertionacross every spec will hide genuine unsound usage. Consider scoping the override to the handful of files that actually need it, or use inline@ts-expect-errorto force justification when a test goes off the type rails.apps/api/src/domains/admin/user/user.controller.spec.ts (2)
140-145: Shadowing invitation payload allows mutation between controller and service.
inviteUserspreadsinvitationDto, but controller addsinvitedBybefore forwarding. Using{ ...invitationDto, invitedBy: userDto }in the expectation mutates the DTO object (because jest compares by reference). Capture the exact argument viaexpect(MockUserService.sendInvitationCode).toHaveBeenCalledWith(expect.objectContaining({ invitedBy: userDto }))to avoid false positives when DTO extends.
195-198:changePasswordexpectation drops DTO fields (e.g., audit metadata).The controller forwards the request DTO as-is plus the user id. By reconstructing the payload manually we risk diverging from the implementation when new properties are added. Instead, assert on the specific fields you care about via
expect.objectContaining({ userId: userDto.id, password: changePasswordDto.password })so additional fields don’t break the test.apps/api/src/domains/admin/project/member/member.controller.spec.ts (1)
51-53: Reset mocks between tests to avoid leaking stubbed behavior
jest.clearAllMocks()only wipes call history; it keeps whatevermockResolvedValue/mockRejectedValueyou set in prior tests. Because all specs share the sameMockMemberServiceinstance, a forgotten override could inherit the previous behavior and produce misleading results. Resetting the mocks (or re-instantiating the service stub per test) avoids that footgun.Apply this diff to reset both call history and implementations:
afterEach(() => { - jest.clearAllMocks(); + jest.resetAllMocks(); });apps/api/src/domains/admin/statistics/issue/issue-statistics.controller.spec.ts (4)
171-173: Remove redundant equality assertion.In this edge-case scenario we only need to assert that the controller wires the request data to the service correctly. The extra
expect(result).toEqual(mockResult);adds no coverage beyond what the other branches already guarantee and just increases maintenance churn if the service response shape evolves.
245-248: Preferrejects.toThrow(error)over string matching.Using the actual
Errorinstance (ortoThrow(error.message)) makes the intent a little clearer and avoids coupling the test to the exact error string formatting. Consider switching toawait expect(...).rejects.toThrow(error);.
262-267: Preferrejects.toThrow(error)over string matching.Same reasoning as the previous error-handling case: assert against the exact
Errorinstance for tighter coverage and less brittle expectations.
278-279: Preferrejects.toThrow(error)over string matching.See earlier comment—matching on the actual
Errorobject keeps the test resilient to incidental message tweaks.apps/api/src/domains/admin/statistics/feedback/feedback-statistics.controller.spec.ts (2)
172-173: Remove redundant equality assertion.The empty-channelIds branch is already exercised by the service mock; checking
toEqual(mockServiceResponse)offers no additional signal beyond what the primary branch covers. Dropping the assertion keeps the test lean.
266-288: Collapse identical assertions via parameterized helper.The three ratio tests repeat the same arrange/act/assert shape; consider extracting a helper (or using
it.each) to reduce repetition while keeping coverage intact.apps/api/src/domains/admin/statistics/issue/issue-statistics.service.spec.ts (10)
219-223: Tighten empty statistics expectation.Consider asserting that
issueStatsRepo.findwas called with the same options as the non-empty case to ensure the code path doesn't diverge in query formation.
240-249: Single-day interval result needs explicitendDate.Double-check the service logic for single-day intervals: if it mirrors the standard day aggregation, the
endDatelikely matches thestartDate. The test currently assumes that; if the service ever changes to use ISO strings or maintain timezone adjustments, this assertion may break. Consider asserting on the DTO-level fields (e.g.,expect(result.statistics).toHaveLength(1); expect(result.statistics[0]).toMatchObject({...})) instead of hardcoding the entire shape.
283-294:countzero case should assert query arguments.Right now the test only checks the returned payload. Please also verify that
issueRepo.countreceived the sameBetweenquery as the primary branch so we retain coverage of the filtering logic.
368-383: Verify transactional mock executescallback.The custom
mockImplementationwraps the transaction callback in a manually crafted manager. That’s solid, but add an assertion thatmockManager.createQueryBuilder(or the relevant builder methods) were invoked. This ensures the mock actually exercised the branch rather than silently succeeding.
523-526: Zero-day branch: assert repositories untouched.The zero-day test ensures we bail early; extend it by checking the repositories (particularly
issueRepo.count) remain untouched—mirroring the pattern used in the feedback service spec for consistency.
551-556:findOneexpectation mismatched with implementation.The service’s
updateCountpath stores a Date adjusted to midnight in the project’s timezone. The spec currently expectsfindOneto receiveexpect.any(Date)without verifying that the adjustment happened. Capture the actual argument (const { date } = issueStatsRepo.findOne.mock.calls[0][0].where) and assert viaDateTime.fromJSDate(date)to ensure the timezone logic is exercised.
589-594: Same as above: verify adjusted date.For the insert path, assert that the
datein the where clause matches the expected normalized midnight (after timezone adjustment).
607-620: Project-not-found branch: assert no repository writes.Add expectations that
issueStatsRepo.save/createQueryBuilderwere not called to ensure we fail fast.
623-635: Zero-count branch should also leave repositories untouched.We bail out before hitting the DB when
count === 0. Please assert thatissueStatsRepo.findOne,.save, and.createQueryBuilderstay untouched in addition to the project lookup.
667-674: Default-count branch should assert timezone normalization.Similar to prior comments, capture the argument passed to
.valuesand check thedatecomponent equals midnight in the project timezone, ensuring we don’t regress formatting logic.apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.spec.ts (8)
126-134: Also assertfeedbackStatsRepo.findcall count.Add
expect(feedbackStatsRepo.find).toHaveBeenCalledTimes(1);to ensure the branch issues exactly one query.
261-271:getCountbranch should verify query arguments.Mirror the issue-service spec by asserting the exact
Betweencondition onfeedbackRepo.count.
297-308:getIssuedRatioshould assert query builder wiring.Beyond checking the final ratio, assert that subsequent builder calls (e.g.,
select,where) are invoked with the expected arguments, just like the issue-service tests do for status counts.
395-403: Project-not-found branch: ensure no locks/transactions acquired.Complement the failure path by asserting
schedulerLockService.acquireLock,schedulerRegistry.addCronJob, and the repository manager transaction were untouched.
368-384: Transaction mock should assert manager usage.As in the issue-service spec, verify the mock manager’s query builder was used to insert/update rows; otherwise the test could pass even if the callback never touches it.
454-457: Missing assertions fororUpdate/updateEntity.After mocking these builder methods, assert they were invoked with the expected values so we cover the upsert branch thoroughly.
477-490: Project-not-found branch: assert repo untouched.Add expectations that
feedbackStatsRepo.save/createQueryBuilder/findOneweren’t called to ensure the method exits early.
492-508: Zero-count early exit should verify no I/O.Good to see parity with the issue service: assert that neither project lookup nor repo operations fire when
countis zero.apps/api/src/domains/admin/channel/field/field.service.spec.ts (2)
271-275: Future‑proof the OpenSearch call assertion.Matching the entire request object makes the test brittle — any extra flag we add to
putMappingswill break the test even though the behavior is still correct. Please relax the matcher to focus on just the contract you care about (index + mappings).- expect(osRepository.putMappings).toHaveBeenCalledWith({ - index: channelId.toString(), - - mappings: expect.any(Object), - }); + expect(osRepository.putMappings).toHaveBeenCalledWith( + expect.objectContaining({ + index: channelId.toString(), + mappings: expect.any(Object), + }), + );
414-418: Apply the same relaxed matcher here.For the same reason as above, please assert with
expect.objectContainingso the test keeps passing if we later add optional arguments toputMappings.- expect(osRepository.putMappings).toHaveBeenCalledWith({ - index: channelId.toString(), - - mappings: expect.any(Object), - }); + expect(osRepository.putMappings).toHaveBeenCalledWith( + expect.objectContaining({ + index: channelId.toString(), + mappings: expect.any(Object), + }), + );apps/api/src/domains/admin/project/webhook/webhook.service.ts (1)
40-62: ExposevalidateEventonly if consumers actually need itMaking
validateEventpublic just to satisfy the spec file expands the service’s API surface; nothing in the production code needs this visibility, so we’re on the hook to support it going forward. The Jest suite can still exercise the method through the public flows (create,update). Please revert this visibility change and, if the tests truly need direct access, move the helper into a dedicated module/export instead of widening the service API.apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts (3)
252-266: Missing expectation for aNotFoundExceptionThe listener throws a
NotFoundExceptionwhen an issue can’t be found, but this test just checks that “something” was thrown. Tighten the assertion so we fail if the behavior regresses.
328-343: Assert the concrete exception for issue status changesSame concern here—the test should assert that a
NotFoundException(or the intended domain error) is thrown, not just any generic error.
380-432: RepeatedwebhookRepo.findstubs make tests nondeterministicEach of these edge-case cases stubs
webhookRepo.findafter the listener was constructed. Because module-level mocks persist across test cases, forgetting to restore them will leak state into subsequent tests. Convert these tomockResolvedValueOnceso the mock is automatically reset after the expectation, or add explicitmockRestore()inafterEach.apps/api/src/domains/admin/feedback/feedback.controller.spec.ts (8)
61-63: Use resetAllMocks to avoid implementation leakage between testsclearAllMocks resets call counts but keeps mock implementations. Prefer resetAllMocks for a clean slate each test.
- // Reset all mocks before each test - jest.clearAllMocks(); + // Reset mocks and their implementations before each test + jest.resetAllMocks();
78-105: Relax exact argument matching to reduce brittlenessFuture additions to the call payload will break strict toHaveBeenCalledWith matches. Use objectContaining.
- expect(MockChannelService.findById).toHaveBeenCalledWith({ channelId }); - expect(MockFeedbackService.create).toHaveBeenCalledWith({ - data: body, - channelId, - }); + expect(MockChannelService.findById).toHaveBeenCalledWith( + expect.objectContaining({ channelId }), + ); + expect(MockFeedbackService.create).toHaveBeenCalledWith( + expect.objectContaining({ data: body, channelId }), + );
106-121: Assert exception type in addition to messageStrengthens guarantees that HTTP exceptions are propagated.
- await expect( - feedbackController.create(projectId, channelId, {}), - ).rejects.toThrow('Invalid channel id'); + const promise = feedbackController.create(projectId, channelId, {}); + await expect(promise).rejects.toBeInstanceOf(BadRequestException); + await expect(promise).rejects.toThrow('Invalid channel id');
124-156: Avoid strict equality on service call payload; verify result valueUse partial matching to prevent fragile tests and assert the returned value explicitly.
- expect(MockFeedbackService.findByChannelIdV2).toHaveBeenCalledWith({ - ...dto, - channelId, - }); - expect(result).toBeDefined(); + expect(MockFeedbackService.findByChannelIdV2).toHaveBeenCalledWith( + expect.objectContaining({ channelId }), + ); + expect(result).toEqual(mockResult);
208-255: Export: relax payload match and assert reply interactionsController params may grow; assert partial payload and basic reply usage to harden the test.
- expect(MockFeedbackService.generateFile).toHaveBeenCalledWith({ - projectId, - channelId, - queries: dto.queries, - operator: dto.operator, - sort: dto.sort, - type: dto.type, - fieldIds: dto.fieldIds, - filterFeedbackIds: dto.filterFeedbackIds, - defaultQueries: dto.defaultQueries, - }); - expect(MockHistoryService.createHistory).toHaveBeenCalled(); + expect(MockFeedbackService.generateFile).toHaveBeenCalledWith( + expect.objectContaining({ + projectId, + channelId, + type: dto.type, + }), + ); + expect(MockHistoryService.createHistory).toHaveBeenCalled(); + expect(response.type).toHaveBeenCalled(); + expect(response.header).toHaveBeenCalled();Ensure controller uses reply.type/header/send (or pipes the stream). If it pipes directly without calling reply.type/header, adjust assertions to match actual behavior.
257-274: Update: prefer partial argument match to avoid brittleness- expect(MockFeedbackService.updateFeedback).toHaveBeenCalledWith({ - channelId, - feedbackId, - data: body, - }); + expect(MockFeedbackService.updateFeedback).toHaveBeenCalledWith( + expect.objectContaining({ channelId, feedbackId, data: body }), + );
277-295: DeleteMany: prefer partial argument match- expect(MockFeedbackService.deleteByIds).toHaveBeenCalledWith({ - channelId, - feedbackIds, - }); + expect(MockFeedbackService.deleteByIds).toHaveBeenCalledWith( + expect.objectContaining({ channelId, feedbackIds }), + );
298-415: Standardize error assertions across testsAdd type assertions (BadRequestException) alongside message checks for stronger guarantees and clearer failures.
Example (apply similarly to other error tests):
- await expect( - feedbackController.findByChannelId(channelId, dto), - ).rejects.toThrow('Invalid channel'); + const promise = feedbackController.findByChannelId(channelId, dto); + await expect(promise).rejects.toBeInstanceOf(BadRequestException); + await expect(promise).rejects.toThrow('Invalid channel');Optional: constrain faker to positive IDs to avoid accidental negatives.
- const channelId = faker.number.int(); + const channelId = faker.number.int({ min: 1 });apps/api/src/shared/mailing/email-verification-mailing.service.spec.ts (12)
21-21: Decouple tests from env/config modules (remove TestConfig).You’re fully mocking ConfigService and MailerService, so importing TestConfig is unnecessary and adds brittle env/schema dependencies and slower module boot. Drop it in all three module builds.
Proposed diffs:
- import { TestConfig } from '@/test-utils/util-functions';- imports: [TestConfig], + imports: [],- const module = await Test.createTestingModule({ - imports: [TestConfig], + const module = await Test.createTestingModule({ + imports: [],- const module = await Test.createTestingModule({ - imports: [TestConfig], + const module = await Test.createTestingModule({ + imports: [],If CI relies on .env.test, confirm it’s intentional; otherwise this change will speed up and stabilize tests. Would you like me to open a follow-up PR to remove TestConfig across similar specs?
Also applies to: 31-31, 108-109, 136-137
19-19: Type import: include TestingModule for cleaner lifecycle management.Small improvement to properly type the testing module when storing/closing it.
-import { Test } from '@nestjs/testing'; +import { Test, TestingModule } from '@nestjs/testing';
24-28: Seed faker for deterministic tests.Ensures reproducible failures and consistent snapshots.
describe('EmailVerificationMailingService', () => { + beforeAll(() => { + faker.seed(20250925); + });
29-56: Store and close TestingModule; avoid “module” name shadowing; reset mocks safely.
- Use a named variable (testingModule) instead of “module”.
- Close the Nest testing module after each test to prevent open handle leaks.
- Prefer restoreAllMocks (or resetAllMocks) to avoid leaked mock implementations when changed in a test.
- beforeEach(async () => { - const module = await Test.createTestingModule({ + let testingModule: TestingModule; + beforeEach(async () => { + testingModule = await Test.createTestingModule({ imports: [TestConfig], providers: [ EmailVerificationMailingService, { provide: MailerService, useValue: { - sendMail: jest.fn(), + sendMail: jest.fn().mockResolvedValue(undefined), }, }, { provide: ConfigService, useValue: { get: jest .fn() .mockReturnValue({ baseUrl: 'http://localhost:3000' }), }, }, ], - }).compile(); + }).compile(); - emailVerificationMailingService = module.get( + emailVerificationMailingService = testingModule.get( EmailVerificationMailingService, ); - mockMailerService = module.get(MailerService); - mockConfigService = module.get(ConfigService); + mockMailerService = testingModule.get(MailerService) as jest.Mocked<MailerService>; + mockConfigService = testingModule.get(ConfigService) as jest.Mocked<ConfigService>; }); - afterEach(() => { - jest.clearAllMocks(); - }); + afterEach(async () => { + await testingModule?.close(); + jest.restoreAllMocks(); + });Also applies to: 58-60
62-70: Constructor/injection tests are fine; consider also asserting MailerService injection.Optional: add a check for MailerService to mirror ConfigService injection.
91-103: Consolidate duplicate assertions with the previous test.This test repeats the prior one; you can merge by asserting individual fields there to reduce runtime and duplication.
112-114: Mock sendMail as resolved Promise.Be explicit to avoid relying on await-ing undefined.
- useValue: { sendMail: jest.fn() }, + useValue: { sendMail: jest.fn().mockResolvedValue(undefined) },
127-131: Close the ad-hoc module to avoid open handles.When you build a new module within a test, close it after assertions.
const callArgs = mockCalls[0]?.[0] as { context?: { baseUrl?: string } }; expect(callArgs.context?.baseUrl).toBe(''); + await module.close();
140-142: Mock sendMail as resolved Promise (second ad-hoc module).Same as above for consistency.
- useValue: { sendMail: jest.fn() }, + useValue: { sendMail: jest.fn().mockResolvedValue(undefined) },
157-161: Close the second ad-hoc module as well.Avoid leaking resources.
const callArgs = mockCalls[0]?.[0] as { context?: { baseUrl?: string } }; expect(callArgs.context?.baseUrl).toBe(''); + await module.close();
165-165: Use mockRejectedValueOnce to limit scope of failure behavior.Keeps the failure setup local to this test.
- (mockMailerService.sendMail as jest.Mock).mockRejectedValue(error); + (mockMailerService.sendMail as jest.Mock).mockRejectedValueOnce(error);
175-206: Iteration assertions are good; optionally assert last call or clear per iteration.For stronger guarantees, consider mockClear() inside the loop or use toHaveBeenLastCalledWith for each iteration.
apps/api/src/domains/admin/project/member/member.service.spec.ts (13)
16-16: Seed faker for deterministic testsRandomized ids/counts can make failures hard to reproduce. Seed faker once in this suite.
Add near the top-level describe:
beforeAll(() => { faker.seed(123456); });
54-59: Reset/restore Jest mocks between testsEnsure no spy/mocks bleed across tests.
Add:
afterEach(() => { jest.restoreAllMocks(); jest.clearAllMocks(); });
72-86: Strengthen assertion: verify uniqueness lookup shapeGood happy-path coverage. Consider asserting the where-clause used by findOne to catch regressions in matching logic.
Example:
expect(memberRepo.findOne).toHaveBeenCalledWith( expect.objectContaining({ where: expect.objectContaining({ user: expect.objectContaining({ id: userId }), role: expect.objectContaining({ project: expect.objectContaining({ id: projectId }) }), }), }), );
139-153: Check batch save payload lengthOn success, also assert the saved batch size to ensure all dtos are persisted.
Example:
const firstCallArg = (memberRepo.save as jest.Mock).mock.calls[0][0]; expect(Array.isArray(firstCallArg)).toBe(true); expect(firstCallArg).toHaveLength(memberCount);
155-171: Avoid brittle call-count expectations in early‑exit failuresExpecting exactly 1 call on role/user lookups couples tests to short-circuiting details. Prefer “at least once” or drop the exact count.
Example:
expect(roleService.findById).toHaveBeenCalled(); expect(userService.findById).toHaveBeenCalled();
173-191: Same brittleness for invalid user in createManyUse presence assertions rather than exact call counts to keep tests resilient if implementation changes iteration/validation order.
205-215: Clarify roleId/newRoleId mismatch in update testfindById(roleId) returning a Role with id=newRoleId is confusing. Return a Role whose id matches the requested roleId, or assert the saved entity uses the returned role id.
Option A (simplify stub):
- const newRoleId = faker.number.int(); - const role = { + const role = { project: { id: projectId }, - id: newRoleId, + id: roleId, } as RoleEntity;Option B (keep differing ids but assert save uses it):
expect(memberRepo.save).toHaveBeenCalledWith( expect.objectContaining({ id: memberId, role: expect.objectContaining({ id: role.id }), }), );
265-319: Add edge cases for email validationExtend coverage to ensure robustness:
- Case-insensitive domain matching (e.g., [email protected] with ['example.com']).
- Multiple tenants where any tenant allowing the domain should pass.
Example additions:
it('validates email case-insensitively', async () => { jest.spyOn(tenantRepo, 'find').mockResolvedValue([ { allowDomains: ['Example.COM'] } as unknown as TenantEntity, ]); await expect(memberService.validateEmail('[email protected]')).resolves.toBe(true); }); it('validates across multiple tenants (any allow passes)', async () => { jest.spyOn(tenantRepo, 'find').mockResolvedValue([ { allowDomains: ['foo.com'] } as unknown as TenantEntity, { allowDomains: ['bar.com'] } as unknown as TenantEntity, ]); await expect(memberService.validateEmail('[email protected]')).resolves.toBe(true); });
369-412: Strengthen findAll base assertionsAlso assert:
- leftJoinAndSelect called with expected aliases.
- andWhere includes projectId condition.
- offset/limit values computed from options.
Example:
expect(mockQueryBuilder.leftJoinAndSelect).toHaveBeenCalled(); expect(mockQueryBuilder.andWhere).toHaveBeenCalledWith( expect.stringContaining('project'), expect.objectContaining({ projectId }), ); expect(mockQueryBuilder.offset).toHaveBeenCalledWith((page - 1) * limit); expect(mockQueryBuilder.limit).toHaveBeenCalledWith(limit);
413-456: Prefer using QueryV2ConditionsEnum rather than string castsAvoid 'as any' to stay aligned with the real condition mapping logic.
import { QueryV2ConditionsEnum } from '@/common/enums'; // ... queries: [ { key: 'name', value: 'John', condition: QueryV2ConditionsEnum.IS }, { key: 'email', value: '[email protected]', condition: QueryV2ConditionsEnum.CONTAINS }, ],
458-494: Verify OR path uses orWhere (not andWhere)Right now the mock only exposes andWhere; ensure OR logic calls orWhere (or equivalent grouped condition). Add orWhere to the mock and assert it’s used.
- const mockQueryBuilder = { + const mockQueryBuilder = { leftJoinAndSelect: jest.fn().mockReturnThis(), andWhere: jest.fn().mockReturnThis(), + orWhere: jest.fn().mockReturnThis(), offset: jest.fn().mockReturnThis(), limit: jest.fn().mockReturnThis(), getMany: jest.fn().mockResolvedValue(items), getCount: jest.fn().mockResolvedValue(total), };Then:
expect(mockQueryBuilder.orWhere).toHaveBeenCalled();
497-509: Consider not-found path for deleteAdd a negative test (e.g., when the target doesn’t exist, ensure proper exception or no-op per spec).
511-541: Add guard/behavior for empty list in deleteManyValidate behavior when memberIds is empty: should short-circuit and not hit the DB builder.
Example:
it('deleteMany short-circuits on empty ids', async () => { const qb = { delete: jest.fn(), from: jest.fn(), where: jest.fn(), execute: jest.fn() }; jest.spyOn(memberRepo, 'createQueryBuilder').mockReturnValue(qb as any); await memberService.deleteMany({ memberIds: [] }); expect(memberRepo.createQueryBuilder).not.toHaveBeenCalled(); });apps/api/src/domains/admin/project/category/category.service.spec.ts (5)
69-71: Initialize CLS properly in testsUse ClsModule.forRoot to ensure ClsService is available; direct import can be insufficient in unit tests.
- imports: [ClsModule], + imports: [ClsModule.forRoot({ global: true, middleware: { mount: false } })],
86-88: Type the repository from module.get to avoid any/unknownMinor typing hardening.
- service = module.get<CategoryService>(CategoryService); - repository = module.get(getRepositoryToken(CategoryEntity)); + service = module.get<CategoryService>(CategoryService); + repository = module.get<Repository<CategoryEntity>>(getRepositoryToken(CategoryEntity)) as jest.Mocked<Repository<CategoryEntity>>;
58-67: DRY up the query builder mock and avoid type hacksThe QB mock is duplicated and re-declared multiple times, with manual “unknown as jest.MockedFunction” casts. Extract a helper to reduce repetition and improve clarity.
Example helper (add near the top of the file):
function makeQBMock({ items = [], count = 0, }: { items?: unknown[]; count?: number } = {}) { const qb = { leftJoin: jest.fn().mockReturnThis(), where: jest.fn().mockReturnThis(), andWhere: jest.fn().mockReturnThis(), addOrderBy: jest.fn().mockReturnThis(), offset: jest.fn().mockReturnThis(), limit: jest.fn().mockReturnThis(), getMany: jest.fn().mockResolvedValue(items), getCount: jest.fn().mockResolvedValue(count), }; return qb; }Then in tests:
const qb = makeQBMock({ items: mockCategories, count: totalCount }); (repository.createQueryBuilder as jest.Mock).mockReturnValue(qb);Optional cleanup: the early mockQueryBuilder in beforeEach becomes unnecessary when each test sets its own QB.
- createQueryBuilder: jest.fn().mockReturnValue(mockQueryBuilder), + createQueryBuilder: jest.fn(),Also applies to: 76-76, 140-163, 182-205, 222-245
320-323: Avoid mutating the fixture in expectationObject.assign mutates existingCategory. Prefer objectContaining to assert payload shape.
- expect(repository.save).toHaveBeenCalledWith( - Object.assign(existingCategory, updateDto), - ); + expect(repository.save).toHaveBeenCalledWith( + expect.objectContaining({ + id: existingCategory.id, + name: updateDto.name, + project: { id: updateDto.projectId }, + }), + );
211-218: Assert key QB interactions to tighten behavior checksGiven these tests set sort and filters, also assert that the QB methods were called appropriately (e.g., addOrderBy on name when sort provided; andWhere on name filter).
Example:
const qb = (repository.createQueryBuilder as jest.Mock).mock.results[0].value; expect(qb.addOrderBy).toHaveBeenCalledWith('category.name', 'ASC'); expect(qb.andWhere).toHaveBeenCalledWith('category.name ILIKE :name', { name: `%${findAllDto.categoryName}%` });Also applies to: 246-249, 171-178, 206-209
apps/api/src/domains/admin/project/category/category.controller.spec.ts (5)
79-81: Relax assertion from DTO instance to response shapeInstance checks couple tests to controller implementation details. Prefer shape checks.
- expect(result).toBeInstanceOf(CreateCategoryResponseDto); - expect(result.id).toBe(categoryId); + expect(result).toEqual( + expect.objectContaining({ id: categoryId, name: body.name }), + );
145-147: Avoid strict DTO instance check in list responseUse shape-based assertions for resilience.
- expect(result).toBeInstanceOf(GetAllCategoriesResponseDto); - expect(result.items).toHaveLength(1); + expect(result).toEqual( + expect.objectContaining({ + items: expect.arrayContaining([ + expect.objectContaining({ id: expect.any(Number), name: expect.any(String) }), + ]), + meta: expect.objectContaining({ + totalItems: 1, + itemsPerPage: body.limit, + currentPage: body.page, + }), + }), + );
171-179: Optional: Prefer shape checks over class instance for default pagination pathSame rationale as above.
- expect(result).toBeInstanceOf(GetAllCategoriesResponseDto); - expect(result.items).toHaveLength(0); + expect(result).toEqual( + expect.objectContaining({ + items: [], + meta: expect.objectContaining({ + totalItems: 0, + itemsPerPage: 10, + currentPage: 1, + }), + }), + );
172-178: DTO typing mismatch: categoryName should be optionalTests assume categoryName can be undefined. The DTO currently types it as required string while decorated with @IsOptional.
Cross-file reference: apps/api/src/domains/admin/project/category/dtos/requests/get-all-categories-request.dto.ts (Lines 21-35).
Proposed change in that DTO file:
- @IsString() - categoryName: string; + @IsString() + categoryName?: string;
71-72: Minor: You can skip spyOn for jest.fn mocksMethods are already jest.fn; set return values directly for brevity.
Example:
MockCategoryService.create.mockResolvedValue(mockCategory); MockCategoryService.findAllByProjectId.mockResolvedValue(mockResult); MockCategoryService.update.mockRejectedValue(new BadRequestException('...'));Also applies to: 132-135, 166-169, 214-215, 252-253
apps/api/src/domains/admin/user/user-password.service.spec.ts (3)
55-57: Reset mock implementations between cases
jest.clearAllMocks()keeps the.mockResolvedValue(...)implementations you set up in earlier specs. That lets a later test accidentally rely on a stub from a previous one and pass when the real code would behave differently. Swapping tojest.resetAllMocks()clears both call history and implementations every time.Apply this diff:
- jest.clearAllMocks(); + jest.resetAllMocks();
42-54: Drop the unused_codeRepohandleWe grab the repository stub but never use it, so the extra variable will trip
no-unused-vars/noUnusedLocals. Please remove it (and theCodeEntityimport if nothing else needs it) to keep the spec lint-clean.Suggested patch:
-import { CodeEntity } from '@/shared/code/code.entity'; import { CodeService } from '@/shared/code/code.service'; +import { CodeTypeEnum } from '@/shared/code/code-type.enum'; @@ - let _codeRepo: Repository<CodeEntity>; @@ - _codeRepo = module.get(getRepositoryToken(CodeEntity));
71-121: Reuse the code-type enumWe repeat the literal
'RESET_PASSWORD'in several expectations. PullingCodeTypeEnum.RESET_PASSWORDkeeps the assertions aligned with the production constant and avoids breakage if the enum value ever changes.Sample patch (update both call sites in this file):
- expect(codeService.setCode).toHaveBeenCalledWith({ - type: 'RESET_PASSWORD', + expect(codeService.setCode).toHaveBeenCalledWith({ + type: CodeTypeEnum.RESET_PASSWORD, key: email, }); @@ - expect(codeService.verifyCode).toHaveBeenCalledWith({ - type: 'RESET_PASSWORD', + expect(codeService.verifyCode).toHaveBeenCalledWith({ + type: CodeTypeEnum.RESET_PASSWORD, key: dto.email, code: dto.code, });apps/api/src/domains/admin/tenant/tenant.service.spec.ts (4)
166-168: Loosen matcher to avoid brittle deep equality on oauthConfig
expect.objectContaining(dto)performs deep equality on nested objects; use nested objectContaining for oauthConfig.- expect(tenantRepo.save).toHaveBeenCalledWith( - expect.objectContaining(dto), - ); + expect(tenantRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + siteName: dto.siteName, + useEmail: dto.useEmail, + allowDomains: dto.allowDomains, + useOAuth: dto.useOAuth, + oauthConfig: expect.objectContaining(dto.oauthConfig), + }), + );
88-99: Strengthen negative test by asserting no save on duplicate site nameAlso assert that tenantRepo.save was not called.
- jest.spyOn(tenantRepo, 'find').mockResolvedValue([tenantFixture]); + jest.spyOn(tenantRepo, 'find').mockResolvedValue([tenantFixture]); + const saveSpy = jest.spyOn(tenantRepo, 'save'); @@ await expect(tenantService.create(dto)).rejects.toThrow( TenantAlreadyExistsException, ); + expect(saveSpy).not.toHaveBeenCalled();
206-210: Make success path explicit by stubbing find()Avoid relying on default provider behavior by stubbing the repository for the happy path.
- it('finding a tenant succeeds when there is a tenant', async () => { - const tenant = await tenantService.findOne(); + it('finding a tenant succeeds when there is a tenant', async () => { + jest.spyOn(tenantRepo, 'find').mockResolvedValue([tenantFixture]); + const tenant = await tenantService.findOne();
101-126: Use enum for user type in test
Replacetype: 'SUPER'withtype: UserTypeEnum.SUPERintenant.service.spec.tsand add at the top:import { UserTypeEnum } from '../user/entities/enums';apps/api/src/domains/admin/project/issue/issue.controller.spec.ts (4)
45-55: Clear/reset Jest mocks between tests to prevent cross-test leakageSince
MockIssueServiceis a shared object, calls can accumulate. Clear mocks in a test lifecycle hook.Apply this diff:
beforeEach(async () => { const module = await Test.createTestingModule({ controllers: [IssueController], providers: [ getMockProvider(IssueService, MockIssueService), getMockProvider(DataSource, MockDataSource), ], }).compile(); issueController = module.get<IssueController>(IssueController); }); + + afterEach(() => { + jest.clearAllMocks(); + jest.restoreAllMocks(); + });
58-77: Strengthen assertions on the created responseCurrently only checks
resultis defined. Assert the transformed shape to catch regressions.Apply this diff:
- expect(result).toBeDefined(); + expect(result).toBeDefined(); + // Adjust fields if the controller transforms the response differently + expect(result).toEqual( + expect.objectContaining({ + id: mockIssue.id, + name: dto.name, + }), + );
95-123: Prefer typing the search DTO to the V2 DTO for accuracyUse the actual DTO type to validate the call shape statically and avoid drift.
Apply this diff:
- const searchDto = { - page: 1, - limit: 10, - }; + const searchDto: any = { + page: 1, + limit: 10, + };Follow-up: If available, import and use
FindIssuesByProjectIdDtoV2instead ofany.
59-67: Guard against zero/negative IDs from fakerFaker can yield 0; if your code assumes positive IDs, constrain the range.
Example:
- mockIssue.id = faker.number.int(); + mockIssue.id = faker.number.int({ min: 1 });Repeat similarly for other IDs in this spec for stability.
apps/api/src/domains/admin/project/issue/issue.service.spec.ts (9)
69-95: Duplicate provider for CategoryEntity — prefer override for clarity
IssueServiceProvidersalready includes aCategoryEntityprovider; redefining it here can hide conflicts. UseoverrideProviderfor the tokens you want to replace.Example (conceptual):
const testingModuleBuilder = Test.createTestingModule({ imports: [TestConfig], providers: [...IssueServiceProviders] }); testingModuleBuilder .overrideProvider(getRepositoryToken(ProjectEntity)) .useValue({ /* mocked repo */ }) .overrideProvider(getRepositoryToken(CategoryEntity)) .useValue({ /* mocked repo */ }); const module = await testingModuleBuilder.compile();
120-155: Assert statistics update on successful createYou stub
issueStatisticsService.updateCountbut don’t verify it was invoked on success.Apply this diff:
const issue = await issueService.create(dto); expect(issue.name).toBe(dto.name); expect(issue.project.id).toBe(projectId); expect(issue.status).toBe(IssueStatusEnum.INIT); expect(issue.feedbackCount).toBe(0); + expect(issueStatisticsService.updateCount).toHaveBeenCalledWith( + expect.objectContaining({ + projectId, + date: expect.anything(), + count: 1, + }), + ); expect(eventEmitterSpy).toHaveBeenCalledWith( EventTypeEnum.ISSUE_CREATION, { issueId: issue.id, }, );
267-293: Avoid brittle deep-equality on TypeORM FindOperator (Like/In)Comparing
Like()orIn()objects via deep-equality can be flaky. Preferexpect.any(Object)orexpect.anything()around those fields.Example:
- column: Like(`%${dto.query.column as string}%`), + column: expect.any(Object),Apply similarly in other tests that assert
Like()/In()operators.
468-589: DRY up V2 query builder mocksYou repeat the same chainable mock builder in multiple tests. Extract a small factory to reduce duplication and future maintenance.
Example helper:
function makeListQbMock(result = [issueFixture], total = 1) { return { leftJoinAndSelect: jest.fn().mockReturnThis(), where: jest.fn().mockReturnThis(), andWhere: jest.fn().mockReturnThis(), addOrderBy: jest.fn().mockReturnThis(), offset: jest.fn().mockReturnThis(), limit: jest.fn().mockReturnThis(), getMany: jest.fn().mockResolvedValue(result), getCount: jest.fn().mockResolvedValue(total), }; }
596-606: Assert the specific error on invalid sort methodCatching a generic error can hide regressions. Match the message or custom error type if one exists.
Example:
- await expect(issueService.findIssuesByProjectIdV2(dto)).rejects.toThrow(); + await expect(issueService.findIssuesByProjectIdV2(dto)).rejects.toThrow( + /invalid sort/i, + );
659-681: Relax equality onIn(feedbackIds)to avoid matcher fragilityUse
expect.objectContainingfor thewhereclause instead of deep equality onIn().Apply this diff:
- expect(issueRepo.find).toHaveBeenCalledWith({ - relations: { feedbacks: true }, - where: { feedbacks: { id: In(feedbackIds) } }, - order: { id: 'ASC' }, - }); + expect(issueRepo.find).toHaveBeenCalledWith( + expect.objectContaining({ + relations: { feedbacks: true }, + where: expect.objectContaining({ + feedbacks: expect.objectContaining({ id: expect.any(Object) }), + }), + order: { id: 'ASC' }, + }), + );
824-830: Clarify expected exception on delete-by-id when issue not foundIf the intended behavior is to throw
IssueNotFoundException, assert it explicitly. The current generic expectation can mask accidental changes.Example:
- await expect(issueService.deleteById(issueId)).rejects.toThrow(); + await expect(issueService.deleteById(issueId)).rejects.toThrow( + IssueNotFoundException, + );If the current service intentionally throws another error due to a known bug, consider adding a TODO to update this test when the service is fixed.
894-913: Decouple table name in feedback-count update assertionAsserting
'issues'ties the test to an internal table name. Prefer a looser assertion so refactors (e.g., entity name changes) don't break tests unnecessarily.Example:
- expect(mockQueryBuilder.update).toHaveBeenCalledWith('issues'); + expect(mockQueryBuilder.update).toHaveBeenCalled();
120-123: Constrain faker IDs to positive integersTo avoid accidental zeros, constrain faker IDs where they represent DB identifiers.
Example:
const id = faker.number.int({ min: 1 });Also applies to: 383-407, 693-701, 745-769, 795-823, 835-862, 864-872
apps/api/src/domains/admin/user/create-user.service.spec.ts (6)
74-74: Use enum instead of magic string for signUpMethod.Prefer SignUpMethodEnum.OAUTH for consistency with other expectations.
- expect(user.signUpMethod).toBe('OAUTH'); + expect(user.signUpMethod).toBe(SignUpMethodEnum.OAUTH);
61-61: Restore spies between tests to prevent cross-test leakage.A failing test that mocks rejections (e.g., memberService.create) can affect later tests unless mocks are restored.
userPasswordService = module.get(UserPasswordService); } + afterEach(() => { + jest.restoreAllMocks(); + });
259-264: Avoid brittle message matching in error assertions.Asserting by message couples tests to implementation details. Prefer asserting by error type.
- await expect(createUserService.createInvitationUser(dto)).rejects.toThrow( - 'Member creation failed', - ); + await expect(createUserService.createInvitationUser(dto)).rejects.toThrow(Error);- await expect(createUserService.createInvitationUser(dto)).rejects.toThrow( - 'Password hashing failed', - ); + await expect(createUserService.createInvitationUser(dto)).rejects.toThrow(Error);- await expect(createUserService.createEmailUser(dto)).rejects.toThrow( - 'Password hashing failed', - ); + await expect(createUserService.createEmailUser(dto)).rejects.toThrow(Error);Also applies to: 275-280, 371-376
39-41: Seed faker for deterministic tests (optional).This stabilizes outputs across runs and avoids rare format edge cases.
describe('CreateUserService', () => { let createUserService: CreateUserService; + beforeAll(() => { + faker.seed(42); + });
97-101: Reduce duplication for generating emails with specific domains (optional).A tiny helper keeps tests concise and consistent.
Example helper to add near the top of the file (inside describe or module scope):
const emailAt = (domain: string) => { const [local] = faker.internet.email().split('@'); return `${local}@${domain}`; };Then use: email: emailAt('linecorp.com') / emailAt('invalid.com') / emailAt('anydomain.com').
Also applies to: 114-118, 155-162, 176-183, 289-293, 304-308
231-246: Align test inputs with descriptions and extend stub to accept null
- Change both occurrences in
create-user.service.spec.ts:- tenantRepo.setAllowDomains(undefined); + tenantRepo.setAllowDomains(null);- Update stub signature in
apps/api/src/test-utils/stubs/tenant-repository.stub.tsto:setAllowDomains(domains: string[] | null | undefined = []) { const value: string[] | null = domains ?? null; this.entities?.forEach(entity => { entity.allowDomains = value; }); }apps/api/src/domains/admin/tenant/tenant.controller.spec.ts (3)
39-42: Prefer jest.resetAllMocks() to avoid leaking mock implementationsclearAllMocks resets call history only; resetAllMocks also resets mocked implementations/return values between tests, reducing flakiness.
- // Reset all mocks before each test - jest.clearAllMocks(); + // Reset mocks & their implementations before each test + jest.resetAllMocks();
36-38: Seed faker for deterministic testsSeeding ensures reproducible values across runs.
describe('TenantController', () => { let tenantController: TenantController; + beforeAll(() => { + faker.seed(42); + });
220-221: Use toHaveBeenCalled() for zero-arg callstoHaveBeenCalledWith() with no args is unusual; toHaveBeenCalled() communicates intent more clearly.
- expect(MockTenantService.findOne).toHaveBeenCalledWith(); + expect(MockTenantService.findOne).toHaveBeenCalled();apps/api/src/domains/admin/auth/auth.controller.spec.ts (3)
72-74: Prefer jest.resetAllMocks() to prevent stale mock implementationsRun after each test to reset both call history and any mockResolvedValue/mockReturnValue state.
- afterEach(() => { - jest.clearAllMocks(); - }); + afterEach(() => { + jest.resetAllMocks(); + });
341-367: Add a test for missing/empty OAuth codeConsider a negative test where query.code is missing/empty to assert proper 400 handling.
Example:
+ it('should reject when OAuth code is missing', async () => { + await expect(authController.handleCallback({} as any)).rejects.toThrow(BadRequestException); + expect(authService.signInByOAuth).not.toHaveBeenCalled(); + });
273-293: Make refreshToken test asynchronousUse mockResolvedValue to stub the async service call and await the controller method. For example:
authService.refreshToken.mockResolvedValue(mockTokens as any); const result = await authController.refreshToken(user);Mark the
itcallback asasync.
| it('should handle array exception response', () => { | ||
| const exceptionResponse = ['Error 1', 'Error 2', 'Error 3']; | ||
| const exception = new HttpException( | ||
| exceptionResponse, | ||
| HttpStatus.BAD_REQUEST, | ||
| ); | ||
|
|
||
| filter.catch(exception, mockArgumentsHost); | ||
|
|
||
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.BAD_REQUEST); | ||
| expect(mockResponse.send).toHaveBeenCalledWith({ | ||
| 0: 'Error 1', | ||
| 1: 'Error 2', | ||
| 2: 'Error 3', | ||
| statusCode: HttpStatus.BAD_REQUEST, | ||
| path: '/test-endpoint', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Fix expectation for array responses
HttpExceptionFilter spreads the array payload and still appends both statusCode and path (see Lines 49–65 of http-exception.filter.ts). The current assertion expects only the spread array plus statusCode, which drops the response property that the filter actually adds. Update the expectation to include the response field so the test reflects real behavior.
Apply this diff:
- expect(mockResponse.send).toHaveBeenCalledWith({
- 0: 'Error 1',
- 1: 'Error 2',
- 2: 'Error 3',
- statusCode: HttpStatus.BAD_REQUEST,
- path: '/test-endpoint',
- });
+ expect(mockResponse.send).toHaveBeenCalledWith({
+ response: {
+ 0: 'Error 1',
+ 1: 'Error 2',
+ 2: 'Error 3',
+ },
+ statusCode: HttpStatus.BAD_REQUEST,
+ path: '/test-endpoint',
+ });📝 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.
| it('should handle array exception response', () => { | |
| const exceptionResponse = ['Error 1', 'Error 2', 'Error 3']; | |
| const exception = new HttpException( | |
| exceptionResponse, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| filter.catch(exception, mockArgumentsHost); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.BAD_REQUEST); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| 0: 'Error 1', | |
| 1: 'Error 2', | |
| 2: 'Error 3', | |
| statusCode: HttpStatus.BAD_REQUEST, | |
| path: '/test-endpoint', | |
| }); | |
| }); | |
| it('should handle array exception response', () => { | |
| const exceptionResponse = ['Error 1', 'Error 2', 'Error 3']; | |
| const exception = new HttpException( | |
| exceptionResponse, | |
| HttpStatus.BAD_REQUEST, | |
| ); | |
| filter.catch(exception, mockArgumentsHost); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.BAD_REQUEST); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: { | |
| 0: 'Error 1', | |
| 1: 'Error 2', | |
| 2: 'Error 3', | |
| }, | |
| statusCode: HttpStatus.BAD_REQUEST, | |
| path: '/test-endpoint', | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/common/filters/http-exception.filter.spec.ts around lines 259 to
276, the test expectation for array exception responses is missing the added
response field; update the expected object passed to mockResponse.send to
include the response property (containing the original array elements keyed
numerically or as the array) alongside statusCode and path so it matches the
filter’s behavior that spreads the array and appends both statusCode and path.
| it('should handle boolean exception response', () => { | ||
|
|
||
| const exception = new HttpException(true as any, HttpStatus.OK); | ||
|
|
||
| filter.catch(exception, mockArgumentsHost); | ||
|
|
||
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.OK); | ||
| expect(mockResponse.send).toHaveBeenCalledWith({ | ||
| statusCode: HttpStatus.OK, | ||
| path: '/test-endpoint', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Restore response field expectation for primitive payloads
For primitive payloads (boolean) the filter wraps the value under the response key before appending statusCode and path. The assertion currently omits that response key, so the test will fail. Adjust it to include the wrapped payload.
Apply this diff:
- expect(mockResponse.send).toHaveBeenCalledWith({
- statusCode: HttpStatus.OK,
- path: '/test-endpoint',
- });
+ expect(mockResponse.send).toHaveBeenCalledWith({
+ response: true,
+ statusCode: HttpStatus.OK,
+ path: '/test-endpoint',
+ });🤖 Prompt for AI Agents
In apps/api/src/common/filters/http-exception.filter.spec.ts around lines 278 to
289, the test for boolean exception payload omits the wrapped `response` field
that the filter adds for primitive payloads; update the assertion for
mockResponse.send to expect an object including `response: true` along with the
existing `statusCode: HttpStatus.OK` and `path: '/test-endpoint'` so the test
matches the filter's actual output.
| it('should handle number exception response', () => { | ||
|
|
||
| const exception = new HttpException(42 as any, HttpStatus.OK); | ||
|
|
||
| filter.catch(exception, mockArgumentsHost); | ||
|
|
||
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.OK); | ||
| expect(mockResponse.send).toHaveBeenCalledWith({ | ||
| statusCode: HttpStatus.OK, | ||
| path: '/test-endpoint', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Align number-response assertion with filter output
As with booleans, numeric responses are wrapped under response by the filter (Lines 52–65 of http-exception.filter.ts). The current expectation omits that field, so it needs to be updated accordingly.
Apply this diff:
- expect(mockResponse.send).toHaveBeenCalledWith({
- statusCode: HttpStatus.OK,
- path: '/test-endpoint',
- });
+ expect(mockResponse.send).toHaveBeenCalledWith({
+ response: 42,
+ statusCode: HttpStatus.OK,
+ path: '/test-endpoint',
+ });📝 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.
| it('should handle number exception response', () => { | |
| const exception = new HttpException(42 as any, HttpStatus.OK); | |
| filter.catch(exception, mockArgumentsHost); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.OK); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| statusCode: HttpStatus.OK, | |
| path: '/test-endpoint', | |
| }); | |
| }); | |
| it('should handle number exception response', () => { | |
| const exception = new HttpException(42 as any, HttpStatus.OK); | |
| filter.catch(exception, mockArgumentsHost); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.OK); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: 42, | |
| statusCode: HttpStatus.OK, | |
| path: '/test-endpoint', | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/common/filters/http-exception.filter.spec.ts around lines 291 to
302, the test for numeric exception response expects the body without the
wrapping response field; update the assertion to match the filter output by
expecting mockResponse.send to have been called with an object containing
statusCode: HttpStatus.OK, response: 42, and path: '/test-endpoint' (i.e., add
the response: 42 field to the expected payload).
|
|
||
| jest | ||
| .spyOn(authService.codeService, 'getDataByCodeAndType') | ||
| .mockResolvedValue({ | ||
| userType: UserTypeEnum.GENERAL, | ||
| roleId: faker.number.int(), | ||
| invitedBy: new UserDto(), | ||
| } as any); | ||
|
|
||
| // Mock the createUserService to avoid complex dependencies | ||
| const mockUser = new UserEntity(); | ||
| mockUser.signUpMethod = SignUpMethodEnum.EMAIL; | ||
| mockUser.email = faker.internet.email(); | ||
|
|
||
| jest | ||
| .spyOn(authService.createUserService, 'createInvitationUser') | ||
| .mockResolvedValue(mockUser); |
There was a problem hiding this comment.
Fix private dependency access in tests
Line 216 and Line 229 reach into authService.codeService / authService.createUserService, but both of those members are declared private inside AuthService. That’s exactly why CI is failing with “Property 'codeService' is private and only accessible within class 'AuthService'.” Please obtain those collaborators via the testing module (e.g., keep the compiled module in a moduleRef, then const codeService = moduleRef.get(CodeService) as jest.Mocked<CodeService> and spy on that) or reuse the mocks that AuthServiceProviders already exports, instead of touching private members. Apply the same strategy for createInvitationUser.
🧰 Tools
🪛 GitHub Actions: CI
[error] 216-216: Property 'codeService' is private and only accessible within class 'AuthService'.
| // Mock projectService.findById to throw error | ||
| jest | ||
| .spyOn(channelService.projectService, 'findById') | ||
| .mockRejectedValue(new Error('Project not found')); | ||
|
|
||
| await expect(channelService.create(dto)).rejects.toThrow(); |
There was a problem hiding this comment.
Stop accessing ChannelService internals directly
Every new test spies on channelService.projectService, channelMySQLService, fieldService, configService, and osRepository. In ChannelService those collaborators are injected as private readonly constructor params, so tsc --noEmit (and editors) will raise “Property 'projectService' is private and only accessible within class 'ChannelService'.” Please grab the collaborators from the testing module instead (e.g., keep a projectService = module.get(ProjectService) reference during beforeEach) and spy on those mocks, then update all similar usages in this spec accordingly.
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/channel/channel/channel.service.spec.ts around
lines 113 to 118, tests are reaching into ChannelService private fields
(projectService, channelMySQLService, fieldService, configService, osRepository)
to create spies which breaks TypeScript access rules; update the spec to
retrieve these collaborators from the testing module in beforeEach (e.g.,
projectService = module.get(ProjectService)) and then call jest.spyOn on those
module-level variables instead of channelService.projectService, doing the same
for channelMySQLService, fieldService, configService and osRepository throughout
the file so all spies reference the mocks obtained from module.get rather than
private properties on ChannelService.
| expect(mockMailerService.sendMail).toHaveBeenCalledTimes(1); | ||
| expect(mockMailerService.sendMail).toHaveBeenCalledWith({ | ||
| to: mockEmail, | ||
| subject: 'User Feedback Invitation', | ||
| context: { | ||
| link: `/link/user-invitation?code=${mockCode}&email=${mockEmail}`, | ||
| baseUrl: '', | ||
| }, | ||
| template: 'invitation', | ||
| }); | ||
| }); | ||
|
|
||
| it('should call sendMail with correct parameters', async () => { | ||
| await userInvitationMailingService.send({ | ||
| code: mockCode, | ||
| email: mockEmail, | ||
| }); | ||
|
|
||
| const callArgs = mockMailerService.sendMail.mock.calls[0][0]; | ||
| expect(callArgs.to).toBe(mockEmail); | ||
| expect(callArgs.subject).toBe('User Feedback Invitation'); | ||
| expect(callArgs.context?.link).toBe( | ||
| `/link/user-invitation?code=${mockCode}&email=${mockEmail}`, | ||
| ); | ||
| expect(callArgs.context?.baseUrl).toBe(''); | ||
| expect(callArgs.template).toBe('invitation'); | ||
| }); |
There was a problem hiding this comment.
Align expectations with actual baseUrl handling.
The service forwards smtpConfig.baseUrl to the mail template (see UserInvitationMailingService.send). With the current stub returning { baseUrl: mockBaseUrl }, context.baseUrl should be that URL—not an empty string. The assertions here will fail against the real implementation.
Update the expectations to assert mockBaseUrl (and only expect the empty string in the dedicated “not available” test).
🤖 Prompt for AI Agents
In apps/api/src/shared/mailing/user-invitation-mailing.service.spec.ts around
lines 77 to 103, the test asserts context.baseUrl is an empty string but the
service forwards smtpConfig.baseUrl; update the assertions to expect mockBaseUrl
instead of '' (only leave '' in the separate test that verifies missing
baseUrl). Modify both the sendMail call expectation and the
callArgs.context.baseUrl assertion to use mockBaseUrl so the test matches the
stubbed smtpConfig.
| it('should use empty string when baseUrl is not available', async () => { | ||
| jest.spyOn(mockConfigService, 'get').mockReturnValue({ baseUrl: '' }); | ||
| const code = faker.string.alphanumeric(10); | ||
| const email = faker.internet.email(); | ||
|
|
||
| await userInvitationMailingService.send({ code, email }); | ||
|
|
||
| expect(mockMailerService.sendMail).toHaveBeenCalledWith({ | ||
| to: email, | ||
| subject: 'User Feedback Invitation', | ||
| context: { | ||
| link: `/link/user-invitation?code=${code}&email=${email}`, | ||
| baseUrl: '', | ||
| }, | ||
| template: 'invitation', | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle null configService response correctly', async () => { | ||
| jest.spyOn(mockConfigService, 'get').mockReturnValue(null); | ||
| const code = faker.string.alphanumeric(10); | ||
| const email = faker.internet.email(); | ||
|
|
||
| await userInvitationMailingService.send({ code, email }); | ||
|
|
||
| expect(mockMailerService.sendMail).toHaveBeenCalledWith({ | ||
| to: email, | ||
| subject: 'User Feedback Invitation', | ||
| context: { | ||
| link: `/link/user-invitation?code=${code}&email=${email}`, | ||
| baseUrl: '', | ||
| }, | ||
| template: 'invitation', | ||
| }); | ||
| }); | ||
|
|
||
| it('should handle undefined baseUrl in smtp config correctly', async () => { | ||
| jest | ||
| .spyOn(mockConfigService, 'get') | ||
| .mockReturnValue({ baseUrl: undefined }); | ||
| const code = faker.string.alphanumeric(10); | ||
| const email = faker.internet.email(); | ||
|
|
||
| await userInvitationMailingService.send({ code, email }); | ||
|
|
||
| expect(mockMailerService.sendMail).toHaveBeenCalledWith({ | ||
| to: email, | ||
| subject: 'User Feedback Invitation', | ||
| context: { | ||
| link: `/link/user-invitation?code=${code}&email=${email}`, | ||
| baseUrl: '', | ||
| }, | ||
| template: 'invitation', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Update baseUrl fallback assertions.
For the tests where you override ConfigService.get to return { baseUrl: '' }, null, or { baseUrl: undefined }, verify the override by setting mockConfigService.get.mockReturnValueOnce(...) rather than adding another jest.spyOn. Also ensure each assertion matches the intended fallback: empty string when the config is blank/nullable, and the supplied URL when it exists.
🤖 Prompt for AI Agents
apps/api/src/shared/mailing/user-invitation-mailing.service.spec.ts lines
105-159: the tests override ConfigService.get using jest.spyOn and
mockReturnValue which can leak between tests and don't assert the intended
fallback behavior; change each override to
mockConfigService.get.mockReturnValueOnce(...) (or mockReturnValueOnce on the
spy) to ensure the stub is applied only for that test, and update assertions so
that when get returns '' / null / { baseUrl: undefined } the expected
context.baseUrl is '' and when get returns a real URL the expected
context.baseUrl equals that URL; keep the rest of the test setup and
expectations the same but use the one-time mock to avoid cross-test pollution.
| it('should construct invitation link correctly with different baseUrls', async () => { | ||
| const testBaseUrls = [ | ||
| 'https://app.example.com', | ||
| 'http://localhost:3000', | ||
| 'https://staging.example.com', | ||
| '', | ||
| ]; | ||
| const mockCode = faker.string.alphanumeric(10); | ||
| const mockEmail = faker.internet.email(); | ||
|
|
||
| for (const baseUrl of testBaseUrls) { | ||
| jest.spyOn(mockConfigService, 'get').mockReturnValue({ baseUrl }); | ||
| await userInvitationMailingService.send({ | ||
| code: mockCode, | ||
| email: mockEmail, | ||
| }); | ||
|
|
||
| const expectedLink = `/link/user-invitation?code=${mockCode}&email=${mockEmail}`; | ||
|
|
||
| expect(mockMailerService.sendMail).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| context: expect.objectContaining({ | ||
| link: expectedLink, | ||
| baseUrl: '', | ||
| }), | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Reset baseUrl expectations per iteration.
In this loop you always expect context.baseUrl to be '', but when baseUrl is 'https://app.example.com', 'http://localhost:3000', etc., the service will pass that through. Use mockConfigService.get.mockReturnValueOnce({ baseUrl }) and assert that context.baseUrl equals the iterated value.
🤖 Prompt for AI Agents
In apps/api/src/shared/mailing/user-invitation-mailing.service.spec.ts around
lines 225 to 252, the test loop always asserts context.baseUrl is '' which is
incorrect for non-empty testBaseUrls; change the mock to return the baseUrl per
iteration using mockReturnValueOnce({ baseUrl }) and update the expectation to
assert context.baseUrl equals the iterated baseUrl (not always ''), ensuring
each iteration both stubs and verifies the corresponding baseUrl value.
| const startDate = '2024-02-28'; | ||
| const endDate = '2024-03-01'; | ||
|
|
||
| const result = calculateDaysBetweenDates(startDate, endDate); | ||
|
|
||
| expect(result).toBe(2); | ||
| }); |
There was a problem hiding this comment.
Leap-year assertion mismatches actual diff.
calculateDaysBetweenDates delegates to DateTime.diff. Between 2024-02-28 and 2024-03-01, diff(..., 'days').days resolves to 2 because the Luxon diff spans 48 hours. If the intended semantics are pure calendar-day difference (expect 2), the implementation must change; otherwise the test should assert 2 to match current behavior.
🤖 Prompt for AI Agents
In apps/api/src/utils/date-utils.spec.ts around lines 54-60, the test expects a
calendar-day difference but the implementation uses Luxon DateTime.diff which
yields a 48-hour span (2 days) between 2024-02-28 and 2024-03-01; decide
intended semantics and fix accordingly: if you want calendar-day difference,
change calculateDaysBetweenDates to compute difference based on calendar dates
(e.g., compare ISO dates or normalize both datetimes to startOf('day') and count
distinct calendar days) so the test can assert the expected value, otherwise
update this test to assert 2 to match current Duration-based behavior.
| it('should sign in email user successfully', () => { | ||
| const user = new UserDto(); | ||
| user.id = faker.number.int(); | ||
| user.email = faker.internet.email(); | ||
| user.name = faker.person.fullName(); | ||
| const mockTokens = { | ||
| accessToken: faker.string.alphanumeric(32), | ||
| refreshToken: faker.string.alphanumeric(32), | ||
| }; | ||
|
|
||
|
|
||
| authService.signIn.mockReturnValue(mockTokens as any); | ||
|
|
||
| const result = authController.signInEmail(user); | ||
|
|
||
| expect(authService.signIn).toHaveBeenCalledWith(user); | ||
| expect(authService.signIn).toHaveBeenCalledTimes(1); | ||
| expect(result).toEqual(mockTokens); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm signInEmail returns synchronously
The test uses mockReturnValue and does not await. If AuthController.signInEmail is async or returns a Promise, switch to mockResolvedValue and await the call.
Run to confirm method signature:
If async, adjust test:
- authService.signIn.mockReturnValue(mockTokens as any);
- const result = authController.signInEmail(user);
+ authService.signIn.mockResolvedValue(mockTokens as any);
+ const result = await authController.signInEmail(user);🏁 Script executed:
#!/bin/bash
# Find AuthController.signInEmail definition and check for async/return type
ast-grep --pattern $'class AuthController {\n $$$\n signInEmail($_) {\n $$$\n }\n $$$\n}' || rg -nP -C3 '(?s)class\s+AuthController\b.*?\bsignInEmail\s*\(' --type tsLength of output: 5739
#!/bin/bash
set -e
Locate AuthService implementation
file=$(rg -lP 'export class AuthService' --type ts | head -1)
echo "AuthService file: $file"
Show signIn method signature and context
rg -nP '(async\s+)?signIn\s*(' -nC3 "$file"
🏁 Script executed:
#!/bin/bash
set -e
# Locate AuthService implementation file
file=$(rg -lP 'export class AuthService\b' -g '*.ts' | head -n1)
echo "AuthService file: $file"
# Display signIn method signature and surrounding lines
rg -nP '(async\s+)?signIn\s*\(' -nC3 "$file"Length of output: 488
🏁 Script executed:
#!/bin/bash
set -e
file="apps/api/src/domains/admin/auth/auth.service.ts"
echo "Inspecting $file"
rg -P '^\s*(?:async\s+)?signIn\s*\(' -nC2 "$file"Length of output: 189
Await controller Promise in test
Mark the it callback async, switch to mockResolvedValue, and await the call:
-it('should sign in email user successfully', () => {
+it('should sign in email user successfully', async () => {
// …
- authService.signIn.mockReturnValue(mockTokens as any);
- const result = authController.signInEmail(user);
+ authService.signIn.mockResolvedValue(mockTokens as any);
+ const result = await authController.signInEmail(user);🤖 Prompt for AI Agents
In apps/api/src/domains/admin/auth/auth.controller.spec.ts around lines 252 to
271, the test currently treats an async controller method as synchronous; update
the test by marking the it callback async, change
authService.signIn.mockReturnValue to mockResolvedValue with the mockTokens, and
await authController.signInEmail(user) when calling the controller; keep the
existing expectations (calledWith, calledTimes, and result equality) but compare
against the awaited result.
| it('should handle invalid fieldId parameter', async () => { | ||
| const invalidFieldId = 'invalid' as unknown as number; | ||
|
|
||
| await expect( | ||
| optionController.getOptions(invalidFieldId), | ||
| ).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Reset the OptionService mocks to avoid false positives
The previous test on Line 94 sets MockSelectOptionService.findByFieldId to mockRejectedValue(error) and we never restore it. When we get here, the promise still rejects because of that leftover mock, so this assertion passes even if getOptions happily accepted a string fieldId. We’re not really exercising the “invalid fieldId” path and a future regression would go unnoticed. Please reset the service mocks before each scenario (e.g. via jest.resetAllMocks() in the suite beforeEach, or by explicitly mockResetting findByFieldId) so the test arranges the behavior it relies on.
beforeEach(async () => {
+ jest.resetAllMocks();
const module = await Test.createTestingModule({
controllers: [OptionController],
providers: [📝 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.
| it('should handle invalid fieldId parameter', async () => { | |
| const invalidFieldId = 'invalid' as unknown as number; | |
| await expect( | |
| optionController.getOptions(invalidFieldId), | |
| ).rejects.toThrow(); | |
| }); | |
| beforeEach(async () => { | |
| jest.resetAllMocks(); | |
| const module = await Test.createTestingModule({ | |
| controllers: [OptionController], | |
| providers: [ | |
| // … existing providers here | |
| ], | |
| }).compile(); | |
| optionController = module.get<OptionController>(OptionController); | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/channel/option/option.controller.spec.ts around
lines 197 to 203, the test for an invalid fieldId is observing a leftover
rejection from a prior test because MockSelectOptionService.findByFieldId was
previously set to mockRejectedValue and never restored; reset the relevant mocks
before this scenario so the test arranges its own behavior — add a global
beforeEach that calls jest.resetAllMocks() (or explicitly call
mockReset/mockClear on MockSelectOptionService.findByFieldId) so this test
actually verifies validation of the input rather than inheriting a rejected
promise from an earlier test.
| describe('create edge cases', () => { | ||
| it('creating an option with empty key succeeds', async () => { | ||
| const fieldId = faker.number.int(); | ||
| const name = faker.string.sample(); | ||
| const dto = new CreateOptionDto(); | ||
| dto.fieldId = fieldId; | ||
| dto.key = ''; | ||
| dto.name = name; | ||
| jest.spyOn(optionRepo, 'findBy').mockResolvedValue([]); | ||
| jest | ||
| .spyOn(optionRepo, 'save') | ||
| .mockImplementation(() => Promise.resolve({ key: '', name } as any)); | ||
|
|
||
| const result = await optionService.create(dto); | ||
|
|
||
| expect(result.key).toBe(''); | ||
| }); | ||
|
|
||
| it('creating an option with empty name succeeds', async () => { | ||
| const fieldId = faker.number.int(); | ||
| const key = faker.string.sample(); | ||
| const dto = new CreateOptionDto(); | ||
| dto.fieldId = fieldId; | ||
| dto.key = key; | ||
| dto.name = ''; | ||
| jest.spyOn(optionRepo, 'findBy').mockResolvedValue([]); | ||
| jest | ||
| .spyOn(optionRepo, 'save') | ||
| .mockImplementation(() => Promise.resolve({ key, name: '' } as any)); | ||
|
|
||
| const result = await optionService.create(dto); | ||
|
|
||
| expect(result.name).toBe(''); | ||
| }); | ||
|
|
||
| it('creating an option with null fieldId succeeds', async () => { | ||
| const dto = new CreateOptionDto(); | ||
|
|
||
| dto.fieldId = null as any; | ||
| dto.key = faker.string.sample(); | ||
| dto.name = faker.string.sample(); | ||
| jest.spyOn(optionRepo, 'findBy').mockResolvedValue([]); | ||
| jest | ||
| .spyOn(optionRepo, 'save') | ||
| .mockImplementation(() => | ||
| Promise.resolve({ key: '', name: faker.string.sample() } as any), | ||
| ); | ||
|
|
||
| const result = await optionService.create(dto); | ||
|
|
||
| expect(result).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('createMany edge cases', () => { | ||
| it('creating many options with empty array succeeds', async () => { | ||
| const fieldId = faker.number.int(); | ||
| const dto = new CreateManyOptionsDto(); | ||
| dto.fieldId = fieldId; | ||
| dto.options = []; | ||
| jest | ||
| .spyOn(optionRepo, 'save') | ||
| .mockImplementation(() => Promise.resolve([] as any)); | ||
|
|
||
| const result = await optionService.createMany(dto); | ||
|
|
||
| expect(result).toEqual([]); | ||
| expect(optionRepo.save).toHaveBeenCalledWith([]); | ||
| }); | ||
|
|
||
| it('creating many options with single option succeeds', async () => { | ||
| const fieldId = faker.number.int(); | ||
| const dto = new CreateManyOptionsDto(); | ||
| dto.fieldId = fieldId; | ||
| dto.options = [ | ||
| { key: faker.string.sample(), name: faker.string.sample() }, | ||
| ]; | ||
| jest | ||
| .spyOn(optionRepo, 'save') | ||
| .mockImplementation(() => Promise.resolve([{}] as any)); | ||
|
|
||
| const result = await optionService.createMany(dto); | ||
|
|
||
| expect(result).toHaveLength(1); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Clarify repository behavior in edge-case tests.
The edge-case blocks still assert on optionRepo.save results without defining the return shape. Mock them explicitly so the tests stay meaningful. Otherwise the assertions interact with undefined.
| describe('replaceMany edge cases', () => { | ||
| it('replacing many options with null options array succeeds', async () => { | ||
| const fieldId = faker.number.int(); | ||
| const dto = new ReplaceManyOptionsDto(); | ||
| dto.fieldId = fieldId; | ||
|
|
||
| dto.options = null as any; | ||
| jest.spyOn(optionRepo, 'find').mockResolvedValue([]); | ||
| jest.spyOn(optionRepo, 'query'); | ||
| jest.spyOn(optionRepo, 'save'); | ||
|
|
||
| await optionService.replaceMany(dto); | ||
|
|
||
| expect(optionRepo.save).toHaveBeenCalledTimes(0); | ||
| }); | ||
|
|
||
| it('replacing many options with undefined options array succeeds', async () => { | ||
| const fieldId = faker.number.int(); | ||
| const dto = new ReplaceManyOptionsDto(); | ||
| dto.fieldId = fieldId; | ||
|
|
||
| dto.options = undefined as any; | ||
| jest.spyOn(optionRepo, 'find').mockResolvedValue([]); | ||
| jest.spyOn(optionRepo, 'query'); | ||
| jest.spyOn(optionRepo, 'save'); | ||
|
|
||
| await optionService.replaceMany(dto); | ||
|
|
||
| expect(optionRepo.save).toHaveBeenCalledTimes(0); | ||
| }); |
There was a problem hiding this comment.
Mock save for null/undefined options arrays.
Even when no options are persisted, replaceMany still awaits optionRepo.save. Make the stub resolve (e.g., mockResolvedValue([])) so the call count assertion doesn’t rely on implicit mock defaults.
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/channel/option/option.service.spec.ts around lines
425 to 454, the tests for replaceMany that set dto.options to null/undefined spy
on optionRepo.save but do not make it a resolving stub, so the test relies on
Jest's implicit mock behavior; update both tests to stub optionRepo.save with a
resolved value (e.g., jest.spyOn(optionRepo, 'save').mockResolvedValue([])) so
the call count/assertions are deterministic and do not depend on implicit
defaults.
| it('throws NotFoundException when feedback is not found', async () => { | ||
| // Mock repository to return null | ||
| const feedbackRepo = webhookListener.feedbackRepo; | ||
|
|
||
| jest.spyOn(feedbackRepo, 'findOne').mockResolvedValue(null); | ||
|
|
||
| await expect( | ||
| webhookListener.handleFeedbackCreation({ | ||
| feedbackId: faker.number.int(), | ||
| }), | ||
| ).rejects.toThrow(NotFoundException); | ||
| }); | ||
|
|
||
| it('handles HTTP errors gracefully when sending webhooks', async () => { | ||
| const axiosError: AxiosError = { | ||
| name: 'AxiosError', | ||
| message: 'Request failed', | ||
| code: 'ECONNREFUSED', | ||
| response: { | ||
| data: { error: 'Connection refused' }, | ||
| status: 500, | ||
| statusText: 'Internal Server Error', | ||
| headers: {}, | ||
| config: {} as any, | ||
| }, | ||
| config: {} as any, | ||
| isAxiosError: true, | ||
| toJSON: () => ({}), | ||
| }; | ||
|
|
||
| jest | ||
| .spyOn(httpService, 'post') | ||
| .mockImplementation(() => throwError(() => axiosError)); | ||
|
|
||
| // Should not throw error, but handle gracefully | ||
| await expect( | ||
| webhookListener.handleFeedbackCreation({ | ||
| feedbackId: faker.number.int(), | ||
| }), | ||
| ).resolves.not.toThrow(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Fix false positive: handleFeedbackCreation should surface NotFound
The real implementation throws a NotFoundException when the feedback row is missing. This test now expects the call to resolve, so it will pass even if handleFeedbackCreation stops surfacing the error. Please change the assertion to rejects.toThrow(NotFoundException) so the test defends the actual behavior.
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts around
lines 69 to 110, the second test currently expects handleFeedbackCreation to
resolve when HTTP errors occur which masks when the function should surface a
NotFoundException; update the test assertion for the relevant case so it expects
the call to reject with NotFoundException (use
rejects.toThrow(NotFoundException)) ensuring the spec validates that missing
feedback results in a NotFoundException rather than silently resolving.
| describe('retry logic and logging', () => { | ||
| it('handles retry logic for failed requests', async () => { | ||
| let callCount = 0; | ||
| const axiosError: AxiosError = { | ||
| name: 'AxiosError', | ||
| message: 'Request failed', | ||
| code: 'ECONNREFUSED', | ||
| response: undefined, | ||
| config: {} as any, | ||
| isAxiosError: true, | ||
| toJSON: () => ({}), | ||
| }; | ||
|
|
||
| jest.spyOn(httpService, 'post').mockImplementation(() => { | ||
| callCount++; | ||
| // Always return error to test retry behavior | ||
| return throwError(() => axiosError); | ||
| }); | ||
|
|
||
| await webhookListener.handleFeedbackCreation({ | ||
| feedbackId: faker.number.int(), | ||
| }); | ||
|
|
||
| // Should make at least one call (retry behavior may vary in test environment) | ||
| expect(callCount).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
Retry test only asserts “called > 0”
The code under test is supposed to retry up to MAX_RETRIES times, yet the test passes as soon as the first call happens. That doesn’t verify the retry loop at all. Please assert the expected call count so the test fails if the implementation stops retrying.
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts around
lines 486-511, the retry test currently only asserts callCount > 0 which doesn't
verify the retry loop; update the test to assert the exact expected number of
attempts by importing or referencing the MAX_RETRIES constant used by
webhookListener and replace the loose expectation with
expect(callCount).toBe(MAX_RETRIES) (or toBeGreaterThanOrEqual(MAX_RETRIES) if
initial attempt semantics differ), ensuring the mock still always errors so the
retry loop runs the full number of retries.
| import { TenantServiceProviders } from '../../../test-utils/providers/tenant.service.providers'; | ||
| import { FeedbackEntity } from '../feedback/feedback.entity'; | ||
| import { UserEntity } from '../user/entities/user.entity'; | ||
| import { UserPasswordService } from '../user/user-password.service'; |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm DI wiring for UserPasswordService in test providers
Good addition. Please ensure TenantServiceProviders registers UserPasswordService, or DI will fail at runtime.
Also applies to: 57-57
🏁 Script executed:
#!/bin/bash
# Locate provider registration and class/methods
rg -nP --type=ts -C3 'TenantServiceProviders|tenant\.service\.providers'
rg -nP --type=ts -C3 '\bclass\s+UserPasswordService\b'
rg -nP --type=ts -C2 '\bcreateHashPassword\s*\('Length of output: 11779
Add UserPasswordServiceProviders to TenantServiceProviders
TenantService injects UserPasswordService; in apps/api/src/test-utils/providers/tenant.service.providers.ts import and include ...UserPasswordServiceProviders in the TenantServiceProviders array to prevent DI failures.
🤖 Prompt for AI Agents
In apps/api/src/test-utils/providers/tenant.service.providers.ts around the area
referenced by apps/api/src/domains/admin/tenant/tenant.service.spec.ts line 27,
the TenantService test providers are missing the UserPasswordService providers
which causes dependency injection failures; update the file to import
UserPasswordServiceProviders from the user-password.service providers module and
add `...UserPasswordServiceProviders` into the TenantServiceProviders array so
the UserPasswordService dependency is registered for tests.
| accessTokenRequestURL: faker.string.sample(), | ||
| userProfileRequestURL: faker.string.sample(), | ||
| emailKey: faker.string.sample(), | ||
| defatulLoginEnable: faker.datatype.boolean(), |
There was a problem hiding this comment.
🧩 Analysis chain
Likely typo in oauthConfig: defatulLoginEnable → defaultLoginEnable
This can hide bugs and/or break type checks.
- defatulLoginEnable: faker.datatype.boolean(),
+ defaultLoginEnable: faker.datatype.boolean(),Run to verify usage across the repo:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C2 '(?<!\w)(defatulLoginEnable|defaultLoginEnable)(?!\w)'Length of output: 68
🏁 Script executed:
#!/bin/bash
# Search for both the typo and the corrected key across the repo
rg -n --type=ts 'defatulLoginEnable'
rg -n --type=ts 'defaultLoginEnable'Length of output: 200
🏁 Script executed:
#!/bin/bash
# Display the Tenant entity definition around the typo to confirm decorator and DB mapping
sed -n '1,60p' apps/api/src/domains/admin/tenant/tenant.entity.tsLength of output: 1857
Correct typo in OAuthConfig and tests (defatulLoginEnable → defaultLoginEnable)
Rename the property in both files:
- apps/api/src/domains/admin/tenant/tenant.entity.ts (interface OAuthConfig)
- apps/api/src/domains/admin/tenant/tenant.service.spec.ts (test fixture)
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/tenant/tenant.service.spec.ts around line 145 (and
also update apps/api/src/domains/admin/tenant/tenant.entity.ts for the
OAuthConfig interface), there is a typo: property "defatulLoginEnable" should be
renamed to "defaultLoginEnable"; update the OAuthConfig interface in
tenant.entity.ts to use defaultLoginEnable, update the test fixture in
tenant.service.spec.ts to the same property name, and search/replace any other
occurrences/usages in the codebase to match the corrected property name, then
run TypeScript build/tests to ensure no remaining references break.
| it('should call deleteOldFeedbacks method', async () => { | ||
| // This test verifies that the method exists and can be called | ||
| // The actual implementation details are tested through integration tests | ||
| await expect(tenantService.deleteOldFeedbacks()).resolves.not.toThrow(); |
There was a problem hiding this comment.
Fix Jest assertion: toThrow expects a function, not a Promise
Use Promise matchers to assert non-throwing async calls.
- await expect(tenantService.deleteOldFeedbacks()).resolves.not.toThrow();
+ await expect(tenantService.deleteOldFeedbacks()).resolves.toBeUndefined();- await expect(tenantService.addCronJob()).resolves.not.toThrow();
+ await expect(tenantService.addCronJob()).resolves.toBeUndefined();Also applies to: 259-259
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/tenant/tenant.service.spec.ts around lines 251 and
259, the test uses expect(...).resolves.not.toThrow() which is invalid because
toThrow expects a function, not a Promise; replace those assertions with
Promise-aware matchers such as await
expect(tenantService.deleteOldFeedbacks()).resolves.toBeUndefined() (or another
appropriate resolves matcher that matches the method's return), updating both
occurrences at lines 251 and 259.
There was a problem hiding this comment.
Actionable comments posted: 18
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/channel/channel/channel.service.spec.ts (1)
17-60: Stop bypassing ChannelService private dependenciesCasting
channelServicetoany, disabling@typescript-eslint/no-unsafe-member-access, and spying throughchannelServiceAny.projectService,channelMySQLService,fieldService,configService, andosRepositoryreintroduces the private-field access that the earlier review asked you to remove. This sidesteps both the type system and eslint/null safety, and it still ties the spec to ChannelService internals. Please revert to the original guidance: resolve each collaborator from the testing module inbeforeEach(e.g.,projectService = module.get(ProjectService)) and spy on those mocks directly, dropping theas anyhelper and the lint disable. Apply the same pattern for every collaborator currently accessed viachannelServiceAny.*.
🧹 Nitpick comments (19)
apps/api/src/domains/admin/project/role/role.service.spec.ts (2)
37-44: Add afterEach to restore mocks (prevents cross-test leakage)Restore spies/mocks between tests to avoid subtle interference.
beforeEach(async () => { const module = await Test.createTestingModule({ imports: [TestConfig], providers: RoleServiceProviders, }).compile(); roleService = module.get(RoleService); roleRepo = module.get(getRepositoryToken(RoleEntity)); }); + + afterEach(() => { + jest.restoreAllMocks(); + });
675-699: Sanity check: null handling may mask validation gapsIf DTO/service should reject nulls, consider asserting a validation error instead. If allowed by design, this is fine.
apps/api/src/shared/code/code.service.spec.ts (1)
376-389: Guard against random collision flakiness in code generation test
setCodeproduces random 6-digit strings, so there’s a small but non-zero chance (≈1e-6) that two successive calls collide and make this spec fail sporadically. Consider stubbing the internal generator or seeding the RNG so you can assert deterministic outputs instead of comparing two random samples.apps/api/src/domains/admin/project/issue/issue.service.spec.ts (8)
59-65: Remove unused TestModule lookups to reduce noise (optional)_projectRepo, _schedulerRegistry, and _schedulerLockService are never used. If you keep the stubs (previous comment), consider removing these gets to simplify the setup.
- let _projectRepo: Repository<ProjectEntity>; - let _schedulerRegistry: SchedulerRegistry; - let _schedulerLockService: SchedulerLockService; @@ - _projectRepo = module.get(getRepositoryToken(ProjectEntity)); - _schedulerRegistry = module.get<SchedulerRegistry>(SchedulerRegistry); - _schedulerLockService = - module.get<SchedulerLockService>(SchedulerLockService);Also applies to: 101-109
447-465: Use class matcher in toThrow for stabilityComparing against an Error instance is brittle. Prefer constructor-based assertion.
- await expect(issueService.update(dto)).rejects.toThrow( - new IssueInvalidNameException('Duplicated name'), - ); + await expect(issueService.update(dto)).rejects.toThrow( + IssueInvalidNameException, + );
183-216: Avoid asserting TypeORM internals in setFindOptions (brittle tests)Relying on internal properties like _objectLiteralParameters and exact call counts makes tests fragile across TypeORM/pagination changes. Loosen the expectations.
- expect(createQueryBuilder.setFindOptions).toHaveBeenCalledTimes(2); + expect(createQueryBuilder.setFindOptions).toHaveBeenCalled(); @@ - createdAt: expect.objectContaining({ - _objectLiteralParameters: { - ...(dto.query.createdAt as TimeRange), - }, - }) as TimeRange | string, + createdAt: expect.any(Object),
172-181: Guard spy restoration to avoid undefined access (optional)If this block evolves or test ordering changes, restoring an undefined spy will throw. Add a tiny guard.
- (spy as jest.SpyInstance).mockRestore(); + (spy as jest.SpyInstance | undefined)?.mockRestore?.();
488-497: DRY up duplicated mock QueryBuilder definitions (optional)The same mock shape is repeated across V2 tests. Extract a factory to reduce duplication and ease future changes.
Example helper within this spec file:
function makeIssueQbMock(result = [issueFixture], count = 1) { return { leftJoinAndSelect: jest.fn().mockReturnThis(), where: jest.fn().mockReturnThis(), andWhere: jest.fn().mockReturnThis(), addOrderBy: jest.fn().mockReturnThis(), offset: jest.fn().mockReturnThis(), limit: jest.fn().mockReturnThis(), getMany: jest.fn().mockResolvedValue(result), getCount: jest.fn().mockResolvedValue(count), }; }Then:
jest.spyOn(issueRepo, 'createQueryBuilder').mockReturnValue(makeIssueQbMock() as any);Also applies to: 515-524, 575-584
827-829: Localize comment to English for consistencyKeep test comments in English to align with repo conventions.
- // IssueService에서 new IssueEntity()를 생성하므로 project가 undefined가 되어 오류 발생 + // IssueService creates new IssueEntity(), so project is undefined and causes an error
69-95: Note on duplicate CategoryEntity repository providersIssueServiceProviders already registers getRepositoryToken(CategoryEntity). This spec re-registers it with a stub (which overrides the prior one). That’s fine in NestJS (last one wins), but be aware to avoid confusion.
1-1: CI webhook failures are unrelated to these tests; isolate external callsCI shows axios errors from a WebhookListener. Ensure tests/settings disable outbound webhooks in test runs:
- Gate webhook dispatch behind an env flag (e.g., WEBHOOK_ENABLED=false in .env.test)
- Or mock axios globally in Jest setup to prevent real network calls
This will stabilize CI while you ramp up coverage.
apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.spec.ts (8)
32-36: DTO naming consistency: “Rate” vs “Ratio”Method is getIssuedRatio while DTO is GetIssuedRateDto. Consider aligning names for discoverability (e.g., GetIssuedRatioDto), or add an alias export.
297-308: Strengthen assertions for issue query conditions (optional)Consider asserting that the query builder applies time range and project filters to issues. Example:
jest .spyOn(issueRepo, 'createQueryBuilder') .mockImplementation( () => createQueryBuilder as unknown as SelectQueryBuilder<IssueEntity>, ); + const whereSpy = jest.spyOn(createQueryBuilder, 'where' as never); + const andWhereSpy = jest.spyOn(createQueryBuilder, 'andWhere' as never); jest .spyOn(createQueryBuilder, 'getRawMany' as never) .mockResolvedValue(feedbackStatsFixture as never); jest .spyOn(feedbackRepo, 'count') .mockResolvedValue(feedbackStatsFixture.length); const result = await feedbackStatsService.getIssuedRatio(dto); expect(result).toEqual({ ratio: 1, }); expect(issueRepo.createQueryBuilder).toHaveBeenCalledWith('issue'); expect(feedbackRepo.count).toHaveBeenCalledWith({ where: { createdAt: Between(dto.from, dto.to), channel: { project: { id: dto.projectId } }, }, }); + expect(whereSpy).toHaveBeenCalled(); + expect(andWhereSpy).toHaveBeenCalled();
323-328: Add assertions for lock acquisition/release in cron testValidate locking behavior to prevent duplicate/parallel cron jobs.
Apply this diff:
jest.spyOn(schedulerRegistry, 'addCronJob'); jest.spyOn(schedulerRegistry, 'getCronJobs').mockReturnValue(new Map()); jest.spyOn(schedulerLockService, 'acquireLock').mockResolvedValue(true); jest .spyOn(schedulerLockService, 'releaseLock') .mockResolvedValue(undefined); await feedbackStatsService.addCronJobByProjectId(projectId); expect(schedulerRegistry.addCronJob).toHaveBeenCalledTimes(1); expect(schedulerRegistry.addCronJob).toHaveBeenCalledWith( `feedback-statistics-${projectId}`, expect.anything(), ); + expect(schedulerLockService.acquireLock).toHaveBeenCalled(); + expect(schedulerLockService.releaseLock).toHaveBeenCalled();
422-436: Make save() expectation less brittleService may pass additional fields (id, date, channel). Match only the updated count to avoid fragile tests.
Apply this diff:
- expect(feedbackStatsRepo.save).toHaveBeenCalledWith({ - count: 1 + count, - }); + expect(feedbackStatsRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ count: 1 + count }), + );
455-461: Optional: Also assert insert() is invoked for non-existent dateGiven you’re exercising an upsert path, adding insert() and orUpdate() call assertions would strengthen behavior guarantees.
Example:
- jest.spyOn(createQueryBuilder, 'values' as never).mockReturnThis(); + const insertSpy = jest.spyOn(createQueryBuilder, 'insert' as never); + jest.spyOn(createQueryBuilder, 'values' as never).mockReturnThis(); jest.spyOn(createQueryBuilder, 'orUpdate' as never).mockReturnThis(); jest.spyOn(createQueryBuilder, 'updateEntity' as never).mockReturnThis(); jest .spyOn(createQueryBuilder, 'execute' as never) .mockResolvedValue({} as never); + expect(insertSpy).toHaveBeenCalled(); + expect(createQueryBuilder.orUpdate).toHaveBeenCalled();
79-107: Stabilize faker for deterministic tests (optional)Seed faker to reduce flakiness across runs/environments.
Apply this diff near the top-level describe:
describe('FeedbackStatisticsService suite', () => { + beforeAll(() => faker.seed(42));
89-107: Hygiene: Restore mocks after each test (optional)Prevents cross-test leakage if a test forgets to fully stub/clear.
Apply this diff:
beforeEach(async () => { const module = await Test.createTestingModule({ imports: [TestConfig], providers: FeedbackStatisticsServiceProviders, }).compile(); @@ schedulerLockService = module.get(SchedulerLockService); }); + + afterEach(() => { + jest.restoreAllMocks(); + });
165-207: Verify “week” bucketing semanticsThe expected windows (2023-01-01–01-03, 01-04–01-10, etc.) don’t match ISO/Sun-start weeks. If this is intentional (e.g., Tue‑ending windows), add a brief comment to document the rule and align service logic accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/api/src/common/filters/http-exception.filter.spec.ts(1 hunks)apps/api/src/domains/admin/auth/auth.controller.spec.ts(3 hunks)apps/api/src/domains/admin/auth/auth.service.spec.ts(7 hunks)apps/api/src/domains/admin/channel/channel/channel.service.spec.ts(8 hunks)apps/api/src/domains/admin/channel/field/field.service.spec.ts(8 hunks)apps/api/src/domains/admin/channel/option/option.service.spec.ts(1 hunks)apps/api/src/domains/admin/project/issue/issue.service.spec.ts(4 hunks)apps/api/src/domains/admin/project/role/role.service.spec.ts(6 hunks)apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts(6 hunks)apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts(8 hunks)apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.spec.ts(14 hunks)apps/api/src/domains/admin/tenant/tenant.controller.spec.ts(1 hunks)apps/api/src/shared/code/code.service.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/domains/admin/tenant/tenant.controller.spec.ts
🧰 Additional context used
🧬 Code graph analysis (10)
apps/api/src/domains/admin/auth/auth.service.spec.ts (4)
apps/web/src/entities/user/user.type.ts (1)
UserTypeEnum(30-30)apps/api/src/domains/admin/user/dtos/user.dto.ts (1)
UserDto(21-51)apps/api/src/test-utils/fixtures.ts (1)
emailFixture(182-182)apps/api/src/test-utils/providers/auth.service.providers.ts (1)
MockJwtService(32-34)
apps/api/src/shared/code/code.service.spec.ts (2)
apps/api/src/test-utils/fixtures.ts (1)
codeFixture(275-284)apps/api/src/domains/admin/user/dtos/user.dto.ts (1)
UserDto(21-51)
apps/api/src/domains/admin/project/role/role.service.spec.ts (1)
apps/api/src/test-utils/fixtures.ts (1)
getRandomEnumValues(165-170)
apps/api/src/domains/admin/channel/option/option.service.spec.ts (1)
apps/api/src/domains/admin/channel/option/dtos/replace-select-options.dto.ts (1)
ReplaceManyOptionsDto(16-19)
apps/api/src/domains/admin/channel/channel/channel.service.spec.ts (2)
apps/api/src/domains/admin/channel/channel/dtos/create-channel.dto.ts (1)
CreateChannelDto(22-57)apps/api/src/test-utils/fixtures.ts (2)
channelFixture(299-309)createFieldDto(76-93)
apps/api/src/domains/admin/project/issue/issue.service.spec.ts (5)
apps/api/src/test-utils/util-functions.ts (2)
TestConfig(64-70)createQueryBuilder(212-233)apps/api/src/test-utils/providers/issue.service.providers.ts (1)
IssueServiceProviders(25-37)apps/api/src/test-utils/fixtures.ts (1)
issueFixture(322-330)apps/api/src/domains/admin/project/issue/dtos/find-issues-by-project-id-v2.dto.ts (1)
FindIssuesByProjectIdDtoV2(23-37)apps/api/src/domains/admin/project/issue/dtos/update-issue.dto.ts (1)
UpdateIssueDto(21-33)
apps/api/src/domains/admin/channel/field/field.service.spec.ts (1)
apps/api/src/test-utils/fixtures.ts (2)
createFieldDto(76-93)updateFieldDto(94-99)
apps/api/src/domains/admin/auth/auth.controller.spec.ts (2)
apps/api/src/domains/admin/user/dtos/user.dto.ts (1)
UserDto(21-51)apps/api/src/domains/admin/auth/dtos/requests/oauth-user-sign-up-request.dto.ts (1)
OAuthUserSignUpRequestDto(19-33)
apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts (2)
apps/api/src/test-utils/util-functions.ts (2)
TestConfig(64-70)getRandomEnumValue(76-81)apps/api/src/test-utils/fixtures.ts (2)
webhookFixture(369-379)getRandomEnumValue(159-164)
apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.spec.ts (2)
apps/api/src/test-utils/stubs/common-repository.stub.ts (1)
count(78-80)apps/api/src/test-utils/util-functions.ts (1)
createQueryBuilder(212-233)
🪛 GitHub Actions: CI
apps/api/src/domains/admin/auth/auth.service.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/shared/code/code.service.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/project/role/role.service.spec.ts
[error] 174-176: RoleService.createMany: Expected result to have length roleCount, but received an object. Ensure createMany returns an array of created roles.
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts
[error] 687-687: FeedbackIssueStatisticsService: Failed to create feedback issue statistics. Transaction failed.
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/channel/option/option.service.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/channel/channel/channel.service.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/project/issue/issue.service.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/channel/field/field.service.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/auth/auth.controller.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/common/filters/http-exception.filter.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.spec.ts
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: Connection refused (status 500)
[error] 1-1: WebhookListener: Failed to send webhook. axiosError: timeout of 5000ms exceeded
🪛 GitHub Check: lint
apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts
[failure] 744-744:
Unsafe call of a(n) any typed value
⏰ 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 (39)
apps/api/src/common/filters/http-exception.filter.spec.ts (3)
281-284: Assert wrapped boolean payloadAs with other primitive responses, the filter emits
{ response: true, statusCode, path }. The current expectation omits theresponsekey, so it won’t match the filter output.expect(mockResponse.send).toHaveBeenCalledWith({ - statusCode: HttpStatus.OK, + response: true, + statusCode: HttpStatus.OK, path: '/test-endpoint', });
293-296: Cover numeric payload underresponseNumeric responses are also wrapped under
responseby the filter. Add the missing field to keep the assertion truthful to the actual payload structure.expect(mockResponse.send).toHaveBeenCalledWith({ - statusCode: HttpStatus.OK, + response: 42, + statusCode: HttpStatus.OK, path: '/test-endpoint', });
264-273: Reinstateresponsewrapper instead of spreading array elements
HttpExceptionFilterwraps array payloads under aresponsekey rather than spreading them into numeric properties. The existing expectation contradicts that structure, so please update it to match the emitted object (array preserved insideresponse).expect(mockResponse.send).toHaveBeenCalledWith({ - 0: 'Error 1', - 1: 'Error 2', - 2: 'Error 3', - statusCode: HttpStatus.BAD_REQUEST, + response: ['Error 1', 'Error 2', 'Error 3'], + statusCode: HttpStatus.BAD_REQUEST, path: '/test-endpoint', });apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts (8)
479-488: Lock acquisition failure should short‑circuit cron registration.This remains the same mismatch flagged earlier:
addCronJobByProjectIdexits before touchingschedulerRegistry.addCronJobwhenschedulerLockService.acquireLockreturnsfalse. The current expectation forces the test red. Please assert thataddCronJobis not called (and that we do not release a lock we never acquired).Apply this diff to align the spec with the service contract:
- await feedbackIssueStatsService.addCronJobByProjectId(projectId); - - expect(schedulerRegistry.addCronJob).toHaveBeenCalledTimes(1); + await feedbackIssueStatsService.addCronJobByProjectId(projectId); + + expect(schedulerRegistry.addCronJob).not.toHaveBeenCalled(); + expect(schedulerLockService.releaseLock).not.toHaveBeenCalled();
546-555:acquireLockrejection must bubble and block cron creation.When the lock service throws, the implementation rejects without registering the cron job. The spec should mirror that behavior (see the prior review note). Update the test to expect a rejection and to assert that
addCronJobstays untouched.-await expect( - feedbackIssueStatsService.addCronJobByProjectId(projectId), -).resolves.not.toThrow(); -expect(schedulerRegistry.addCronJob).toHaveBeenCalledTimes(1); +await expect( + feedbackIssueStatsService.addCronJobByProjectId(projectId), +).rejects.toThrow('Lock service error'); +expect(schedulerRegistry.addCronJob).not.toHaveBeenCalled();
671-696: Transaction failures are propagated, not swallowed.
createFeedbackIssueStatisticsrethrows when the transactional callback rejects (the CI log shows “Transaction failed”). The spec should therefore assertrejects.toThrow('Transaction failed'), notresolves. Please adjust this test (and any similar ones) accordingly.-await expect( - feedbackIssueStatsService.createFeedbackIssueStatistics( - projectId, - dayToCreate, - ), -).resolves.not.toThrow(); +await expect( + feedbackIssueStatsService.createFeedbackIssueStatistics( + projectId, + dayToCreate, + ), +).rejects.toThrow('Transaction failed');
698-725: Zero feedback should skip the transaction entirely.The service exits before invoking
manager.transactionwhenfeedbackCount === 0; expecting one call contradicts the implementation (see earlier review feedback). Please assert the transaction spy is not called here, and mirror that adjustment in the “large dayToCreate” scenario that also feeds a zero count.-// Transaction is called but no database operations are performed when feedback count is 0 -expect(transactionSpy).toHaveBeenCalledTimes(1); +expect(transactionSpy).not.toHaveBeenCalled();
757-784: LargedayToCreatewith zero feedback still skips transactions.Same rationale as the previous test: with a zero feedback count, the method returns early. Update the expectation to assert no transactions are run instead of
dayToCreate * issues.length.-// Transaction is called for each day*issue combination, but no database operations when feedback count is 0 -expect(transactionSpy).toHaveBeenCalledTimes(dayToCreate * issues.length); +expect(transactionSpy).not.toHaveBeenCalled();
809-840: Timeout rejections must surface to the caller.The service does not swallow timeouts—it propagates the rejection. Please flip this assertion to
rejects.toThrow('Transaction timeout')(same change applies to any other timeout expectations).-// Should not throw error, but log it -await expect( - feedbackIssueStatsService.createFeedbackIssueStatistics( - projectId, - dayToCreate, - ), -).resolves.not.toThrow(); +await expect( + feedbackIssueStatsService.createFeedbackIssueStatistics( + projectId, + dayToCreate, + ), +).rejects.toThrow('Transaction timeout');
959-982:saveexpectations must include the entity identifier.
updateFeedbackCountmutates the fetched entity and passes it (with itsid) back tosave. Expectingsaveto receive only{ feedbackCount: 2 }will fail once the real call includes the primary key. Please assert withexpect.objectContaining({ id: existing.id, … })(and apply the same fix to the negative/large count variants below).-expect(feedbackIssueStatsRepo.save).toHaveBeenCalledWith({ - feedbackCount: 2, // 1 (existing) + 1 (default) -}); +expect(feedbackIssueStatsRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + id: expect.any(Number), + feedbackCount: 2, + }), +);
1008-1058: Ensuresaveassertions include the mutated entity, not a bare object.Both the negative and very large feedback-count scenarios expect
saveto receive{ feedbackCount: … }, but the service forwards the full entity (including its id). Mirror the adjustment suggested above: useexpect.objectContaining({ id: existing.id, feedbackCount: … })so the expectation matches the actual call.-expect(feedbackIssueStatsRepo.save).toHaveBeenCalledWith({ - feedbackCount: 5, // 10 + (-5) -}); +expect(feedbackIssueStatsRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + id: expect.any(Number), + feedbackCount: 5, + }), +); @@ -expect(feedbackIssueStatsRepo.save).toHaveBeenCalledWith({ - feedbackCount: 1000000, // 1 + 999999 -}); +expect(feedbackIssueStatsRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + id: expect.any(Number), + feedbackCount: 1_000_000, + }), +);apps/api/src/domains/admin/auth/auth.service.spec.ts (1)
206-234: Stop reaching into private collaborators withas anyCasting
AuthServicetoanyto spy oncodeService/createUserServicekeeps the tests brittle, requires the eslint disable above, and resurrects the same private-access issue we already flagged. Please hold on to the compiledTestingModule(e.g.,let moduleRef: TestingModule;) and retrieve those collaborators viamoduleRef.get(CodeService)/moduleRef.get(CreateUserService)instead. That keeps everything typed, avoids the lint suppression, and won’t break if the service internals change.apps/api/src/domains/admin/auth/auth.controller.spec.ts (1)
250-266: Await the controller’s async sign-in flow in this test
AuthService.signInis async, soAuthController.signInEmailreturns a Promise. By usingmockReturnValueand skippingawait, we’re masking that contract and the assertion is actually comparing against a pending Promise. Please mirror the real behavior by awaiting the controller call and usingmockResolvedValueon the mock.- it('should sign in email user successfully', () => { + it('should sign in email user successfully', async () => { const user = new UserDto(); user.id = faker.number.int(); user.email = faker.internet.email(); user.name = faker.person.fullName(); const mockTokens = { accessToken: faker.string.alphanumeric(32), refreshToken: faker.string.alphanumeric(32), }; - authService.signIn.mockReturnValue(mockTokens as any); - - const result = authController.signInEmail(user); + authService.signIn.mockResolvedValue(mockTokens as any); + + const result = await authController.signInEmail(user); expect(authService.signIn).toHaveBeenCalledWith(user); expect(authService.signIn).toHaveBeenCalledTimes(1); expect(result).toEqual(mockTokens); });apps/api/src/domains/admin/project/role/role.service.spec.ts (13)
53-61: LGTM: happy-path create assertions are solidGood shape/field assertions and interaction checks.
Also applies to: 68-68
79-79: LGTM: duplicate create guards verifiedRight expectation on no save when duplicate is detected.
Also applies to: 90-91
239-249: LGTM: update flow covers existence check, uniqueness, and save interactionGood coverage and call argument verification.
Also applies to: 255-257
292-315: LGTM: upsert-like path validatedClear expectations when id not found; checks payload to save.
317-350: LGTM: permission de-duplication asserted via save payloadAsserting unique permissions in saved entity is appropriate.
395-434: LGTM: findByUserId behavior and repository filter verifiedCovers both populated and empty results with correct where/relations.
436-467: LGTM: findByProjectId roles + total contract checkedValidates repository call and response shape for both non-empty and empty cases.
469-497: LGTM: findAndCount nominal and empty paths testedSimple and effective expectations.
499-521: LGTM: delete flow assertions focus on repository interactionCovers both normal and non-existent role scenarios.
523-571: LGTM: validateRoleName path covered via create()Good verification of query args and exception path.
655-674: LGTM: empty permissions edge case coveredAsserts persisted permissions are empty.
107-107: Don’t call jest.spyOn on the same method twice in one testReusing the same spy avoids “Cannot spy on the same method more than once”.
- jest.spyOn(roleRepo, 'findOneBy').mockResolvedValueOnce(null); + const findOneBySpy = jest.spyOn(roleRepo, 'findOneBy'); + findOneBySpy.mockResolvedValueOnce(null); @@ - jest.spyOn(roleRepo, 'findOneBy').mockResolvedValueOnce({ + findOneBySpy.mockResolvedValueOnce({ id: faker.number.int(), } as unknown as RoleEntity);Also applies to: 119-121
590-599: Avoid double spying in integration scenario testReuse spies for findOneBy/save instead of re-registering them later in the same test.
- jest.spyOn(roleRepo, 'findOneBy').mockResolvedValue(null); - jest.spyOn(roleRepo, 'save').mockResolvedValue({ + const findOneBySpy = jest.spyOn(roleRepo, 'findOneBy'); + const saveSpy = jest.spyOn(roleRepo, 'save'); + findOneBySpy.mockResolvedValue(null); + saveSpy.mockResolvedValue({ id: faker.number.int(), project: { id: projectId }, members: [], createdAt: new Date(), updatedAt: new Date(), deletedAt: null, } as unknown as RoleEntity); @@ - jest.spyOn(roleRepo, 'findOneBy').mockResolvedValue(mockRoles[0]); + findOneBySpy.mockResolvedValue(mockRoles[0]); jest.spyOn(roleRepo, 'findOne').mockResolvedValue(null); - jest.spyOn(roleRepo, 'save').mockResolvedValue({ + saveSpy.mockResolvedValue({ ...mockRoles[0], ...updateDto, } as unknown as RoleEntity);Also applies to: 629-635
apps/api/src/domains/admin/channel/option/option.service.spec.ts (1)
200-231: Nice coverage for duplicate validation.These duplicate-name/key scenarios round out the replaceMany contract and keep the exception paths locked in. 👌
apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.spec.ts (12)
21-21: Good: Using TypeORM operators directly in testsImporting Between and In improves clarity of expected query shapes.
27-27: Good: Explicitly covering project-not-found pathsImporting ProjectNotFoundException to assert error paths is appropriate.
29-29: Good: Locking service included for cron job testsSchedulerLockService inclusion enables testing concurrency/locking behavior.
87-87: Good: Lock service referenceKeeps tests aligned with behavior guarded by locks.
106-107: Good: Resolving lock service from moduleEnsures real provider wiring is exercised in tests.
261-271: LGTM: Count query shapeBetween on createdAt and nested project filter look correct.
280-281: DTO naming consistency for issued metricRe-flagging: DTO uses “Rate” while method/result use “Ratio”. Align or document to avoid confusion.
338-345: Good: Negative path for missing projectCovers error handling for addCronJobByProjectId.
368-385: LGTM: Transaction mocking for bulk insert/upsertNice isolated stub of manager.createQueryBuilder with insert/upsert chain.
397-405: Good: Negative path for missing projectCovers error handling for createFeedbackStatistics.
481-494: Good: Negative path for missing project on updateCountEnsures early failure when project is absent.
496-512: Good: Zero-count no-op guardPrevents unnecessary DB calls.
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.BAD_REQUEST); | ||
| expect(mockResponse.send).toHaveBeenCalledWith({ | ||
| response: 'Test error message', | ||
| path: '/test-endpoint', | ||
| }); |
There was a problem hiding this comment.
Restore expected statusCode in string-response assertions
HttpExceptionFilter always appends the derived statusCode alongside response and path. The underlying filter (see Line 57 of http-exception.filter.ts) pushes statusCode onto the payload for primitive responses, so the test will fail without asserting it. Please update the expectation accordingly.
expect(mockResponse.send).toHaveBeenCalledWith({
response: 'Test error message',
- path: '/test-endpoint',
+ statusCode: HttpStatus.BAD_REQUEST,
+ path: '/test-endpoint',
});📝 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.
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.BAD_REQUEST); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: 'Test error message', | |
| path: '/test-endpoint', | |
| }); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.BAD_REQUEST); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: 'Test error message', | |
| statusCode: HttpStatus.BAD_REQUEST, | |
| path: '/test-endpoint', | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/common/filters/http-exception.filter.spec.ts around lines 74 to
78, the test assertion for a primitive (string) response omits the derived
statusCode that HttpExceptionFilter adds to the payload; update the expected
object passed to mockResponse.send to include statusCode: HttpStatus.BAD_REQUEST
(so the expectation should assert response, path, and statusCode fields) and
ensure the value uses the HttpStatus enum constant.
| statusCodes.forEach((statusCode) => { | ||
| const exception = new HttpException('Test error', statusCode); | ||
|
|
||
| filter.catch(exception, mockArgumentsHost); | ||
|
|
||
| expect(mockResponse.status).toHaveBeenCalledWith(statusCode); | ||
| expect(mockResponse.send).toHaveBeenCalledWith({ | ||
| response: 'Test error', | ||
| path: '/test-endpoint', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Account for statusCode field in looped assertions
Each call to filter.catch adds the current statusCode to the response payload. The looped assertion only checks for response and path, which misses the injected field and causes a mismatch with the filter’s actual behavior.
expect(mockResponse.send).toHaveBeenCalledWith({
response: 'Test error',
+ statusCode,
path: '/test-endpoint',
});📝 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.
| statusCodes.forEach((statusCode) => { | |
| const exception = new HttpException('Test error', statusCode); | |
| filter.catch(exception, mockArgumentsHost); | |
| expect(mockResponse.status).toHaveBeenCalledWith(statusCode); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: 'Test error', | |
| path: '/test-endpoint', | |
| }); | |
| }); | |
| statusCodes.forEach((statusCode) => { | |
| const exception = new HttpException('Test error', statusCode); | |
| filter.catch(exception, mockArgumentsHost); | |
| expect(mockResponse.status).toHaveBeenCalledWith(statusCode); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: 'Test error', | |
| statusCode, | |
| path: '/test-endpoint', | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/common/filters/http-exception.filter.spec.ts around lines 111 to
121, the test loop currently asserts only the response and path but the filter
also injects the statusCode into the payload; update the assertion to include
the current statusCode (use the statusCode variable from the loop) when
verifying mockResponse.send was called, or build the expected object dynamically
per iteration to include response, path and statusCode; ensure the assertion
uses the same statusCode value for each iteration and reset/clear mocks between
iterations if necessary.
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | ||
| expect(mockResponse.send).toHaveBeenCalledWith({ | ||
| response: '', | ||
| path: '/test-endpoint', | ||
| }); |
There was a problem hiding this comment.
Include statusCode for empty string payloads
Even when the payload is an empty string, the filter still adds the numeric statusCode before sending the response. The current expectation omits it, so the assertion does not reflect reality.
expect(mockResponse.send).toHaveBeenCalledWith({
response: '',
+ statusCode: HttpStatus.NO_CONTENT,
path: '/test-endpoint',
});📝 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.
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: '', | |
| path: '/test-endpoint', | |
| }); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: '', | |
| statusCode: HttpStatus.NO_CONTENT, | |
| path: '/test-endpoint', | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/common/filters/http-exception.filter.spec.ts around lines 161 to
165, the test expects the response body for an empty string payload but omits
the numeric statusCode that the filter adds; update the assertion to include
statusCode (use HttpStatus.NO_CONTENT or its numeric value) alongside response:
'' and path: '/test-endpoint' so the mockResponse.send expectation matches the
actual payload sent by the filter.
| filter.catch(exception, mockArgumentsHost); | ||
|
|
||
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | ||
| expect(mockResponse.send).toHaveBeenCalledWith({ | ||
| statusCode: HttpStatus.NO_CONTENT, | ||
| path: '/test-endpoint', | ||
| }); |
There was a problem hiding this comment.
Missing response field for null payload
When the exception payload is null, the filter normalizes it under the response property (set to null) while also adding statusCode and path. Bringing the test in line with that behavior will prevent false negatives.
expect(mockResponse.send).toHaveBeenCalledWith({
- statusCode: HttpStatus.NO_CONTENT,
+ response: null,
+ statusCode: HttpStatus.NO_CONTENT,
path: '/test-endpoint',
});📝 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.
| filter.catch(exception, mockArgumentsHost); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| statusCode: HttpStatus.NO_CONTENT, | |
| path: '/test-endpoint', | |
| }); | |
| filter.catch(exception, mockArgumentsHost); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: null, | |
| statusCode: HttpStatus.NO_CONTENT, | |
| path: '/test-endpoint', | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/common/filters/http-exception.filter.spec.ts around lines 171 to
177, the test expects the filter to call response.send with an object missing
the normalized response field for a null payload; update the expected call to
include response: null along with statusCode: HttpStatus.NO_CONTENT and path:
'/test-endpoint' so the assertion matches the filter's normalization behavior.
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | ||
| expect(mockResponse.send).toHaveBeenCalledWith({ | ||
| statusCode: HttpStatus.NO_CONTENT, | ||
| path: '/test-endpoint', | ||
| }); |
There was a problem hiding this comment.
Align undefined-payload assertion with filter output
The filter treats undefined like other primitives, wrapping it under response in the dispatched body. The expectation should assert that response is undefined rather than omitting it altogether.
expect(mockResponse.send).toHaveBeenCalledWith({
- statusCode: HttpStatus.NO_CONTENT,
+ response: undefined,
+ statusCode: HttpStatus.NO_CONTENT,
path: '/test-endpoint',
});📝 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.
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| statusCode: HttpStatus.NO_CONTENT, | |
| path: '/test-endpoint', | |
| }); | |
| expect(mockResponse.status).toHaveBeenCalledWith(HttpStatus.NO_CONTENT); | |
| expect(mockResponse.send).toHaveBeenCalledWith({ | |
| response: undefined, | |
| statusCode: HttpStatus.NO_CONTENT, | |
| path: '/test-endpoint', | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/common/filters/http-exception.filter.spec.ts around lines 188 to
192, the test expects the response body to omit the response field for an
undefined payload but the filter wraps undefined under the "response" key;
update the assertion to expect an object containing statusCode:
HttpStatus.NO_CONTENT, path: '/test-endpoint', and response: undefined so the
test aligns with the filter's actual output.
| it('handles network timeout errors', async () => { | ||
| const timeoutError: AxiosError = { | ||
| name: 'AxiosError', | ||
| message: 'timeout of 5000ms exceeded', | ||
| code: 'ECONNABORTED', | ||
| response: undefined, | ||
| config: {} as any, | ||
| isAxiosError: true, | ||
| toJSON: () => ({}), | ||
| }; | ||
|
|
||
| jest | ||
| .spyOn(httpService, 'post') | ||
| .mockImplementation(() => throwError(() => timeoutError)); | ||
|
|
||
| // Should handle timeout gracefully | ||
| await expect( | ||
| webhookListener.handleFeedbackCreation({ | ||
| feedbackId: faker.number.int(), | ||
| }), | ||
| ).resolves.not.toThrow(); | ||
| }); | ||
|
|
||
| it('handles malformed response errors', async () => { | ||
| const malformedError: AxiosError = { | ||
| name: 'AxiosError', | ||
| message: 'Request failed with status code 400', | ||
| code: 'ERR_BAD_REQUEST', | ||
| response: { | ||
| data: 'Invalid JSON response', | ||
| status: 400, | ||
| statusText: 'Bad Request', | ||
| headers: {}, | ||
| config: {} as any, | ||
| }, | ||
| config: {} as any, | ||
| isAxiosError: true, | ||
| toJSON: () => ({}), | ||
| }; | ||
|
|
||
| jest | ||
| .spyOn(httpService, 'post') | ||
| .mockImplementation(() => throwError(() => malformedError)); | ||
|
|
||
| // Should handle malformed response gracefully | ||
| await expect( | ||
| webhookListener.handleFeedbackCreation({ | ||
| feedbackId: faker.number.int(), | ||
| }), | ||
| ).resolves.not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Timeout/malformed error tests never hit the implementation
The spies on httpService.post are installed before calling handleFeedbackCreation, so the mock throws immediately and the listener’s logic (including its retry/error-handling paths) is bypassed. Like the earlier HTTP-error test, restructure the spy so it actually intercepts the call during execution, otherwise these tests don’t exercise the code they’re intended to protect.
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts around
lines 437-487, the tests install spies that throw immediately so the listener’s
runtime logic is bypassed; change the spies to be applied for the actual call by
using mockImplementationOnce (or otherwise set the mock immediately before
invoking handleFeedbackCreation) so the throwError Observable is returned when
httpService.post is invoked during execution, not at spy setup; ensure the mock
returns an Observable (throwError(() => error)) and restore/clear the spy
between tests if needed.
| jest.spyOn(httpService, 'post').mockImplementation(() => { | ||
| callCount++; | ||
| // Always return error to test retry behavior | ||
| return throwError(() => axiosError); | ||
| }); | ||
|
|
||
| await webhookListener.handleFeedbackCreation({ | ||
| feedbackId: faker.number.int(), | ||
| }); | ||
|
|
||
| // Should make at least one call (retry behavior may vary in test environment) | ||
| expect(callCount).toBeGreaterThan(0); | ||
| }); |
There was a problem hiding this comment.
Retry test must assert configured retry count
The listener’s contract is to retry up to its configured maximum, but this test only asserts callCount > 0, so it happily passes even if retries stop working and only the initial call runs. Import the retry constant (or hardcode with a TODO) and assert the exact number of attempts. Without that, the test provides no coverage for the retry loop.
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts around
lines 503–515, the test currently only asserts callCount > 0 which doesn't
verify retry behavior; import the listener's configured retry constant (or
hardcode a value with a TODO) and change the assertion to expect the exact
number of attempts (e.g., expect(callCount).toBe(CONFIGURED_RETRY_ATTEMPTS)) so
the test fails if the retry loop stops working; ensure you reference the same
constant used by the listener (or add a TODO comment if hardcoding) and update
any test setup timeouts if needed to accommodate all retry attempts.
| jest | ||
| .spyOn(httpService, 'post') | ||
| .mockImplementation(() => throwError(() => axiosError)); | ||
|
|
||
| await webhookListener.handleFeedbackCreation({ | ||
| feedbackId: faker.number.int(), | ||
| }); | ||
|
|
||
| expect(loggerSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining('Retrying webhook... Attempt #'), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Warn log assertion is too loose
The listener prefixes retry attempts with the attempt number (e.g., Attempt #1), but this expectation matches any warning containing “Retrying webhook... Attempt #”. A future change could log something unrelated yet still satisfy the test. Assert the full log message—including the attempt number—to ensure the retry loop’s logging format stays intact.
🤖 Prompt for AI Agents
In apps/api/src/domains/admin/project/webhook/webhook.listener.spec.ts around
lines 545 to 556, the test currently uses a loose
expect.stringContaining('Retrying webhook... Attempt #') which can be satisfied
by unrelated messages; update the assertion to verify the full log message
including the attempt number (e.g., replace the stringContaining check with an
exact match or a precise regex that includes "Retrying webhook... Attempt #1" or
"Attempt #<digit>" as emitted by the code) so the test asserts the complete
formatted retry log.
| const result = await feedbackStatsService.getCountByDateByChannel(dto); | ||
|
|
||
| expect(countByDateByChannel).toEqual({ | ||
| expect(feedbackStatsRepo.find).toHaveBeenCalledWith({ | ||
| where: { | ||
| channel: In(channelIds), | ||
| date: Between(new Date(startDate), new Date(endDate)), | ||
| }, | ||
| relations: { channel: true }, | ||
| order: { channel: { id: 'ASC' }, date: 'ASC' }, | ||
| }); | ||
| expect(result).toEqual({ |
There was a problem hiding this comment.
🧩 Analysis chain
Fix: Incorrect find() where shape for relation filter
TypeORM find options should nest the relation property when using In. Using channel: In(channelIds) is not valid for a relation; prefer channel: { id: In(channelIds) }.
Apply this diff:
- expect(feedbackStatsRepo.find).toHaveBeenCalledWith({
- where: {
- channel: In(channelIds),
- date: Between(new Date(startDate), new Date(endDate)),
- },
- relations: { channel: true },
- order: { channel: { id: 'ASC' }, date: 'ASC' },
- });
+ expect(feedbackStatsRepo.find).toHaveBeenCalledWith({
+ where: {
+ channel: { id: In(channelIds) },
+ date: Between(new Date(startDate), new Date(endDate)),
+ },
+ relations: { channel: true },
+ order: { channel: { id: 'ASC' }, date: 'ASC' },
+ });If production code currently uses channel: In(channelIds), it may silently return wrong results. Please double‑check the service implementation.
#!/bin/bash
set -e
# Locate the service file
SERVICE_FILE=$(rg -l "getCountByDateByChannel" -g "*.ts" apps/api/src)
echo "Service file: $SERVICE_FILE"
# Show implementation of getCountByDateByChannel
rg -n "getCountByDateByChannel" -C5 "$SERVICE_FILE"
# Show the find call inside that method
rg -n "feedbackStatsRepo\.find" -C5 "$SERVICE_FILE"🏁 Script executed:
#!/bin/bash
set -e
# Locate the FeedbackStatisticsService file
SERVICE_FILE=$(rg -l "class .*FeedbackStatisticsService" -g "*.ts" apps/api/src)
echo "Service file: $SERVICE_FILE"
# Show the getCountByDateByChannel method implementation
rg -n -C3 "getCountByDateByChannel" "$SERVICE_FILE"
# Show the feedbackStatsRepo.find call within that service
rg -n -C3 "feedbackStatsRepo\.find" "$SERVICE_FILE"Length of output: 375
#!/bin/bash
set -e
FILE="apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.ts"
echo "===== repository.find call ====="
rg -n -C5 "repository\.find" "$FILE"
echo
echo "===== where clause context ====="
rg -n -C5 "where:" "$FILE"
echo
echo "===== relations & order context ====="
rg -n -C5 "relations:" "$FILE"🏁 Script executed:
#!/bin/bash
set -e
FILE="apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.ts"
sed -n '60,90p' "$FILE"Length of output: 925
Fix nested relation filter in service and spec
Update the where clause to nest channel.id under channel in both files:
In apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.ts:
// before
where: {
channel: In(channelIds),
date: Between(new Date(startDate), new Date(endDate)),
},
// after
where: {
channel: { id: In(channelIds) },
date: Between(new Date(startDate), new Date(endDate)),
},In apps/api/src/domains/admin/statistics/feedback/feedback-statistics.service.spec.ts:
- where: {
- channel: In(channelIds),
+ where: {
+ channel: { id: In(channelIds) },
date: Between(new Date(startDate), new Date(endDate)),
},| it('set code with custom duration', async () => { | ||
| const dto = new SetCodeEmailVerificationDto(); | ||
| dto.key = faker.string.sample(); | ||
| dto.type = CodeTypeEnum.EMAIL_VEIRIFICATION; | ||
| dto.durationSec = 300; // 5 minutes | ||
| jest.spyOn(codeRepo, 'save'); | ||
|
|
||
| const code = await codeService.setCode(dto); | ||
|
|
||
| expect(code).toHaveLength(6); | ||
| expect(codeRepo.save).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| code, | ||
| type: dto.type, | ||
| key: dto.key, | ||
| isVerified: false, | ||
| expiredAt: expect.any(Date), | ||
| }), | ||
| ); | ||
| }); | ||
| it('set code with default duration when durationSec is not provided', async () => { | ||
| const dto = new SetCodeEmailVerificationDto(); | ||
| dto.key = faker.string.sample(); | ||
| dto.type = CodeTypeEnum.EMAIL_VEIRIFICATION; | ||
| jest.spyOn(codeRepo, 'save'); | ||
|
|
||
| const code = await codeService.setCode(dto); | ||
|
|
||
| expect(code).toHaveLength(6); | ||
| expect(codeRepo.save).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| code, | ||
| type: dto.type, | ||
| key: dto.key, | ||
| isVerified: false, | ||
| expiredAt: expect.any(Date), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Validate custom/default expiry timestamps explicitly
Right now the “custom duration” spec sets durationSec = 300, which matches the default TTL, so it can’t detect whether the override path actually runs. Both new specs also only assert that expiredAt is some Date, so they would keep passing even if the service stopped touching the expiry timestamp entirely. Please freeze time, pick a distinct override (e.g., 120 s), and assert the saved entity’s expiredAt matches the expected timestamp; do the same for the default path so we exercise the real fallback. Here’s an example adjustment:
@@
- it('set code with custom duration', async () => {
- const dto = new SetCodeEmailVerificationDto();
- dto.key = faker.string.sample();
- dto.type = CodeTypeEnum.EMAIL_VEIRIFICATION;
- dto.durationSec = 300; // 5 minutes
- jest.spyOn(codeRepo, 'save');
-
- const code = await codeService.setCode(dto);
-
- expect(code).toHaveLength(6);
- expect(codeRepo.save).toHaveBeenCalledWith(
- expect.objectContaining({
- code,
- type: dto.type,
- key: dto.key,
- isVerified: false,
- expiredAt: expect.any(Date),
- }),
- );
- });
- it('set code with default duration when durationSec is not provided', async () => {
- const dto = new SetCodeEmailVerificationDto();
- dto.key = faker.string.sample();
- dto.type = CodeTypeEnum.EMAIL_VEIRIFICATION;
- jest.spyOn(codeRepo, 'save');
-
- const code = await codeService.setCode(dto);
-
- expect(code).toHaveLength(6);
- expect(codeRepo.save).toHaveBeenCalledWith(
- expect.objectContaining({
- code,
- type: dto.type,
- key: dto.key,
- isVerified: false,
- expiredAt: expect.any(Date),
- }),
- );
- });
+ it('set code with custom duration', async () => {
+ const dto = new SetCodeEmailVerificationDto();
+ dto.key = faker.string.sample();
+ dto.type = CodeTypeEnum.EMAIL_VEIRIFICATION;
+ dto.durationSec = 120; // 2 minutes
+ const saveSpy = jest.spyOn(codeRepo, 'save');
+
+ MockDate.set('2025-01-01T00:00:00.000Z');
+
+ try {
+ const code = await codeService.setCode(dto);
+
+ expect(code).toHaveLength(6);
+ expect(saveSpy).toHaveBeenCalledWith(
+ expect.objectContaining({
+ code,
+ type: dto.type,
+ key: dto.key,
+ isVerified: false,
+ }),
+ );
+
+ const savedEntity = saveSpy.mock.calls[0][0] as CodeEntity;
+ expect(savedEntity.expiredAt.getTime()).toBe(
+ Date.now() + dto.durationSec * 1000,
+ );
+ } finally {
+ MockDate.reset();
+ }
+ });
+ it('set code with default duration when durationSec is not provided', async () => {
+ const dto = new SetCodeEmailVerificationDto();
+ dto.key = faker.string.sample();
+ dto.type = CodeTypeEnum.EMAIL_VEIRIFICATION;
+ const saveSpy = jest.spyOn(codeRepo, 'save');
+
+ MockDate.set('2025-01-01T00:00:00.000Z');
+
+ try {
+ const code = await codeService.setCode(dto);
+
+ expect(code).toHaveLength(6);
+ expect(saveSpy).toHaveBeenCalledWith(
+ expect.objectContaining({
+ code,
+ type: dto.type,
+ key: dto.key,
+ isVerified: false,
+ }),
+ );
+
+ const savedEntity = saveSpy.mock.calls[0][0] as CodeEntity;
+ expect(savedEntity.expiredAt.getTime()).toBe(
+ Date.now() + 5 * 60 * 1000,
+ );
+ } finally {
+ MockDate.reset();
+ }
+ });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/shared/code/code.service.spec.ts around lines 135 to 173, the
tests use the same duration as the default and only assert expiredAt is a Date,
so they don't verify custom vs default expiry behavior; update the tests to
freeze time (jest.useFakeTimers or mock Date.now) at a fixed timestamp, choose a
distinct custom duration (e.g., 120s) and a known default duration (keep
whatever the service default is if documented or set to another distinct value),
call setCode, capture the saved entity via the codeRepo.save spy, compute the
expected expiredAt by adding durationSec*1000 to the frozen time, and assert
that saved.expiredAt equals that exact Date for both the custom-duration test
and the default-duration test (for the latter omit durationSec and verify
fallback to the default).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/domains/admin/project/role/role.service.spec.ts(6 hunks)apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/src/domains/admin/project/role/role.service.spec.ts (1)
apps/api/src/test-utils/fixtures.ts (1)
getRandomEnumValues(165-170)
🪛 GitHub Actions: CI
apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts
[error] 687-687: Failed to create feedback issue statistics. Transaction failed
[error] 830-830: Failed to create feedback issue statistics. Transaction timeout
⏰ 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/src/domains/admin/project/role/role.service.spec.ts (2)
107-125: Stop double-spying onroleRepo.findOneByin this test
jest.spyOnwill throw (“Cannot spy on the same method more than once”) because we register a second spy after the first call. Reuse the original spy and chain themockResolvedValueOnceinvocations instead so the test can run its assertions.- jest.spyOn(roleRepo, 'findOneBy').mockResolvedValueOnce(null); + const findOneBySpy = jest.spyOn(roleRepo, 'findOneBy'); + findOneBySpy.mockResolvedValueOnce(null); jest.spyOn(roleRepo, 'save').mockResolvedValueOnce({ ...dto1, id: faker.number.int(), members: [], createdAt: new Date(), updatedAt: new Date(), deletedAt: null, } as unknown as RoleEntity); await roleService.create(dto1); - jest.spyOn(roleRepo, 'findOneBy').mockResolvedValueOnce({ + findOneBySpy.mockResolvedValueOnce({ id: faker.number.int(), } as unknown as RoleEntity);
590-635: Reuse the existing spies in the integration scenarioWe register fresh spies for
roleRepo.findOneByandroleRepo.savetwice in the same test, which makes Jest blow up before the workflow is exercised. Capture the spies once and update their return values as the flow evolves.- jest.spyOn(roleRepo, 'findOneBy').mockResolvedValue(null); - jest.spyOn(roleRepo, 'save').mockResolvedValue({ + const findOneBySpy = jest.spyOn(roleRepo, 'findOneBy'); + const saveSpy = jest.spyOn(roleRepo, 'save'); + findOneBySpy.mockResolvedValue(null); + saveSpy.mockResolvedValue({ id: faker.number.int(), project: { id: projectId }, members: [], createdAt: new Date(), updatedAt: new Date(), deletedAt: null, } as unknown as RoleEntity); @@ - jest.spyOn(roleRepo, 'findOneBy').mockResolvedValue(mockRoles[0]); + findOneBySpy.mockResolvedValue(mockRoles[0]); jest.spyOn(roleRepo, 'findOne').mockResolvedValue(null); - jest.spyOn(roleRepo, 'save').mockResolvedValue({ + saveSpy.mockResolvedValue({ ...mockRoles[0], ...updateDto, } as unknown as RoleEntity);apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts (6)
980-1059: Include entity identifiers when assertingsavepayloadAs noted earlier, the service mutates the fetched stats entity and passes it back to
repo.save, including the primary key. Expectations liketoHaveBeenCalledWith({ feedbackCount: 2 })drop the identifier and will never match the real call. Please assert on the merged entity (e.g.,expect.objectContaining({ id: existing.id, feedbackCount: … })) in these specs (default feedbackCount,negative feedback,very large feedback,edge case date).- jest.spyOn(feedbackIssueStatsRepo, 'findOne').mockResolvedValue({ - feedbackCount: 1, - } as FeedbackIssueStatisticsEntity); + const existingStats = { + id: faker.number.int(), + feedbackCount: 1, + } as FeedbackIssueStatisticsEntity; + jest + .spyOn(feedbackIssueStatsRepo, 'findOne') + .mockResolvedValue(existingStats); ... - expect(feedbackIssueStatsRepo.save).toHaveBeenCalledWith({ - feedbackCount: 2, // 1 (existing) + 1 (default) - }); + expect(feedbackIssueStatsRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + id: existingStats.id, + feedbackCount: existingStats.feedbackCount + 1, + }), + );Apply the same pattern to the subsequent assertions that currently omit the entity identifier.
834-841: Timeout rejection must be observed
createFeedbackIssueStatisticssurfaces the timeout error (CI failure at Line 830 shows the rejection). The spec should assert the promise rejects, not resolves.- // Should not throw error, but log it - await expect( - feedbackIssueStatsService.createFeedbackIssueStatistics( - projectId, - dayToCreate, - ), - ).resolves.not.toThrow(); + await expect( + feedbackIssueStatsService.createFeedbackIssueStatistics( + projectId, + dayToCreate, + ), + ).rejects.toThrow('Transaction timeout');
783-785: 0-feedback load test expects too many transactionsWith
feedbackRepo.countmocked to0, the service bails out before entering any transaction. ExpectingdayToCreate * issues.lengthcalls conflicts with the real flow.- // Transaction is called for each day*issue combination, but no database operations when feedback count is 0 - expect(transactionSpy).toHaveBeenCalledTimes(dayToCreate * issues.length); + expect(transactionSpy).not.toHaveBeenCalled();
468-487: Lock acquisition failure should skip cron registrationThe service immediately returns when
acquireLockyieldsfalse, soaddCronJobis never invoked. This expectation currently forces a behavior the implementation does not exhibit and will keep the suite red. Please assert that we don’t register the job when the lock can’t be acquired.- await feedbackIssueStatsService.addCronJobByProjectId(projectId); - - expect(schedulerRegistry.addCronJob).toHaveBeenCalledTimes(1); + await feedbackIssueStatsService.addCronJobByProjectId(projectId); + + expect(schedulerRegistry.addCronJob).not.toHaveBeenCalled();
546-555: Lock service rejection should bubble up
addCronJobByProjectIdpropagates the rejection fromschedulerLockService.acquireLockand never reachesaddCronJob. The pipeline failure confirms the promise rejects, so this spec needs to expect a rejection (and no registry call) instead of a silent success.- // Should not throw error, cron job should still be added - await expect( - feedbackIssueStatsService.addCronJobByProjectId(projectId), - ).resolves.not.toThrow(); - expect(schedulerRegistry.addCronJob).toHaveBeenCalledTimes(1); + await expect( + feedbackIssueStatsService.addCronJobByProjectId(projectId), + ).rejects.toThrow('Lock service error'); + expect(schedulerRegistry.addCronJob).not.toHaveBeenCalled();
723-725: Zero feedback should skip the transaction entirelyThe implementation returns early when
feedbackCount === 0; it never callsmanager.transaction. This expectation contradicts that contract and will keep failing.- // Transaction is called but no database operations are performed when feedback count is 0 - expect(transactionSpy).toHaveBeenCalledTimes(1); + expect(transactionSpy).not.toHaveBeenCalled();
| // Should not throw error, but log it | ||
| await expect( | ||
| feedbackIssueStatsService.createFeedbackIssueStatistics( | ||
| projectId, | ||
| dayToCreate, | ||
| ), | ||
| ).resolves.not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Transaction failures must surface to callers
When the transactional block rejects, the service propagates the error (see CI failure at Line 687). The test should assert the rejection rather than expecting a silent success.
- // Should not throw error, but log it
- await expect(
- feedbackIssueStatsService.createFeedbackIssueStatistics(
- projectId,
- dayToCreate,
- ),
- ).resolves.not.toThrow();
+ await expect(
+ feedbackIssueStatsService.createFeedbackIssueStatistics(
+ projectId,
+ dayToCreate,
+ ),
+ ).rejects.toThrow('Transaction failed');📝 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.
| // Should not throw error, but log it | |
| await expect( | |
| feedbackIssueStatsService.createFeedbackIssueStatistics( | |
| projectId, | |
| dayToCreate, | |
| ), | |
| ).resolves.not.toThrow(); | |
| }); | |
| await expect( | |
| feedbackIssueStatsService.createFeedbackIssueStatistics( | |
| projectId, | |
| dayToCreate, | |
| ), | |
| ).rejects.toThrow('Transaction failed'); | |
| }); |
🤖 Prompt for AI Agents
In
apps/api/src/domains/admin/statistics/feedback-issue/feedback-issue-statistics.service.spec.ts
around lines 689 to 696, the test currently expects
createFeedbackIssueStatistics to silently succeed when the transactional block
rejects; change the assertion to expect the promise to reject (e.g., use
rejects.toThrow()/rejects.toEqual(...) against the error you mock) so the test
asserts the error surfaces to callers; update the mock/setup if needed to
provide a deterministic error instance and assert that the returned/rejected
value matches that error.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores