test(svelte-query-persist-client/PersistQueryClientProvider): switch to fake timers, replace 'waitFor' with 'advanceTimersByTimeAsync', and use 'sleep().then()' pattern #9900
Conversation
…to fake timers, replace 'waitFor' with 'advanceTimersByTimeAsync', and use 'sleep().then()' pattern
|
WalkthroughTest files in packages/svelte-query-persist-client were modified to add artificial 10ms delays in query functions (via Changes
Sequence Diagram(s)sequenceDiagram
%%{init: {"themeVariables":{"actorBorder":"#2b6cb0","actorBackground":"#ebf8ff","noteBackground":"#f7fafc"}}}%%
participant Test as Test Runner
participant Persister as Persister Mock
participant QueryClient as Query Client
participant Component as Svelte Component
rect rgba(43,108,176,0.08)
Note over Test,Component: Old flow (waitFor)
Test->>Component: render
Component->>QueryClient: prefetch/query (immediate Promise.resolve)
QueryClient-->>Component: data resolved
Test->>Test: waitFor(assert DOM state)
end
rect rgba(72,187,120,0.08)
Note over Test,Component: New flow (fake timers + sleep)
Test->>Test: useFakeTimers()
Test->>Component: render
Component->>QueryClient: prefetch/query (sleep(10).then(...))
Persister->>Test: restoreClient resolves after delay
Test->>Test: advanceTimersByTimeAsync(10)
QueryClient-->>Component: data resolved (after timer)
Test->>Test: assert DOM state (hydrated -> fetched)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
View your CI Pipeline Execution ↗ for commit 163797c
☁️ Nx Cloud last updated this comment at |
![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 (4)
packages/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.ts (4)
44-60: Minor readability tweak for error persister rejection
createMockErrorPersister.restoreClientcurrently doesreturn sleep(10).then(() => Promise.reject(error)). Thanks to async/thenable assimilation this works, but it’s a bit indirect; consider throwing inside thethenfor clarity:- async restoreClient() { - return sleep(10).then(() => Promise.reject(error)) - }, + async restoreClient() { + return sleep(10).then(() => { + throw error + }) + },Functionally equivalent, but makes the “reject after 10ms” intent more obvious.
75-76: Optional: simplify persist flow by awaitingpersistQueryClientSaveAcross multiple tests you do:
persistQueryClientSave({ queryClient, persister }) await vi.advanceTimersByTimeAsync(0)Given the current
persistQueryClientSaveimplementation and a synchronouspersistClient, you could optionally make this a bit clearer as:await persistQueryClientSave({ queryClient, persister })which avoids implying there are timer‑driven side effects in the persist path. Purely stylistic; behavior is fine as is.
Also applies to: 127-128, 179-180, 231-232, 287-288, 320-321
219-275: Use async/await for the side‑effectfulqueryFnin the fresh‑data testThe
setQueryDefaultsqueryFnboth waits and mutatesfetched:queryClient.setQueryDefaults(['test'], { queryFn: () => sleep(10).then(() => { fetched = true return 'fetched' }), })The logic is correct, but since this
queryFnhas side effects (togglingfetched),async/awaitwould be clearer, e.g.:queryClient.setQueryDefaults(['test'], { queryFn: async () => { await sleep(10) fetched = true return 'fetched' }, })This keeps the “only set
fetchedif the queryFn actually runs” semantics while matching the preferred style for side‑effectful queryFns. Based on learnings, this aligns with how similar tests are written elsewhere.
277-308: Optional: assertonSuccessis not called again on refetchThe
onSuccesstest correctly verifies that:
onSuccessis0before timers advance,- it’s
1after the 10ms restore/hydrate step.Since you also advance timers another 10ms to reach the
'fetched'state, you could optionally tighten the contract by asserting the call count again after the second advance:await vi.advanceTimersByTimeAsync(10) expect(rendered.getByText('hydrated')).toBeInTheDocument() expect(onSuccess).toHaveBeenCalledTimes(1) await vi.advanceTimersByTimeAsync(10) expect(rendered.getByText('fetched')).toBeInTheDocument() expect(onSuccess).toHaveBeenCalledTimes(1)This would pin down that
PersistQueryClientProvideronly firesonSuccessfor restore, not for subsequent refetches. Current test is still functionally valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/svelte-query-persist-client/tests/AwaitOnSuccess/AwaitOnSuccess.svelte(1 hunks)packages/svelte-query-persist-client/tests/FreshData/FreshData.svelte(2 hunks)packages/svelte-query-persist-client/tests/InitialData/InitialData.svelte(1 hunks)packages/svelte-query-persist-client/tests/OnSuccess/OnSuccess.svelte(1 hunks)packages/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.ts(16 hunks)packages/svelte-query-persist-client/tests/RemoveCache/RemoveCache.svelte(1 hunks)packages/svelte-query-persist-client/tests/RestoreCache/RestoreCache.svelte(1 hunks)packages/svelte-query-persist-client/tests/UseQueries/UseQueries.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
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-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/svelte-query-persist-client/tests/UseQueries/UseQueries.sveltepackages/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.tspackages/svelte-query-persist-client/tests/OnSuccess/OnSuccess.sveltepackages/svelte-query-persist-client/tests/RestoreCache/RestoreCache.sveltepackages/svelte-query-persist-client/tests/AwaitOnSuccess/AwaitOnSuccess.sveltepackages/svelte-query-persist-client/tests/InitialData/InitialData.sveltepackages/svelte-query-persist-client/tests/RemoveCache/RemoveCache.sveltepackages/svelte-query-persist-client/tests/FreshData/FreshData.svelte
📚 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/svelte-query-persist-client/tests/UseQueries/UseQueries.sveltepackages/svelte-query-persist-client/tests/OnSuccess/OnSuccess.sveltepackages/svelte-query-persist-client/tests/RestoreCache/RestoreCache.sveltepackages/svelte-query-persist-client/tests/RemoveCache/RemoveCache.svelte
🧬 Code graph analysis (1)
packages/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.ts (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 (13)
packages/svelte-query-persist-client/tests/InitialData/InitialData.svelte (1)
11-11: LGTM! Follows established pattern for queryFn without side effects.The refactor from async/await to the
.then()pattern is appropriate here since the queryFn has no side effects—it only sleeps and returns a value. The 10ms delay standardization aligns with the PR's fake timer strategy.Based on learnings, when a queryFn contains no side effects, prefer the
.then()pattern for conciseness.packages/svelte-query-persist-client/tests/UseQueries/UseQueries.svelte (1)
3-3: LGTM! Consistent with the sleep().then() pattern for queryFn without side effects.The import of
sleepand the use ofsleep(10).then(() => 'fetched')align with the PR's objective to introduce artificial delays for fake timer control. The.then()pattern is appropriate here since the queryFn has no side effects.Based on learnings, when queryFn contains no side effects, the
.then()pattern is preferred for conciseness.Also applies to: 13-13
packages/svelte-query-persist-client/tests/AwaitOnSuccess/AwaitOnSuccess.svelte (1)
12-12: LGTM! Standardized delay duration aligns with other tests.Increasing the sleep duration from 5 to 10ms standardizes the timing across all test files in this PR. The async/await syntax is appropriate here since the queryFn contains side effects (state tracking).
Based on learnings, when queryFn contains side effects, async/await syntax is preferred for clarity.
packages/svelte-query-persist-client/tests/OnSuccess/OnSuccess.svelte (1)
3-3: LGTM! Consistent with the sleep().then() pattern for queryFn without side effects.The import of
sleepand the use ofsleep(10).then(() => 'fetched')align with the PR's objective to introduce artificial delays for fake timer control. The.then()pattern is appropriate here since the queryFn has no side effects.Based on learnings, when queryFn contains no side effects, the
.then()pattern is preferred for conciseness.Also applies to: 7-7
packages/svelte-query-persist-client/tests/RemoveCache/RemoveCache.svelte (1)
3-3: LGTM! Consistent with the sleep().then() pattern for queryFn without side effects.The import of
sleepand the use ofsleep(10).then(() => 'fetched')align with the PR's objective to introduce artificial delays for fake timer control. The.then()pattern is appropriate here since the queryFn has no side effects.Based on learnings, when queryFn contains no side effects, the
.then()pattern is preferred for conciseness.Also applies to: 7-7
packages/svelte-query-persist-client/tests/RestoreCache/RestoreCache.svelte (1)
3-3: LGTM! Consistent with the sleep().then() pattern for queryFn without side effects.The import of
sleepand the refactoring from an async function tosleep(10).then(() => 'fetched')align with the PR's objective to introduce artificial delays for fake timer control. The.then()pattern is appropriate here since the queryFn has no side effects.Based on learnings, when queryFn contains no side effects, the
.then()pattern is preferred for conciseness.Also applies to: 11-11
packages/svelte-query-persist-client/tests/FreshData/FreshData.svelte (2)
24-24: Display change already validated by test expectations.The test at line 254 of
PersistQueryClientProvider.svelte.test.tsalready expectsgetByText('data: null'), confirming the display change is correct and properly tested. No further action needed.
11-15: Based on my verification of the shell script results, I can now provide a definitive response:The queryFn setup is correct and properly externalized.
The shell script output confirms that
queryClient.setQueryDefaults(['test'], { queryFn: ... })is called in the test file (PersistQueryClientProvider.svelte.test.ts at line 238), which matches the queryKey['test']used in FreshData.svelte at line 12. The queryFn specified in setQueryDefaults will be used by queries with matching queryKey.The comment at line 13 in FreshData.svelte accurately describes the current implementation. No issues were found with the queryFn externalization pattern.
packages/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.ts (5)
1-2: Fake timers setup matches the new timer‑driven test styleUsing
vi.useFakeTimers()inbeforeEachandvi.useRealTimers()inafterEach, combined withvi.advanceTimersByTimeAsync, is a clean way to make these tests deterministic; no issues spotted here.Also applies to: 20-26
28-42: Mock persister restore delay is consistent with core utils
createMockPersister.restoreClientdelaying viasleep(10).then(() => storedState)mirrors the behavior of the corecreateMockPersisterhelper while fitting the.then()style for a side‑effect‑free path; this looks correct and plays nicely with fake timers.
62-113: Restore/hydrate/refetch timelines look correct and deterministicFor the first three tests, the pattern of:
- prefetching with a
sleep(10)‑basedqueryFn,- saving via
persistQueryClientSave,- restoring through a persister that resolves after 10ms, and
- advancing timers in 10ms steps to assert
idle → hydrated → fetchedand the recordedstatesis internally consistent and should remove the flakiness risk from the old
waitFor‑based assertions. I don’t see functional issues here.Also applies to: 115-165, 167-217
310-351: Await‑onSuccess timing test matches the intended sequencingUsing
await vi.advanceTimersByTimeAsync(15)to cover 10ms for restore plus 5ms for the awaitedonSuccesscallback, followed by another 10ms to cover the subsequent refetch, matches the expectedstatessequence:
onSuccessonSuccess donefetchingfetchedThis gives good coverage that
onSuccessis awaited before normal fetching resumes; no issues here.
353-382: Error‑path test for failed restore and cache removal looks solidDriving the failure path with the error persister and two 10ms timer advances (restore failure + fresh fetch) and then asserting:
- UI in
'fetched'state,removeClientcalled once,onErrorcalled once andonSuccessnever called,console.errorcalled with the error,gives comprehensive coverage of the non‑successful restore behavior under fake timers; this looks correct and robust.
| return 'fetched' | ||
| }, | ||
| queryFn: () => sleep(10).then(() => 'fetched'), |
There was a problem hiding this comment.
why is this better than await ?
There was a problem hiding this comment.
In my opinion, sleep().then() is more concise than async/await. There's no functional difference, but it avoids the async keyword and extra braces, and makes the timer-to-callback dependency explicit in a single line. I discussed this with @manudeli.
// await: 4 lines
async () => {
await sleep(10)
return 'fetched'
}
// then: 1 line
() => sleep(10).then(() => 'fetched')There was a problem hiding this comment.
okay but we have over 350 instances of await sleep(...) and I think it’s unnecessary to refactor them “just because”. This is also only true if you have a single await statement, we also sometimes have:
mutationFn: async () => {
results.push('start-A')
await sleep(10)
results.push('finish-A')
return 'a'
}
which is much more readable with async/await.
If it’s all the same, I’d like to keep it as-is.
There was a problem hiding this comment.
So the .then() ones from the fake timers PRs that already got merged stay put, use .then() for single-statement cases as I suggested, and keep await otherwise from now on?
packages/svelte-query-persist-client/tests/FreshData/FreshData.svelte
Outdated
Show resolved
Hide resolved
… 'setQueryDefaults' with 'onFetch' callback in FreshData
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.ts (1)
28-60: Persister mocks now match delayed restore semanticsSwitching
restoreClientin bothcreateMockPersisterandcreateMockErrorPersistertosleep(10).then(...)nicely aligns the mocks with the new timer-based tests and makes restore behavior explicitly async.Minor readability nit: for the error case you could slightly simplify the rejection:
- async restoreClient() { - return sleep(10).then(() => Promise.reject(error)) - }, + async restoreClient() { + return sleep(10).then(() => { + throw error + }) + },Functionally equivalent, but a bit easier to scan.
Based on learnings, the use of
.thenhere (no side effects) is a good fit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/svelte-query-persist-client/tests/FreshData/FreshData.svelte(2 hunks)packages/svelte-query-persist-client/tests/FreshData/Provider.svelte(1 hunks)packages/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.ts(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/svelte-query-persist-client/tests/FreshData/FreshData.svelte
🧰 Additional context used
🧠 Learnings (2)
📓 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/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.ts
⏰ 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 (10)
packages/svelte-query-persist-client/tests/FreshData/Provider.svelte (2)
15-19: LGTM! Prop threading is correctly implemented.The
onFetchprop is properly destructured from props and forwarded to theFreshDatacomponent using idiomatic Svelte shorthand syntax.
12-12: I'll help you verify the review comment about theonFetchprop. Let me search for how this Provider component is used throughout the test suite.
<function_calls>
<invoke_name>shell
<invoke_command>#!/bin/bashFind all files that import or use the Provider component from FreshData/Provider
echo "=== Searching for Provider imports ==="
rg -n "from.*FreshData/Provider|import.*Provider.*FreshData" packages/svelte-query-persist-client/tests/echo -e "\n=== Searching for Provider component usage (opening tags) ==="
rg -n "<Provider" packages/svelte-query-persist-client/tests/ -A 5 -B 2
</invoke_command>
</invoke_function_calls>Let me also view the Provider component file to understand the context better:
<function_calls>
<invoke_name>shell
<invoke_command>#!/bin/bashView the Provider component file to see the full context
cat -n packages/svelte-query-persist-client/tests/FreshData/Provider.svelte
</invoke_command>
</function_calls>Let me check for the usage patterns more thoroughly:
<function_calls>
<invoke_name>shell
<invoke_command>#!/bin/bashSearch for all test files in the FreshData directory
echo "=== Test files in FreshData directory ==="
find packages/svelte-query-persist-client/tests/FreshData -type f -name ".svelte" -o -name ".ts" -o -name "*.tsx" | head -20echo -e "\n=== Content of test files that might use Provider ==="
find packages/svelte-query-persist-client/tests/FreshData -type f ( -name ".test." -o -name ".spec." ) | head -10
</invoke_command>
</function_calls>Let me search more broadly for where Provider is used:
<function_calls>
<invoke_name>shell
<invoke_command>#!/bin/bashSearch for any file mentioning onFetch in the test directory
echo "=== Files mentioning onFetch ==="
rg -n "onFetch" packages/svelte-query-persist-client/tests/ -B 3 -A 3echo -e "\n=== All Svelte files in tests/FreshData ==="
Let me check all test files more thoroughly: shell #!/bin/bash # List all files in the tests directory structure echo "=== Directory structure of tests ===" find packages/svelte-query-persist-client/tests -type f | sort
find packages/svelte-query-persist-client/tests/FreshData -name "*.svelte" -type f
</invoke_command>
</function_calls>echo -e "\n=== Search for FreshData references ==="
rg -n "FreshData" packages/svelte-query-persist-client/tests/ -B 2 -A 2
</invoke_command>
</function_calls>packages/svelte-query-persist-client/tests/PersistQueryClientProvider.svelte.test.ts (8)
1-2: Consistent global fake‑timer setupImporting
viand enabling fake timers inbeforeEachwithvi.useRealTimers()inafterEachgives you deterministic control over all thesleep/timer-driven flows in this suite without leaking fake timers across tests. This setup looks correct and symmetric.If you haven’t already, keep an eye out for any tests in this file that might implicitly rely on real time (e.g.
Date.now()), but it sounds likepnpm run test:prhas already validated this.Also applies to: 20-26
67-72: Clear, phase‑by‑phase restore/hydrate/fetch flowThe first test’s sequence—prefetch with
sleep(10), save, clear, then assertingidle → hydrated → fetchedwhile advancing timers by10 + 10—makes the restore pipeline very explicit and deterministic. The intermediate DOM assertions and thestates.currentshape all line up with the expected phases.Also applies to: 75-77, 88-92
119-124: Deterministic idle → hydrated → fetched foruseQueriesMirroring the single-query test for the
UseQueriescase with the samesleep(10)prefetch and explicitadvanceTimersByTimeAsyncsteps gives a nice, symmetric check that the multi-query path also goes through idle, hydrated, and fetched in order. The expectations and timer advancements look consistent.Also applies to: 127-128, 140-144
171-176: Good coverage ofinitialDataduring restoreThis test now clearly verifies that
initialis rendered while restore is in progress, then transitions tohydratedand finallyfetchedwith the two 10ms timer advances. The timing and expectedstates.currenttransitions are coherent with the persister’s 10ms delay and the subsequent refetch.Also applies to: 179-181, 192-196
223-227:onFetchcheck cleanly proves “no refetch when fresh”Using a local
fetchedflag viaonFetchtogether with the three phase assertions (data: null→data: hydrated→data: hydrated) and only two 10ms advances is a tight way to show that no second network fetch is triggered for fresh data. The finalexpect(fetched).toBe(false)plus the two-entrystates.currentarray make the intent very clear.Also applies to: 231-233, 236-237, 243-245, 249-253, 257-257
274-279: SynchronousonSuccesstiming verified with timersFor the simple
onSuccesscase, asserting it’s not called before the first 10ms advance, then called exactly once whenhydratedappears, and leaving the second 10ms solely to drive the “fetched” UI gives good confidence about callback ordering relative to restore and refetch. The timer increments are well matched to the mocked delays.Also applies to: 282-284, 298-302
307-312: AsynconSuccessflow and state ordering look correctThe async
onSuccessimplementation that pushes'onSuccess', awaitssleep(5), then pushes'onSuccess done', combined withadvanceTimersByTimeAsync(15)followed by another 10ms, nicely validates that:
- restore completes and
onSuccessfully resolves before the extra'fetching'/'fetched'states, and- the final
states.currentordering matches the expected'onSuccess','onSuccess done','fetching','fetched'.Using
async/awaithere is appropriate given the side effects onstates.current(and matches the prior learning).Also applies to: 315-317, 335-338
367-369: Error restore path correctly driven by delayed rejectionAdvancing timers by 10ms twice to account for the delayed
restoreClientrejection and any follow-up timers, then asserting the'fetched'UI plusremoveClient/onErrorcalls, gives a solid, deterministic check of the error path. Stubbingconsole.error/console.warnsolely to silence logs is fine here, since you already assert against the error log count.
2 participants
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.