test(react-query/useSuspenseQueries): add test for suspending only pending queries when some already have cached data#10126
Conversation
…nding queries when some already have cached data
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a test for useSuspenseQueries that verifies when multiple queries run with mixed cache states (one cached, one pending), only the pending query suspends while the cached query returns immediately and both update after a background refetch. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit f61c23d
☁️ Nx Cloud last updated this comment at |
TkDodo
left a comment
TkDodo
left a comment
There was a problem hiding this comment.
i don’t think this test tests what the description says it tests. In order to see that we only suspend until key2 is “done”, they would both need to have a different sleep time (likely shorter for key2 than for key1). Since key1 already has data but triggers a background refetch, we would only suspend until key2 is done. Then, data should be visible, but it should be cached data from key1 and fresh data from key2. At that time, the background refetch for key1 ist still ongoing, and eventually it should settle with data1: data1. Does that make sense?
… verify background refetch settles with fresh data
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-query/src/__tests__/useSuspenseQueries.test.tsx (1)
880-888: Timing in the test appears to be longer than necessary.The test advances time by a total of 4000ms, but based on the query configuration:
- key1's queryFn takes 2000ms and its background refetch should start at mount (since
staleTimedefaults to 0, cached data is immediately stale)- key2's queryFn takes 1000ms
If key1's background refetch starts at mount, it should complete at t=2000ms, not t=4000ms. The current timing will still pass but doesn't precisely verify the expected behavior.
Additionally, the comment "key1 stale timer fires, triggering background refetch" at line 880 may be misleading—with default
staleTime: 0, the background refetch should trigger immediately when the component mounts, not after a separate stale timer.Consider simplifying the timing
expect(rendered.getByText('data1: cached')).toBeInTheDocument() expect(rendered.getByText('data2: data2')).toBeInTheDocument() - // key1 stale timer fires, triggering background refetch - await act(() => vi.advanceTimersByTimeAsync(1000)) - - // key1 background refetch completes: key1 updates to fresh data - await act(() => vi.advanceTimersByTimeAsync(2000)) + // key1 background refetch completes (started at mount, takes 2000ms total) + await act(() => vi.advanceTimersByTimeAsync(1000)) expect(rendered.getByText('data1: data1')).toBeInTheDocument() expect(rendered.getByText('data2: data2')).toBeInTheDocument()Please verify whether the background refetch for key1 starts immediately at mount or after a delay. If there's specific behavior that justifies the 4000ms total time, consider adding a clarifying comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-query/src/__tests__/useSuspenseQueries.test.tsx` around lines 880 - 888, The test advances timers longer than necessary and the comment is misleading: because queries default to staleTime: 0, key1's background refetch starts immediately at mount and should complete at t=2000ms (queryFn takes 2000ms), so replace the two vi.advanceTimersByTimeAsync calls that total 4000ms with a single advance to 2000ms (using act(() => vi.advanceTimersByTimeAsync(2000))) and update the comment near the advances to say "key1 background refetch started at mount (staleTime: 0) and completes at 2000ms" to accurately reflect behavior; locate and change the lines referencing key1/key2, the act+/vi.advanceTimersByTimeAsync calls, and the comment in useSuspenseQueries.test.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-query/src/__tests__/useSuspenseQueries.test.tsx`:
- Around line 880-888: The test advances timers longer than necessary and the
comment is misleading: because queries default to staleTime: 0, key1's
background refetch starts immediately at mount and should complete at t=2000ms
(queryFn takes 2000ms), so replace the two vi.advanceTimersByTimeAsync calls
that total 4000ms with a single advance to 2000ms (using act(() =>
vi.advanceTimersByTimeAsync(2000))) and update the comment near the advances to
say "key1 background refetch started at mount (staleTime: 0) and completes at
2000ms" to accurately reflect behavior; locate and change the lines referencing
key1/key2, the act+/vi.advanceTimersByTimeAsync calls, and the comment in
useSuspenseQueries.test.tsx.
sukvvon
force-pushed
the
test/react-query-useSuspenseQueries-partial-cached-suspend
branch
from
4a85919 to
f61c23d
Compare
2 participants
🎯 Changes
Add a test to verify that
useSuspenseQueriesonly suspends queries that are still pending, skipping queries that already have cached data viasetQueryData.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit