ui: Add tests for silences datasource#5128
ui: Add tests for silences datasource#5128sysadmind wants to merge 2 commits intoprometheus:mainfrom
Conversation
Adds tests for silences data component for list of silences and query by ID. It covers the typical happy path and a few that result in an error. Signed-off-by: Joe Adams <github@joeadams.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new Vitest/React Testing Library test suite that exercises Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (2)
ui/mantine-ui/src/data/silences.test.tsx (2)
77-80: Wrap test wrappers withSuspensefor proper suspense-based query handling.The test uses
useSuspenseAPIQuery, which is implemented viauseSuspenseQueryand suspends during loading. Currently, wrappers at lines 77–80, 150–153, 176–178, and 264–266 provideQueryClientProviderand/orErrorBoundary, but noSuspenseboundary. This can cause incomplete test behavior sinceSuspenseis required to properly handle suspension states thrown byuseSuspenseQuery.🔧 Suggested wrapper adjustment
-import React, { ReactNode } from 'react'; +import React, { ReactNode, Suspense } from 'react'; @@ const getWrapper = (client: QueryClient) => { return ({ children }: { children: ReactNode }) => ( - <QueryClientProvider client={client}>{children}</QueryClientProvider> + <QueryClientProvider client={client}> + <Suspense fallback={null}>{children}</Suspense> + </QueryClientProvider> ); };Apply the same
Suspensewrapping to the error wrappers at lines 150–153, 176–178, and 264–266.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/data/silences.test.tsx` around lines 77 - 80, The test wrappers for suspense-based queries (e.g., getWrapper) are missing a Suspense boundary which breaks useSuspenseAPIQuery/useSuspenseQuery behavior; update the wrapper factories (the getWrapper function and the ErrorBoundary-wrapping factories used around tests) to wrap QueryClientProvider (and ErrorBoundary where used) inside a React.Suspense with a fallback (e.g., null) so suspended renders are handled properly; locate getWrapper and the other wrapper functions referenced in tests and nest their returned JSX inside <Suspense fallback={null}> ... </Suspense> while keeping QueryClientProvider and ErrorBoundary in place.
66-75: Add global cleanup to prevent fetch mock leakage between tests.The
afterEachhook only clears mock call history withvi.clearAllMocks()but does not restore stubbed globals. Since lines 92, 121, 147, 172, 199, 230, 260, and 301 directly reassignglobal.fetch, stub implementations persist across tests and create cross-test coupling.Use
vi.restoreAllMocks()inbeforeEachandvi.unstubAllGlobals()inafterEach, and replace direct assignments withvi.stubGlobal('fetch', mockFetch).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/mantine-ui/src/data/silences.test.tsx` around lines 66 - 75, Test teardown is leaking stubbed globals because tests directly reassign global.fetch and only call vi.clearAllMocks(); update the test setup to call vi.restoreAllMocks() in beforeEach and vi.unstubAllGlobals() in afterEach, and change any direct assignments to global.fetch in the tests to use vi.stubGlobal('fetch', mockFetch) so each test gets an isolated stub that is properly restored; look for beforeEach, afterEach, global.fetch assignments, and replace them with vi.restoreAllMocks(), vi.unstubAllGlobals(), and vi.stubGlobal calls respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/mantine-ui/src/data/silences.test.tsx`:
- Around line 282-315: The test is falsely passing because it creates two
QueryClient instances; to actually validate cache key isolation, use a single
QueryClient for both renderHook calls so they share cache. Update the test that
uses useSilence, QueryClient, renderHook and getWrapper to create one client
(const client = new QueryClient(...)), renderHook(() => useSilence(id1), {
wrapper: getWrapper(client) }) and renderHook(() => useSilence(id2), { wrapper:
getWrapper(client) }), then assert mockFetch was called twice; additionally add
(or modify) an assertion that rendering useSilence(id1) twice with the same
client results in only one fetch call to ensure same-ID cache hits.
---
Nitpick comments:
In `@ui/mantine-ui/src/data/silences.test.tsx`:
- Around line 77-80: The test wrappers for suspense-based queries (e.g.,
getWrapper) are missing a Suspense boundary which breaks
useSuspenseAPIQuery/useSuspenseQuery behavior; update the wrapper factories (the
getWrapper function and the ErrorBoundary-wrapping factories used around tests)
to wrap QueryClientProvider (and ErrorBoundary where used) inside a
React.Suspense with a fallback (e.g., null) so suspended renders are handled
properly; locate getWrapper and the other wrapper functions referenced in tests
and nest their returned JSX inside <Suspense fallback={null}> ... </Suspense>
while keeping QueryClientProvider and ErrorBoundary in place.
- Around line 66-75: Test teardown is leaking stubbed globals because tests
directly reassign global.fetch and only call vi.clearAllMocks(); update the test
setup to call vi.restoreAllMocks() in beforeEach and vi.unstubAllGlobals() in
afterEach, and change any direct assignments to global.fetch in the tests to use
vi.stubGlobal('fetch', mockFetch) so each test gets an isolated stub that is
properly restored; look for beforeEach, afterEach, global.fetch assignments, and
replace them with vi.restoreAllMocks(), vi.unstubAllGlobals(), and vi.stubGlobal
calls respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a2407b9-e6eb-4966-9d34-8329109cd49d
📒 Files selected for processing (1)
ui/mantine-ui/src/data/silences.test.tsx
Signed-off-by: Joe Adams <github@joeadams.io>
|
I have deep scars from mocking, and we really prefer seeing tests running against a real server. Maybe like so: let queryClient: QueryClient;
let respond: (req: IncomingMessage, res: ServerResponse) => void;
const server = createServer((req, res) => respond(req, res));
beforeAll(async () => {
await new Promise<void>((resolve) => server.listen(0, resolve));
const { port } = server.address() as AddressInfo;
vi.stubEnv('VITE_API_PREFIX', `http://localhost:${port}`);
});
afterAll(async () => {
vi.unstubAllEnvs();
await new Promise<void>((resolve) => server.close(resolve));
});
beforeEach(() => {
queryClient = new QueryClient({
defaultOptions: { queries: { retry: false } },
});
respond = (_, res) => { res.writeHead(500); res.end(); };
});
afterEach(() => {
queryClient.clear();
});It's probably not ideal code, since I'm a complete typescript novice and have to trust the AI. But it does run locally: SoloJacobs@541a729 Another thing: It appears we are mostly testing |
2 participants
Adds tests for silences data component for list of silences and query by ID. It covers the typical happy path and a few that result in an error.
Pull Request Checklist
Please check all the applicable boxes.
Which user-facing changes does this PR introduce?
Summary by CodeRabbit