test(react-query-persist-client/PersistQueryClientProvider): switch to fake timers, replace 'await' with 'advanceTimersByTimeAsync', and use 'sleep().then()' pattern #9859
Conversation
|
WalkthroughTests refactored to use fake timers and explicit timer advancement; waitFor and sleep-based delays replaced by timer-driven promise chains and direct DOM assertions; persistence restore synchronization added via a restoration Promise; additional StrictMode and multi-client scenarios covered. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test
participant FakeTimer as Fake Timer (vi)
participant Persister
participant QueryClient
participant DOM
Note over Test,FakeTimer: Test sets up fake timers and mounts provider
Test->>Persister: call .restoreClient()
Persister-->>Test: returns restorePromise (pending)
Test->>QueryClient: register queries (queryFn returns Promise via sleep.then(...))
Note over FakeTimer,QueryClient: No real time passes until timers advanced
Test->>FakeTimer: advanceTimersByTimeAsync(x)
FakeTimer->>QueryClient: timers fire, queryFns resolve
QueryClient->>DOM: update UI states ("hydrated","fetched","updated")
DOM-->>Test: render updates asserted via toBeInTheDocument
Note over Test: Repeat advances to drive subsequent state transitions
Estimated code review effortπ― 2 (Simple) | β±οΈ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touchesβ Failed checks (2 warnings)
β Passed checks (1 passed)
β¨ Finishing touches
π§ͺ Generate unit tests (beta)
π Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro π Files selected for processing (1)
𧰠Additional context used𧬠Code graph analysis (1)packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (2)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
π Additional comments (4)
Comment |
|
View your CI Pipeline Execution β for commit 6d43ba3
βοΈ Nx Cloud last updated this comment at |
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9859 +/- ##
============================================
+ Coverage 45.76% 100.00% +54.23%
============================================
Files 200 1 -199
Lines 8410 18 -8392
Branches 1930 2 -1928
============================================
- Hits 3849 18 -3831
+ Misses 4113 0 -4113
+ Partials 448 0 -448 π New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
π§Ή Nitpick comments (1)
packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (1)
75-76: Consider awaitingpersistQueryClientSavefor clarity and error handling.The pattern of calling
persistQueryClientSavewithoutawaitfollowed byawait vi.advanceTimersByTimeAsync(0)works in this test context, but it could lead to unhandled promise rejections ifpersistQueryClientSavethrows an error. For better clarity and error handling, consider:- persistQueryClientSave({ queryClient, persister }) - await vi.advanceTimersByTimeAsync(0) + await persistQueryClientSave({ queryClient, persister })This pattern appears throughout the test file at lines 75-76, 150-151, 218-219, 298-299, 378-379, 449-450, 501-502, 620-621, and 758-759.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx(24 hunks)
π§° Additional context used
𧬠Code graph analysis (1)
packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (2)
packages/query-persist-client-core/src/__tests__/utils.ts (1)
createMockPersister(5-20)packages/query-persist-client-core/src/persist.ts (1)
persistQueryClientSave(114-127)
π Additional comments (9)
packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (9)
54-60: LGTM!The fake timer setup and teardown is correctly implemented following vitest best practices.
17-31: LGTM!The conversion from
async/awaitto promise chaining (sleep().then()) aligns with the PR objectives and works correctly with fake timers.
33-51: LGTM!The error persister correctly uses promise chaining to simulate a delayed throw, which integrates well with the fake timer approach.
62-136: LGTM!The test correctly implements the fake timer pattern:
- Initial prefetch and persistence setup with appropriate timer advances
- Restoration and fetching sequences driven by explicit timer controls
- Proper use of
act()for timer advances after component mountThe use of 11ms (instead of exactly 10ms) for the fetch advance is intentional to account for React render cycles and microtasks, which helps prevent flakiness.
198-200: LGTM!The pattern of capturing the promise before advancing timers is the correct approach with fake timers:
const statePromise = persister.restoreClient() await act(() => vi.advanceTimersByTimeAsync(10)) const state = await statePromiseThis ensures the timer advances before awaiting the result, avoiding the need for multiple timer advances.
511-516: Verify: Intentional use of async/await pattern.This test uses
async/awaitin thequeryFnwhile all other tests use promise chaining (sleep().then()). Given the test name "should await onSuccess after successful restoring" and that theonSuccesshandler also usesasync/await(lines 531-535), this appears intentional to verify that async/await patterns work correctly with fake timers.If this is intentional to provide coverage for both patterns, consider adding a comment explaining why this test differs from others. Otherwise, consider converting to promise chaining for consistency:
- queryFn: async () => { - states.push('fetching') - await sleep(10) - states.push('fetched') - return 'fetched' - }, + queryFn: () => + sleep(0).then(() => { + states.push('fetching') + return sleep(10) + }).then(() => { + states.push('fetched') + return 'fetched' + }),
205-435: LGTM!These tests correctly implement the fake timer pattern with:
- Consistent use of promise chaining in
queryFnimplementations- Appropriate timer advances for restoration and fetching sequences
- Correct verification of query states and fetch behaviors
437-605: LGTM!These tests properly validate the
onSuccessand error handling workflows:
onSuccesscallback is verified to fire after restoration- Error paths correctly trigger
removeClientandonError- Timer advances account for both restoration and callback delays
607-797: LGTM!The multiple-client and StrictMode tests are well-structured:
- Multiple client scenario correctly verifies that only the second client's
queryFnis called after hydration- StrictMode test validates single restoration despite React 19's double-mounting behavior
- Both tests properly use fake timers to drive the async flows
β¦o fake timers, replace 'await' with 'advanceTimersByTimeAsync', and use 'sleep().then()' pattern
sukvvon
force-pushed
the
test/react-query-persist-client-persist-query-client-provider-fake-timers
branch
from
810b079 to
6d43ba3
Compare
1 participant
π― Changes
β Checklist
pnpm run test:pr.π Release Impact
Summary by CodeRabbit