-
Notifications
You must be signed in to change notification settings - Fork 502
fix: use authenticated API for remote config polling #8266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
|
|
||
| import { api } from '@/scripts/api' | ||
|
|
||
| import { refreshRemoteConfig } from './refreshRemoteConfig' | ||
| import { remoteConfig } from './remoteConfig' | ||
|
|
||
| vi.mock('@/scripts/api', () => ({ | ||
| api: { | ||
| fetchApi: vi.fn() | ||
| } | ||
| })) | ||
|
|
||
| global.fetch = vi.fn() | ||
|
|
||
| describe('refreshRemoteConfig', () => { | ||
| const mockConfig = { feature1: true, feature2: 'value' } | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| remoteConfig.value = {} | ||
| window.__CONFIG__ = {} | ||
| }) | ||
|
|
||
| describe('with auth (default)', () => { | ||
| it('uses api.fetchApi when useAuth is true', async () => { | ||
| vi.mocked(api.fetchApi).mockResolvedValue({ | ||
| ok: true, | ||
| json: async () => mockConfig | ||
| } as Response) | ||
|
Comment on lines
+26
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need the type assertion? |
||
|
|
||
| await refreshRemoteConfig({ useAuth: true }) | ||
|
|
||
| expect(api.fetchApi).toHaveBeenCalledWith('/features', { | ||
| cache: 'no-store' | ||
| }) | ||
| expect(global.fetch).not.toHaveBeenCalled() | ||
| expect(remoteConfig.value).toEqual(mockConfig) | ||
| expect(window.__CONFIG__).toEqual(mockConfig) | ||
| }) | ||
|
|
||
| it('uses api.fetchApi by default', async () => { | ||
| vi.mocked(api.fetchApi).mockResolvedValue({ | ||
| ok: true, | ||
| json: async () => mockConfig | ||
| } as Response) | ||
|
|
||
| await refreshRemoteConfig() | ||
|
|
||
| expect(api.fetchApi).toHaveBeenCalled() | ||
| expect(global.fetch).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
|
|
||
| describe('without auth', () => { | ||
| it('uses raw fetch when useAuth is false', async () => { | ||
| vi.mocked(global.fetch).mockResolvedValue({ | ||
| ok: true, | ||
| json: async () => mockConfig | ||
| } as Response) | ||
|
Comment on lines
+57
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: This pattern seems repeated enough times in here to be worth extracting. |
||
|
|
||
| await refreshRemoteConfig({ useAuth: false }) | ||
|
|
||
| expect(global.fetch).toHaveBeenCalledWith('/api/features', { | ||
| cache: 'no-store' | ||
| }) | ||
| expect(api.fetchApi).not.toHaveBeenCalled() | ||
| expect(remoteConfig.value).toEqual(mockConfig) | ||
| expect(window.__CONFIG__).toEqual(mockConfig) | ||
| }) | ||
| }) | ||
|
|
||
| describe('error handling', () => { | ||
| it('clears config on 401 response', async () => { | ||
| vi.mocked(api.fetchApi).mockResolvedValue({ | ||
| ok: false, | ||
| status: 401, | ||
| statusText: 'Unauthorized' | ||
| } as Response) | ||
|
|
||
| await refreshRemoteConfig() | ||
|
|
||
| expect(remoteConfig.value).toEqual({}) | ||
| expect(window.__CONFIG__).toEqual({}) | ||
| }) | ||
|
|
||
| it('clears config on 403 response', async () => { | ||
| vi.mocked(api.fetchApi).mockResolvedValue({ | ||
| ok: false, | ||
| status: 403, | ||
| statusText: 'Forbidden' | ||
| } as Response) | ||
|
|
||
| await refreshRemoteConfig() | ||
|
|
||
| expect(remoteConfig.value).toEqual({}) | ||
| expect(window.__CONFIG__).toEqual({}) | ||
| }) | ||
|
|
||
| it('clears config on fetch error', async () => { | ||
| vi.mocked(api.fetchApi).mockRejectedValue(new Error('Network error')) | ||
|
|
||
| await refreshRemoteConfig() | ||
|
|
||
| expect(remoteConfig.value).toEqual({}) | ||
| expect(window.__CONFIG__).toEqual({}) | ||
| }) | ||
|
Comment on lines
+73
to
+107
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider adding a test for non-auth error statuses (e.g., 500). The current tests cover 401 and 403 (which clear config) and fetch errors. However, the implementation preserves the existing config for other non-ok statuses like 500. Adding a test for this scenario would document the expected behavior and prevent regressions. 📝 Suggested additional test caseit('preserves config on 500 response', async () => {
remoteConfig.value = { existingFeature: true }
window.__CONFIG__ = { existingFeature: true }
vi.mocked(api.fetchApi).mockResolvedValue({
ok: false,
status: 500,
statusText: 'Internal Server Error'
} as Response)
await refreshRemoteConfig()
// Config should NOT be cleared on transient server errors
expect(remoteConfig.value).toEqual({ existingFeature: true })
expect(window.__CONFIG__).toEqual({ existingFeature: true })
})🤖 Prompt for AI Agents |
||
| }) | ||
| }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I know it's not coming from this PR, but this feels like it should be a part of the remoteConfig module, not in a separate file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nit: I usually like stubGlobal for consistency and control