test: add missing tests for React hooks #225
Conversation
cristianoventura
requested review from
e-fisher,
Llandy3d and
going-confetti
and removed request for
a team
There was a problem hiding this comment.
🙌
Overall looks good, left a few comments about casting as Mock and beforeAll/beforeEach placement - they apply to multiple files, but I didn't repeat comments to reduce noise.
Also, the useSetWindowTitle is currently not used anywhere, perhaps we keep it in case we use it later since it's well tested now. 😄 But for the next tests, I would suggest focusing on discovering and testing "fragile" and critical parts of the app, instead of striving for 100% coverage on specific type of components.
| import { renderHook } from '@testing-library/react' | ||
| import { useCloseSplashScreen } from './useCloseSplashScreen' | ||
| import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| const closeSplashscreen = vi.fn() | ||
|
|
||
| beforeAll(() => { | ||
| window.studio = { | ||
| ...window.studio, | ||
| app: { | ||
| closeSplashscreen, | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| describe('useCloseSplashScreen', () => { | ||
| it('should call closeSplashscreen on mount', () => { | ||
| renderHook(() => useCloseSplashScreen()) | ||
|
|
||
| expect(closeSplashscreen).toHaveBeenCalled() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
There's always this line between not enough tests and testing too much - I think there are some hooks that deserve test coverage, like useListenProxyData or useProxyDataGroups, which have some logic covering cases not immediately obvious when just looking at code. But testing hooks like useCloseSplashScreen feels an overkill, it becomes more of a framework testing - verifying useEffect behaves as expected and executes on mount, which is likely already tested in react codebase.
src/hooks/useCreateGenerator.test.ts
Outdated
| ;(createNewGeneratorFile as Mock).mockReturnValue(newGenerator) | ||
| ;(generateFileNameWithTimestamp as Mock).mockReturnValue(fileName) | ||
| ;(getRoutePath as Mock).mockReturnValue(routePath) |
There was a problem hiding this comment.
By casting as Mock you lose type safety and mockReturnValue accepts any.
There's a helper vi.mocked which allows removing cast (as well as this ugly syntax 😅) and preserve type safety:
| ;(createNewGeneratorFile as Mock).mockReturnValue(newGenerator) | |
| ;(generateFileNameWithTimestamp as Mock).mockReturnValue(fileName) | |
| ;(getRoutePath as Mock).mockReturnValue(routePath) | |
| vi.mocked(createNewGeneratorFile).mockReturnValue(newGenerator) | |
| vi.mocked(generateFileNameWithTimestamp).mockReturnValue(fileName) | |
| vi.mocked(getRoutePath).mockReturnValue(routePath) |
You'll get a typescript error on the first line because { id: 'test' } is not a valid GeneratorFileData, but that's expected, and a fixture should be created
| const proxyDataWithResponse = createProxyData() | ||
| const proxyDataWithoutResponse = createProxyDataWithoutResponse() | ||
| const responseWith304StatusCode = createResponse({ | ||
| statusCode: 304, | ||
| content: '', | ||
| headers: [], | ||
| cookies: [], | ||
| query: [], | ||
| scheme: 'http', | ||
| host: 'example.com', | ||
| method: 'GET', | ||
| path: '/api/v1/users', | ||
| content: null, | ||
| timestampStart: 0, | ||
| timestampEnd: 0, | ||
| contentLength: 0, | ||
| httpVersion: '1.1', | ||
| url: 'http://example.com', | ||
| } | ||
|
|
||
| const response: ProxyData['response'] = { | ||
| statusCode: 200, | ||
| headers: [['content-type', 'application/json']], | ||
| cookies: [], | ||
| reason: 'OK', | ||
| content: '{"hello":"world"}', | ||
| path: '/api/v1/users', | ||
| httpVersion: '1.1', | ||
| timestampStart: 0, | ||
| contentLength: 0, | ||
| } | ||
|
|
||
| const proxyDataWithoutResponse: ProxyData = { | ||
| id: '1', | ||
| request, | ||
| } | ||
|
|
||
| const proxyDataWithResponse: ProxyData = { | ||
| id: '1', | ||
| request, | ||
| response, | ||
| } | ||
|
|
||
| const proxyDataWith304Response: ProxyData = { | ||
| ...proxyDataWithResponse, | ||
| request, | ||
| response: { | ||
| ...response, | ||
| statusCode: 304, | ||
| content: '', | ||
| headers: [], | ||
| }, | ||
| } | ||
| }) | ||
| const proxyDataWith304Response = createProxyData({ | ||
| response: responseWith304StatusCode, | ||
| }) |
src/hooks/useRunChecks.test.ts
Outdated
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| }) |
There was a problem hiding this comment.
Should this be inside describe?
src/hooks/useRunChecks.test.ts
Outdated
| window.studio = { | ||
| ...window.studio, | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-expect-error | ||
| script: { | ||
| onScriptCheck, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Slightly shorter and won't require eslint/ts comments
| window.studio = { | |
| ...window.studio, | |
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | |
| // @ts-expect-error | |
| script: { | |
| onScriptCheck, | |
| }, | |
| } | |
| vi.stubGlobal('studio', { | |
| script: { | |
| onScriptCheck, | |
| }, | |
| }) |
Description
This PR adds missing tests for the React hooks present in
./src/hooks/How to Test
Checklist
npm run lint) and all checks pass.npm test) and all tests pass.Screenshots (if appropriate):
N/A
Related PR(s)/Issue(s)
N/A