Skip to content

Added test cases for services: form-detector, groq-service, session-tracker.#24

Open
kanjambhatlidhoo wants to merge 1 commit intoShantanugupta43:mainfrom
kanjambhatlidhoo:fix/add-unit-test-cases
Open

Added test cases for services: form-detector, groq-service, session-tracker.#24
kanjambhatlidhoo wants to merge 1 commit intoShantanugupta43:mainfrom
kanjambhatlidhoo:fix/add-unit-test-cases

Conversation

@kanjambhatlidhoo
Copy link
Copy Markdown

Added some test cases covering functionality for test cases.

});

test('getIntentContext resets expired session', async () => {
const oldTime = Date.now() - (3 * 60 * 60 * 1000);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TTL in the actual code is 2 hours, but this test uses 3 hours. It passes today by accident. Change the hardcoded 3 * to read sessionTracker.SESSION_TTL_MS directly so if someone changes the TTL later, the test actually catches it. Also worth adding a second test that checks a session just before expiry is NOT reset.

apiSpy.mockRestore();
});

test('calls Groq API and parses valid JSON response', async () => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test only checks the happy path but never confirms isFormFill is absent. The whole point of the form-fill shortcut is that it sets isFormFill: true and skips the API. Without asserting isFormFill is falsy on the API path, someone could accidentally set it everywhere and this test wouldn't catch it. Add expect(result.isFormFill).toBeFalsy().

expect(result.suggestions[0].text).toBe('suggestion one');
});

test('parseResponse returns empty suggestions on invalid payload', () => {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The console.error call inside parseResponse fires visibly every time this test runs. It's expected behaviour but pollutes CI output. Wrap the call in jest.spyOn(console, 'error').mockImplementation(() => {}) and restore it after.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry logic on 429 responses in callWithRetry has no test at all. If someone breaks that retry, nothing fails. Worth adding a test that mocks the first fetch as 429 and the second as success, and asserts fetch was called twice.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bug in the source worth flagging: _detectOS checks for Windows NT 11 but real Windows 11 browsers still report Windows NT 10.0 in the UA string — same as Windows 10. That branch can never match. The tests don't cover this so the bug is invisible. Either fix _detectOS to use navigator.userAgentData instead, or at minimum add a test with a real Windows 11 UA string that documents what actually comes back.

Copy link
Copy Markdown
Owner

@Shantanugupta43 Shantanugupta43 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi thanks for contributing there are few improvements worth addressing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants