WEB-341 Add Integration Tests for CreateClientComponent #2700
Conversation
WalkthroughCentralizes and explicitizes Jest configuration, adds a global Angular/Jest setup file, adjusts TypeScript testing paths/types, updates npm test scripts and ts-node, and introduces an extensive integration test suite for CreateClientComponent. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant AR as ActivatedRoute
participant C as CreateClientComponent
participant SS as SettingsService
participant CS as ClientsService
participant R as Router
note right of AR #DFF2E1: Route-resolved data provided\n(clientTemplate, addressConfig)
AR-->>C: provide resolved data
SS-->>C: getSettings() -> locale/dateFormat
U->>C: fill forms (general, family, address, datatables)
C->>C: validate sections & datatables
alt valid
C->>CS: createClient(aggregatedPayload)
CS-->>C: createdClient (id)
C->>R: navigate to ['clients', id]
else invalid
C->>U: display validation errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
gkbishnoi07
force-pushed
the
WEB-341-add-integration-tests-for-create-client-component
branch
from
a918073 to
7a1bcbc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/clients/create-client/create-client.component.spec.ts (1)
58-110: Trigger change detection during setupRight after creating the component we never call
fixture.detectChanges(), so Angular lifecycle hooks (e.g.,ngOnInit) stay dormant. The suite currently leans on constructor state, but any future move of init logic into lifecycle hooks will quietly break these tests. Please run an initialfixture.detectChanges()in the sharedbeforeEachto keep the tests aligned with real runtime behavior. As per coding guidelines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**/*
📒 Files selected for processing (5)
jest.config.ts(1 hunks)package.json(2 hunks)src/app/clients/create-client/create-client.component.spec.ts(1 hunks)src/setup-jest.ts(1 hunks)src/tsconfig.spec.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/clients/create-client/create-client.component.spec.ts
**/*.spec.ts
⚙️ CodeRabbit configuration file
**/*.spec.ts: For tests: encourage clear Arrange-Act-Assert structure,
stable selectors, and minimal brittle timing dependencies.
Files:
src/app/clients/create-client/create-client.component.spec.ts
⏰ 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: Run Lint, Build and Deploy
| const localStorageMock = { | ||
| getItem: jest.fn(), | ||
| setItem: jest.fn(), | ||
| removeItem: jest.fn(), | ||
| clear: jest.fn(), | ||
| length: 0, | ||
| key: jest.fn() | ||
| }; | ||
|
|
||
| Object.defineProperty(window, 'localStorage', { | ||
| value: localStorageMock | ||
| }); | ||
|
|
||
| // Mock sessionStorage | ||
| Object.defineProperty(window, 'sessionStorage', { | ||
| value: localStorageMock | ||
| }); |
There was a problem hiding this comment.
Separate the storage mocks to mirror browser behavior
Both localStorage and sessionStorage are wired to the same backing object, so writes to one instantly appear in the other. That diverges from the browser contract and can mask regressions when code relies on the storages being isolated. Please create independent mocks (or factory per storage) before defining them on window.
-const localStorageMock = {
- getItem: jest.fn(),
- setItem: jest.fn(),
- removeItem: jest.fn(),
- clear: jest.fn(),
- length: 0,
- key: jest.fn()
-};
+const createStorageMock = () => ({
+ getItem: jest.fn(),
+ setItem: jest.fn(),
+ removeItem: jest.fn(),
+ clear: jest.fn(),
+ length: 0,
+ key: jest.fn()
+});
+
+const localStorageMock = createStorageMock();
Object.defineProperty(window, 'localStorage', {
- value: localStorageMock
+ value: localStorageMock
});
-// Mock sessionStorage
-Object.defineProperty(window, 'sessionStorage', {
- value: localStorageMock
-});
+const sessionStorageMock = createStorageMock();
+
+Object.defineProperty(window, 'sessionStorage', {
+ value: sessionStorageMock
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const localStorageMock = { | |
| getItem: jest.fn(), | |
| setItem: jest.fn(), | |
| removeItem: jest.fn(), | |
| clear: jest.fn(), | |
| length: 0, | |
| key: jest.fn() | |
| }; | |
| Object.defineProperty(window, 'localStorage', { | |
| value: localStorageMock | |
| }); | |
| // Mock sessionStorage | |
| Object.defineProperty(window, 'sessionStorage', { | |
| value: localStorageMock | |
| }); | |
| const createStorageMock = () => ({ | |
| getItem: jest.fn(), | |
| setItem: jest.fn(), | |
| removeItem: jest.fn(), | |
| clear: jest.fn(), | |
| length: 0, | |
| key: jest.fn() | |
| }); | |
| const localStorageMock = createStorageMock(); | |
| Object.defineProperty(window, 'localStorage', { | |
| value: localStorageMock | |
| }); | |
| const sessionStorageMock = createStorageMock(); | |
| Object.defineProperty(window, 'sessionStorage', { | |
| value: sessionStorageMock | |
| }); |
🤖 Prompt for AI Agents
In src/setup-jest.ts around lines 13 to 29, both localStorage and sessionStorage
are assigned the same mock object so mutations to one affect the other; create
independent mocks (or call a mock factory twice) so each storage has its own
getItem/setItem/removeItem/clear/length/key implementations and then define them
separately on window to mirror real browser isolation.
There was a problem hiding this comment.
@coderabbitai is this a problem or just a suggestion?
There was a problem hiding this comment.
@gkbishnoi07, it's a potential problem that can manifest as an actual bug in specific scenarios. Let me explain:
Why it's problematic:
Since both storages point to the same mock object, they share:
- Mock call history: If you verify
localStorage.setItemwas called, you can't distinguish whether it was actuallylocalStorageorsessionStorage - Spies and implementations: Any
jest.spyOn()or mock implementation set on one affects the other - Mock function state: Call counts, return values, and implementations are shared
When it causes real issues:
- Tests that use both storages: If your component uses both
localStorageandsessionStorage, your tests won't catch bugs where the wrong storage is used - Mock verification: When asserting
expect(localStorage.setItem).toHaveBeenCalledWith(...), calls tosessionStorage.setItemwill also count - Different implementations: If you need different behavior for each storage in a test, you can't achieve that
When it might be acceptable:
- If your current tests only interact with one storage at a time
- If you never verify mock calls on these storages
- If your components don't rely on the isolation between storages
Recommendation:
Given that the fix is simple (just create independent mocks), I recommend applying it to prevent future headaches. The suggested change ensures your test environment accurately mirrors browser behavior, making your tests more reliable and easier to reason about.
Would you like to apply the fix, or do you have specific reasons to keep them shared?
|
@IOhacker @alberto-art3ch @steinwinde please review my test file |
1 participant
Description
Add Integration Tests for Create-Client.Component.ts file
Related issues and discussion
WEB-341
Summary by CodeRabbit
Tests
Chores