test(solid-query-persist-client/PersistQueryClientProvider): switch to fake timers, replace 'waitFor' with 'advanceTimersByTimeAsync', and use 'sleep().then()' pattern #9892
Conversation
|
WalkthroughRefactors the PersistQueryClientProvider test to use fake timers (vi.useFakeTimers/useRealTimers), replacing Promise.resolve-based delays and waitFor calls with sleep-based delays plus explicit advanceTimersByTimeAsync calls; adjusts mocked persister/restore behavior accordingly. Changes
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touchesβ Failed checks (2 warnings)
β Passed checks (1 passed)
β¨ Finishing touches
π§ͺ Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution β for commit c9e6076
βοΈ 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 #9892 +/- ##
============================================
+ Coverage 45.70% 100.00% +54.29%
============================================
Files 200 1 -199
Lines 8437 20 -8417
Branches 1936 2 -1934
============================================
- Hits 3856 20 -3836
+ Misses 4133 0 -4133
+ 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: 1
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx(19 hunks)
π§° Additional context used
π§ Learnings (3)
π Common learnings
Learnt from: justjake
Repo: TanStack/query PR: 9612
File: packages/query-core/src/__tests__/timeoutManager.test.tsx:37-57
Timestamp: 2025-09-04T16:36:04.575Z
Learning: In Node.js v12.19+ (and browsers), converting a Timeout object to a number (e.g., Number(timeoutId)) is valid because Timeout implements Symbol.toPrimitive; clearTimeout/clearInterval accept the object or its numeric primitive. Do not flag this pattern in tests like packages/query-core/src/__tests__/timeoutManager.test.tsx.
π Learning: 2025-09-02T17:57:33.184Z
Learnt from: TkDodo
Repo: TanStack/query PR: 9612
File: packages/query-async-storage-persister/src/asyncThrottle.ts:0-0
Timestamp: 2025-09-02T17:57:33.184Z
Learning: When importing from tanstack/query-core in other TanStack Query packages like query-async-storage-persister, a workspace dependency "tanstack/query-core": "workspace:*" needs to be added to the package.json.
Applied to files:
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
π Learning: 2025-08-19T03:18:18.303Z
Learnt from: oscartbeaumont
Repo: TanStack/query PR: 9564
File: packages/solid-query-devtools/src/production.tsx:2-3
Timestamp: 2025-08-19T03:18:18.303Z
Learning: In the solid-query-devtools package, the codebase uses a pattern of type-only default imports combined with typeof for component type annotations (e.g., `import type SolidQueryDevtoolsComp from './devtools'` followed by `typeof SolidQueryDevtoolsComp`). This pattern is consistently used across index.tsx and production.tsx files, and the maintainers prefer consistency over changing this approach.
Applied to files:
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
𧬠Code graph analysis (1)
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (2)
packages/query-persist-client-core/src/__tests__/utils.ts (2)
createMockPersister(5-20)removeClient(16-18)packages/query-persist-client-core/src/persist.ts (1)
persistQueryClientSave(114-127)
π Additional comments (8)
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (8)
49-55: LGTM! Proper fake timer lifecycle setup.The beforeEach/afterEach hooks correctly initialize and clean up fake timers for all tests in the suite.
20-21: LGTM! Consistent with the sleep().then() pattern.The refactor from async/await to promise chaining aligns with the PR objectives and works correctly with fake timers.
57-134: LGTM! Well-structured test with deterministic timing.The test correctly uses fake timers with the sleep().then() pattern throughout, and timer advancements properly match the sleep durations (10ms for async operations, 0ms to flush microtasks for persist).
136-218: LGTM! Consistent pattern for useQueries.The test properly validates useQueries behavior with the same fake timer patterns used throughout the suite.
220-302: LGTM! Correct validation of initialData during restoration.The test properly validates that initialData is displayed while the restore operation is in progress, with correct fake timer usage.
386-435: LGTM! Proper validation of onSuccess callback timing.The test correctly validates that onSuccess is called after restoration completes, with consistent use of fake timers and the sleep().then() pattern.
437-485: LGTM! Error handling test with correct timer usage.The test properly validates error handling during restoration, including removal of corrupted cache, with consistent use of fake timers.
487-599: LGTM! Multi-client persistence test with proper mock setup.The test correctly validates persistence across multiple clients with vi.fn().mockImplementation() using the sleep().then() pattern, and proper timer advancement to control execution flow.
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
Outdated
Show resolved
Hide resolved
β¦o fake timers, replace 'waitFor' with 'advanceTimersByTimeAsync', and use 'sleep().then()' pattern
sukvvon
force-pushed
the
test/solid-query-persist-client-persist-query-client-provider-fake-timers
branch
from
22509e6 to
c9e6076
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
π§Ή Nitpick comments (1)
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (1)
331-335: Inconsistent with established pattern for side effects.Based on learnings, when a queryFn contains side effects (like setting the
fetchedflag), the preferred pattern is async/await for clarity rather than.then(). This makes side effects more explicit and easier to understand.Apply this diff to align with the established pattern:
- const state = useQuery(() => ({ - queryKey: key, - queryFn: () => - sleep(10).then(() => { - fetched = true - return 'fetched' - }), - staleTime: Infinity, - })) + const state = useQuery(() => ({ + queryKey: key, + queryFn: async () => { + await sleep(10) + fetched = true + return 'fetched' + }, + staleTime: Infinity, + }))Based on learnings, this pattern makes the side effect more visible and easier to verify.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx(19 hunks)
π§° Additional context used
π§ Learnings (4)
π Common learnings
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
π Learning: 2025-11-22T09:06:05.219Z
Learnt from: sukvvon
Repo: TanStack/query PR: 9892
File: packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx:331-335
Timestamp: 2025-11-22T09:06:05.219Z
Learning: In TanStack/query test files, when a queryFn contains side effects (e.g., setting flags for test verification), prefer async/await syntax for clarity; when there are no side effects, prefer the .then() pattern for conciseness.
Applied to files:
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
π Learning: 2025-08-19T03:18:18.303Z
Learnt from: oscartbeaumont
Repo: TanStack/query PR: 9564
File: packages/solid-query-devtools/src/production.tsx:2-3
Timestamp: 2025-08-19T03:18:18.303Z
Learning: In the solid-query-devtools package, the codebase uses a pattern of type-only default imports combined with typeof for component type annotations (e.g., `import type SolidQueryDevtoolsComp from './devtools'` followed by `typeof SolidQueryDevtoolsComp`). This pattern is consistently used across index.tsx and production.tsx files, and the maintainers prefer consistency over changing this approach.
Applied to files:
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
π Learning: 2025-09-04T16:36:04.575Z
Learnt from: justjake
Repo: TanStack/query PR: 9612
File: packages/query-core/src/__tests__/timeoutManager.test.tsx:37-57
Timestamp: 2025-09-04T16:36:04.575Z
Learning: In Node.js v12.19+ (and browsers), converting a Timeout object to a number (e.g., Number(timeoutId)) is valid because Timeout implements Symbol.toPrimitive; clearTimeout/clearInterval accept the object or its numeric primitive. Do not flag this pattern in tests like packages/query-core/src/__tests__/timeoutManager.test.tsx.
Applied to files:
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
𧬠Code graph analysis (1)
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (3)
packages/query-persist-client-core/src/__tests__/utils.ts (2)
createMockPersister(5-20)removeClient(16-18)packages/query-persist-client-core/src/persist.ts (1)
persistQueryClientSave(114-127)packages/solid-query/src/useQuery.ts (1)
useQuery(36-50)
β° 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). (1)
- GitHub Check: Test
π Additional comments (5)
packages/solid-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (5)
49-55: LGTM! Fake timer setup is correct.The beforeEach/afterEach hooks properly set up and tear down fake timers for all tests.
20-22: LGTM! Persister pattern aligns with fake timers.The change from async/await to the
.then()pattern works correctly with fake timers and matches the PR objectives.
66-113: LGTM! Timer-driven test flow is correct.The test properly uses fake timers with explicit timer advancement at each async boundary. The replacement of
waitForwith direct assertions followed byadvanceTimersByTimeAsyncmakes the test more deterministic.
372-384: Verify the change in expected state count is intentional.The expected number of states changed from 3 to 2. With fake timers providing deterministic timing, this appears correct: when
staleTime: Infinityprevents a refetch, we should see only the initial pending state and the hydrated success state, without an intermediate fetching state.Please confirm this behavioral change is expected with fake timers and not a regression.
512-517: LGTM! Mock implementations use correct pattern.The mock queryFn implementations properly use the
sleep().then()pattern without side effects, which aligns with the established convention and works correctly with fake timers.
1 participant
π― Changes
β Checklist
pnpm run test:pr.π Release Impact
Summary by CodeRabbit
Summary by CodeRabbit
βοΈ Tip: You can customize this high-level summary in your review settings.