test(angular-query-persist-client/withPersistQueryClient): switch to fake timers, replace 'waitFor' with 'advanceTimersByTimeAsync', use 'sleep().then()' pattern, replace 'screen' with 'rendered', add 'toBeInTheDocument', and add '@testing-library/jest-dom/vitest' #9895
Conversation
|
WalkthroughTests were refactored to use Vitest fake timers with explicit timer advancement; mock persister and prefetch query functions now resolve via 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)
π Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro π Files selected for processing (1)
π§° Additional context usedπ§ Learnings (3)π Common learningsπ Learning: 2025-11-22T09:06:05.219ZApplied to files:
π Learning: 2025-09-21T00:31:02.518ZApplied to files:
𧬠Code graph analysis (1)packages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts (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 (8)
Comment |
|
View your CI Pipeline Execution β for commit f432d8c
βοΈ Nx Cloud last updated this comment at |
sukvvon
force-pushed
the
test/angular-query-persist-client-with-persist-query-client-fake-timers
branch
from
2e93bb9 to
652e89a
Compare
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9895 +/- ##
============================================
+ Coverage 45.70% 100.00% +54.29%
============================================
Files 200 1 -199
Lines 8437 19 -8418
Branches 1936 1 -1935
============================================
- Hits 3856 19 -3837
+ Misses 4133 0 -4133
+ Partials 448 0 -448 π New features to boost your workflow:
|
sukvvon
force-pushed
the
test/angular-query-persist-client-with-persist-query-client-fake-timers
branch
from
652e89a to
9b92bc7
Compare
sukvvon
force-pushed
the
test/angular-query-persist-client-with-persist-query-client-fake-timers
branch
from
9b92bc7 to
fa348ff
Compare
![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 (2)
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts (2)
75-80: Prefetch + persist pattern with fake timers is logically sound; consider awaitingpersistQueryClientSaveThe sequence β
prefetchQuerywithsleep(10),await vi.advanceTimersByTimeAsync(10), thenpersistQueryClientSave(...); await vi.advanceTimersByTimeAsync(0)β correctly ensures the hydrated state is in the cache before persisting, and the fake timers drive allsleepcalls deterministically. Given the current mock persister is synchronous, behavior is correct.To avoid potential floatingβpromise lint warnings and make future async persister changes safer, you might optionally make the persist step explicit:
await persistQueryClientSave({ queryClient, persister }) await vi.advanceTimersByTimeAsync(0)This doesnβt change semantics today but clarifies intent.
If you have ESLintβs noβfloatingβpromises (or similar) enabled elsewhere in the repo, itβs worth checking whether this file triggers any warnings with the current pattern.
Also applies to: 83-85, 163-168, 171-173, 251-256, 259-261, 328-333, 335-337
110-110: Timerβdriven DOM assertions +detectChangespattern is consistent; consider a tiny helperAcross the tests you follow a clear pattern:
const rendered = await render(Page, { ... })await vi.advanceTimersByTimeAsync(N)rendered.fixture.detectChanges()- DOM assertions using
rendered.getByText(...).toBeInTheDocument()This is exactly what you want with zoneless CD and fake timers, and it nicely replaces the older
waitFor+toBeTruthystyle.If you find yourself adding more such tests, you might consider a small helper like
await advanceAndDetect(rendered, ms)to reduce repetition and make it harder to forget thedetectChanges()call.Please verify that there are no remaining assertions in this file still using
toBeTruthyfor DOM presence now that@testing-library/jest-dom/vitestis wired up.Also applies to: 120-123, 125-127, 202-202, 212-215, 217-219, 293-293, 303-306, 357-357, 371-373, 407-407, 421-422, 427-429
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (2)
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts(19 hunks)packages/angular-query-persist-client/test-setup.ts(1 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: arnoud-dv
Repo: TanStack/query PR: 9669
File: docs/framework/angular/guides/testing.md:49-56
Timestamp: 2025-09-21T00:31:02.518Z
Learning: TestBed.tick() is a valid API introduced in Angular 20 for triggering effects in unit tests, similar to ApplicationRef.tick() but for testing contexts.
π Learning: 2025-09-21T00:31:02.518Z
Learnt from: arnoud-dv
Repo: TanStack/query PR: 9669
File: docs/framework/angular/guides/testing.md:49-56
Timestamp: 2025-09-21T00:31:02.518Z
Learning: TestBed.tick() is a valid API introduced in Angular 20 for triggering effects in unit tests, similar to ApplicationRef.tick() but for testing contexts.
Applied to files:
packages/angular-query-persist-client/test-setup.tspackages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts
π 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/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts
𧬠Code graph analysis (1)
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.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 (3)
packages/angular-query-persist-client/test-setup.ts (1)
1-1: JestβDOM matcher setup looks correctSideβeffect import of
@testing-library/jest-dom/vitestin the shared test setup is the right way to enable matchers liketoBeInTheDocumentfor these Angular/Vitest tests; no further changes needed here.Please doubleβcheck that other package test setups in this repo use the same entry point so matcher behavior stays consistent across packages.
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts (2)
1-1: Fakeβtimer + render setup is clean and wellβscopedUsing
beforeEach(vi.useFakeTimers)/afterEach(vi.useRealTimers)together withrenderfrom@testing-library/angularkeeps timer control localized per test and avoids crossβtest leakage. This fits well withprovideZonelessChangeDetection()and the rest of the Angular testing stack.Please just confirm there are no other tests in this package that rely on real timers implicitly (e.g.
setTimeoutwithoutadvanceTimersByTimeAsync) so this global fakeβtimer setup doesnβt introduce surprises.Also applies to: 13-13, 21-27
275-283: Sideβeffecting queryFn correctly uses async/await, matching repo testing conventionsIn the βshould not refetch after restoring when data is freshβ test, the
queryFnthat flips thefetchedflag usesasync/awaitaroundsleep(10), while other pure queryFns use thesleep(10).then(...)pattern. This aligns with the prior guidance to prefer async/await when there are side effects and.thenwhen there arenβt, and thefetchedflag plusstatesexpectations give a clear signal that no refetch occurred for fresh data.Based on learnings, this pattern matches similar tests in other persistβclient packages; itβs still worth running the full Angular test suite to ensure no subtle behavior differences slipped in with the fakeβtimer migration.
Also applies to: 310-322
β¦fake timers, replace 'waitFor' with 'advanceTimersByTimeAsync', use 'sleep().then()' pattern, replace 'screen' with 'rendered', add 'toBeInTheDocument', and add '@testing-library/jest-dom/vitest'
sukvvon
force-pushed
the
test/angular-query-persist-client-with-persist-query-client-fake-timers
branch
from
fa348ff to
914e012
Compare
sukvvon
changed the title
β¦sync/await' with 'sleep().then()' pattern in queryFn and restoreClient
There was a problem hiding this comment.
Actionable comments posted: 0
π§Ή Nitpick comments (2)
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts (2)
75-79: Verify if zero-millisecond timer advancement is necessary.The prefetch pattern correctly uses sleep().then() and advances timers by 10ms. However, the
advanceTimersByTimeAsync(0)afterpersistQueryClientSave(line 84) appears to flush microtasks. Consider verifying whether this is required for the test to pass or if it can be removed.Also applies to: 83-84
277-281: Prefer async/await syntax for queryFn with side effects.The queryFn uses the .then() pattern but contains a side effect (setting the
fetchedflag). For clarity, prefer async/await syntax when side effects are present.Based on learnings, when a queryFn contains side effects like setting flags for test verification, async/await syntax is preferred for clarity over the .then() pattern.
Apply this diff to improve clarity:
- queryFn: () => - sleep(10).then(() => { - fetched = true - return 'fetched' - }), + queryFn: async () => { + await sleep(10) + fetched = true + return 'fetched' + },
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (1)
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts(21 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: arnoud-dv
Repo: TanStack/query PR: 9669
File: docs/framework/angular/guides/testing.md:49-56
Timestamp: 2025-09-21T00:31:02.518Z
Learning: TestBed.tick() is a valid API introduced in Angular 20 for triggering effects in unit tests, similar to ApplicationRef.tick() but for testing contexts.
π 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/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts
π Learning: 2025-09-21T00:31:02.518Z
Learnt from: arnoud-dv
Repo: TanStack/query PR: 9669
File: docs/framework/angular/guides/testing.md:49-56
Timestamp: 2025-09-21T00:31:02.518Z
Learning: TestBed.tick() is a valid API introduced in Angular 20 for triggering effects in unit tests, similar to ApplicationRef.tick() but for testing contexts.
Applied to files:
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts
𧬠Code graph analysis (1)
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.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)
β° 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 (4)
packages/angular-query-persist-client/src/__tests__/with-persist-query-client.test.ts (4)
1-1: LGTM! Fake timer setup is correct.The import changes and fake timer setup in beforeEach/afterEach hooks align with the PR objectives and follow standard testing patterns.
Also applies to: 13-13, 21-27
36-38: LGTM! Correctly uses .then() pattern for no-side-effect async code.The restoreClient implementation properly uses the sleep().then() pattern for conciseness, which is appropriate since there are no side effects.
Based on learnings, when a queryFn contains no side effects, the .then() pattern is preferred.
55-59: LGTM! Error persister correctly delays error throw.The implementation properly uses sleep().then() to throw the error after a delay, ensuring compatibility with fake timer control.
110-128: Verify the 11-millisecond timer advancement.The render and assertion pattern correctly uses the captured
renderedresult, manual change detection, andtoBeInTheDocumentmatchers. However, line 125 advances timers by 11ms instead of 10ms (which matches the sleep duration). The extra millisecond might be intended to ensure microtask completion, but this pattern appears fragile and is repeated across multiple tests (lines 217, 376, 427).Consider verifying whether 10ms is sufficient or if a different approach (e.g., explicit microtask flushing) would be more reliable.
β¦onvert complex 'then' callbacks to async/await pattern
1 participant
π― Changes
β Checklist
pnpm run test:pr.π Release Impact
Summary by CodeRabbit
βοΈ Tip: You can customize this high-level summary in your review settings.