feat(cc-digital-channel): add-uts-for-digital-channel#611
feat(cc-digital-channel): add-uts-for-digital-channel#611akulakum merged 10 commits intowebex:nextfrom
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
adhmenon
left a comment
There was a problem hiding this comment.
LGTM. One question, only.
| const storeModule = require('@webex/cc-store'); | ||
| expect(storeModule.default.currentTask).toBeTruthy(); | ||
| expect(storeModule.default.logger).toBeTruthy(); | ||
| expect(storeModule.default.dataCenter).toBe('produs1'); |
There was a problem hiding this comment.
Do we know this is always going to be the default?
There was a problem hiding this comment.
Good catch! 'produs1' is not a default value - it's a mock test value we configure in the test setup. I've refactored to use a MOCK_DATA_CENTER constant to make this clearer. The test verifies that the component correctly receives whatever dataCenter value is configured in the store.
rsarika
left a comment
There was a problem hiding this comment.
Great work on adding comprehensive unit tests for the cc-digital-channels package! The tests are well-structured, follow proper MobX/React testing patterns, and cover important edge cases including error handling.
Highlights:
- Good use of
act()withrunInAction()for MobX state updates - Proper mock strategy using
jest.requireActual - Nice refactor to use named constants for mock values
- Good coverage of error scenarios
Left a few minor suggestions as inline comments - these are non-blocking improvements for future consideration.
| expect(screen.container.querySelector('div > md-theme')).toBeNull(); | ||
| expect(screen.queryByTestId('engage-widget')).toBeNull(); | ||
| expect(screen.container.innerHTML).toBe(''); | ||
|
|
There was a problem hiding this comment.
Suggestion (non-blocking): Consider extracting the store state restoration logic into a nested describe block with afterEach for cleaner test isolation:
describe('when dataCenter is empty', () => {
let originalDataCenter: string;
beforeEach(() => {
originalDataCenter = store.dataCenter;
act(() => { runInAction(() => { store.dataCenter = ''; }); });
});
afterEach(() => {
act(() => { runInAction(() => { store.dataCenter = originalDataCenter; }); });
});
it('should not render', async () => { /* ... */ });
});This pattern makes test cleanup more explicit and less error-prone.
There was a problem hiding this comment.
Thanks for the suggestion! Refactored to use nested describe blocks with beforeEach/afterEach for dataCenter and currentTask tests. This makes the test cleanup more explicit and isolated.
|
|
||
| const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| mockShouldThrow = true; | ||
|
|
There was a problem hiding this comment.
Suggestion (non-blocking): The consoleErrorSpy pattern is used multiple times. Consider extracting to a reusable helper or using beforeEach/afterEach:
let consoleErrorSpy: jest.SpyInstance;
beforeEach(() => {
consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
});
afterEach(() => {
consoleErrorSpy.mockRestore();
});There was a problem hiding this comment.
Good catch! Moved consoleErrorSpy to beforeEach/afterEach in the ErrorBoundary describe block. Also reset mockShouldThrow in afterEach for cleaner test isolation.
| const {result} = renderHook(() => | ||
| useDigitalChannelsData({ | ||
| getAccessToken, | ||
| currentTask: defaultTask, |
There was a problem hiding this comment.
Suggestion (non-blocking): Consider a more descriptive test name that captures the full scenario:
it('should handle token fetch error gracefully when logger is undefined')This makes test failures easier to diagnose.
There was a problem hiding this comment.
Updated the test name to 'should handle token fetch error gracefully when logger is undefined' for better clarity on what scenario is being tested.
| storeWrapper['store'].cc.webex.credentials.getUserToken = jest | ||
| .fn() | ||
| .mockResolvedValue({access_token: mockAccessToken}); | ||
|
|
There was a problem hiding this comment.
Suggestion (non-blocking): There are several @ts-expect-error comments in these tests for the webex credentials API. For future maintainability, consider:
- Creating a typed mock interface for the credentials API, or
- Adding type definitions to the store's type declarations
This would improve IDE support and catch potential type mismatches early.
There was a problem hiding this comment.
This is a larger refactoring effort for future consideration - no changes made as it's non-blocking.
COMPLETES #https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7458
This pull request addresses
Adding comprehensive unit tests for the cc-digital-channels package to improve code coverage and test reliability.
by making the following changes
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging
Make sure to have followed the contributing guidelines before submitting.