test(react-query-persist-client/PersistQueryClientProvider): convert complex 'then' callback to async/await and add delayed assertion for refetch verification #9901
Conversation
β¦'await sleep' with 'sleep().then()' pattern in restoreClient and add additional timer advance for refetch verification
|
WalkthroughTest updates in Changes
Estimated code review effortπ― 2 (Simple) | β±οΈ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touchesβ Failed checks (1 warning)
β Passed checks (2 passed)
β¨ Finishing touches
π§ͺ Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution β for commit fb4071e
βοΈ 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 #9901 +/- ##
============================================
+ Coverage 45.89% 100.00% +54.10%
============================================
Files 200 1 -199
Lines 8437 18 -8419
Branches 1943 2 -1941
============================================
- Hits 3872 18 -3854
+ Misses 4116 0 -4116
+ Partials 449 0 -449 π New features to boost your workflow:
|
| await sleep(10) | ||
| throw error | ||
| return sleep(10).then(() => { | ||
| throw error | ||
| }) |
There was a problem hiding this comment.
same question: why? are there any functional difference ?
There was a problem hiding this comment.
@TkDodo If we apply the pattern from #9900 (comment), we could write it like this:
return sleep(10).then(() => Promise.reject(error))What do you think about this approach?
β¦ convert complex 'then' callbacks to async/await pattern
sukvvon
changed the title
![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 (1)
packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (1)
43-46: Consider .then() pattern for consistency.For consistency with the established pattern (per learnings: prefer
.then()when there are no side effects), consider refactoring this to:async restoreClient() { - await sleep(10) - throw error + return sleep(10).then(() => Promise.reject(error)) },This matches the suggestion from the past review comments and aligns with the
.then()pattern used increateMockPersister(line 25).
π Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx(2 hunks)
π§° 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/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
β° 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)
- GitHub Check: Test
- GitHub Check: Preview
π Additional comments (1)
packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx (1)
417-418: LGTM! Improved test coverage.The additional timer advance verifies that the hydration state remains stable and no refetch occurs when data is fresh, which strengthens the test assertions.
| queryFn: async () => { | ||
| await sleep(10) | ||
| fetched = true | ||
| return 'fetched' | ||
| }, |
There was a problem hiding this comment.
Inconsistent PR description, but changes align with learnings.
The PR title and description claim to "replace 'await sleep' with 'sleep().then()' pattern in restoreClient", but the actual change here is in a queryFn and goes in the opposite direction (from .then() to async/await).
However, the change itself is appropriate: the queryFn has a side effect (fetched = true), so async/await syntax is preferred for clarity.
Based on learnings, please update the PR title and description to accurately reflect the actual changes.
π€ Prompt for AI Agents
In
packages/react-query-persist-client/src/__tests__/PersistQueryClientProvider.test.tsx
around lines 387 to 391, the PR title/description incorrectly state "replace
'await sleep' with 'sleep().then()' pattern in restoreClient" while the actual
change in this test replaces a .then() style sleep with an async/await queryFn
(which is correct due to the side effect). Update the PR title and description
to accurately describe the change (e.g., "use async/await in queryFn with side
effect in PersistQueryClientProvider tests" or similar) and remove any
references to modifying restoreClient or switching to .then() so they match the
code changes.
2 participants
π― Changes
β Checklist
pnpm run test:pr.π Release Impact
Summary by CodeRabbit
βοΈ Tip: You can customize this high-level summary in your review settings.