test(query-core/utils): add tests for 'ensureQueryFn' initialPromise fallback and skipToken handling#10527
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new test suite for the ensureQueryFn utility that verifies behavior when queryFn is absent but an initialPromise is provided, when initialPromise rejects, and when queryFn is skipToken (expects error logging and a rejection keyed by queryHash). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…fallback and skipToken handling
KimHyeongRae0
force-pushed
the
test/query-core-utils-ensure-query-fn
branch
from
8cdebe3 to
b881482
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/query-core/src/__tests__/utils.test.tsx (2)
540-588: Unnecessaryas unknown as () => Promise<...>casts.
ensureQueryFnreturns aQueryFunction, which is directly callable; the double cast to() => Promise<T>just to invoke it is noisy and hides the real signature. Passing a minimalQueryFunctionContext(or typingresolvedas() => Promise<unknown>once) reads cleaner and still exercises the branch. Not blocking — the tests are correct as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/utils.test.tsx` around lines 540 - 588, The tests use noisy double casts like (resolved as unknown as () => Promise<...>)() — change them to call the returned QueryFunction directly by either typing resolved as the appropriate function type or invoking it with a minimal QueryFunctionContext; specifically update uses of resolved from ensureQueryFn (and the skipToken case) to remove the "as unknown as" casts and instead call resolved(...) with a minimal context or declare resolved: QueryFunction/() => Promise<...> so the real signature is exercised and the casts are eliminated.
567-587: Restoreconsole.errorspy in afinally/afterEachto prevent leakage on assertion failure.If either
expectabovemockRestore()throws, the spy is never restored and will bleed into subsequent tests in the suite (e.g. polluting otherconsole.errorassertions or hiding real errors). PreferafterEach(() => vi.restoreAllMocks())at thedescribe('ensureQueryFn')level, or wrap intry { ... } finally { consoleErrorSpy.mockRestore() }.♻️ Suggested structure
it('should return a function that rejects with missing queryFn error when queryFn is set to skipToken', async () => { const consoleErrorSpy = vi .spyOn(console, 'error') .mockImplementation(() => undefined) - const resolved = ensureQueryFn({ - queryFn: skipToken, - queryHash: '["skip"]', - }) - - expect(consoleErrorSpy).toHaveBeenCalledWith( - expect.stringContaining( - 'Attempted to invoke queryFn when set to skipToken', - ), - ) - await expect( - (resolved as unknown as () => Promise<unknown>)(), - ).rejects.toThrow('Missing queryFn: \'["skip"]\'') - - consoleErrorSpy.mockRestore() + try { + const resolved = ensureQueryFn({ + queryFn: skipToken, + queryHash: '["skip"]', + }) + + expect(consoleErrorSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'Attempted to invoke queryFn when set to skipToken', + ), + ) + await expect( + (resolved as unknown as () => Promise<unknown>)(), + ).rejects.toThrow('Missing queryFn: \'["skip"]\'') + } finally { + consoleErrorSpy.mockRestore() + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/utils.test.tsx` around lines 567 - 587, The test creates a spy on console.error (consoleErrorSpy) but restores it only at the end of the test, which can leak if an assertion throws; update the test suite that contains the ensureQueryFn tests to guarantee the spy is always restored by either adding an afterEach(() => vi.restoreAllMocks()) scoped to the describe block for ensureQueryFn or wrapping the test body in try { ... } finally { consoleErrorSpy.mockRestore() }; reference ensureQueryFn, skipToken, and consoleErrorSpy when making the change so the spy is always cleaned up even on assertion failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/query-core/src/__tests__/utils.test.tsx`:
- Around line 540-588: The tests use noisy double casts like (resolved as
unknown as () => Promise<...>)() — change them to call the returned
QueryFunction directly by either typing resolved as the appropriate function
type or invoking it with a minimal QueryFunctionContext; specifically update
uses of resolved from ensureQueryFn (and the skipToken case) to remove the "as
unknown as" casts and instead call resolved(...) with a minimal context or
declare resolved: QueryFunction/() => Promise<...> so the real signature is
exercised and the casts are eliminated.
- Around line 567-587: The test creates a spy on console.error (consoleErrorSpy)
but restores it only at the end of the test, which can leak if an assertion
throws; update the test suite that contains the ensureQueryFn tests to guarantee
the spy is always restored by either adding an afterEach(() =>
vi.restoreAllMocks()) scoped to the describe block for ensureQueryFn or wrapping
the test body in try { ... } finally { consoleErrorSpy.mockRestore() };
reference ensureQueryFn, skipToken, and consoleErrorSpy when making the change
so the spy is always cleaned up even on assertion failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ed94f86-9b94-43af-ba5b-88d745b565ad
📒 Files selected for processing (1)
packages/query-core/src/__tests__/utils.test.tsx
|
Leaving the spy/cast style as-is. Matches the existing pattern in this file. |
sukvvon
left a comment
sukvvon
left a comment
There was a problem hiding this comment.
The triple (resolved as unknown as () => Promise<...>)() cast can be avoided by calling the returned function with a QueryFunctionContext. Since the implementation ignores the context, {} as QueryFunctionContext is enough, and it stays type-safe.
| shouldThrowError, | ||
| skipToken, | ||
| } from '../utils' | ||
| import { Mutation } from '../mutation' |
There was a problem hiding this comment.
| import { Mutation } from '../mutation' | |
| import { Mutation } from '../mutation' | |
| import type { QueryFunctionContext } from '..' |
| await expect( | ||
| (resolved as unknown as () => Promise<string>)(), | ||
| ).resolves.toBe('initial-data') |
There was a problem hiding this comment.
| await expect( | |
| (resolved as unknown as () => Promise<string>)(), | |
| ).resolves.toBe('initial-data') | |
| await expect(resolved(context)).resolves.toBe('initial-data') |
Also add a shared context at the top of the describe block:
describe('ensureQueryFn', () => {
const context = {} as QueryFunctionContext
...| await expect( | ||
| (resolved as unknown as () => Promise<unknown>)(), | ||
| ).rejects.toBe(error) |
There was a problem hiding this comment.
| await expect( | |
| (resolved as unknown as () => Promise<unknown>)(), | |
| ).rejects.toBe(error) | |
| await expect(resolved(context)).rejects.toBe(error) |
| await expect( | ||
| (resolved as unknown as () => Promise<unknown>)(), | ||
| ).rejects.toThrow('Missing queryFn: \'["skip"]\'') |
There was a problem hiding this comment.
| await expect( | |
| (resolved as unknown as () => Promise<unknown>)(), | |
| ).rejects.toThrow('Missing queryFn: \'["skip"]\'') | |
| await expect(resolved(context)).rejects.toThrow( | |
| 'Missing queryFn: \'["skip"]\'', | |
| ) |
…Fn tests Replace the `(resolved as unknown as () => Promise<...>)()` double-cast with `resolved(context)` using a shared `QueryFunctionContext` fixture. Per review suggestions on TanStack#10527.
|
Fair point. Applied all four in 902e5b9 — context fixture is cleaner than the double cast. |
|
View your CI Pipeline Execution ↗ for commit 6763bff
☁️ Nx Cloud last updated this comment at |
2 participants
🎯 Changes
Adds unit tests for
ensureQueryFninpackages/query-core/src/utils.ts, covering branches that previously had no direct coverage:fetchOptions.initialPromisewhenqueryFnis missing but aninitialPromiseis provided (utils.ts:450-451).initialPromiserejects.console.errorand returns a function that rejects with the "Missing queryFn" error whenqueryFnis set toskipToken(utils.ts:439-444, 454-457).Coverage for
packages/query-core/src/utils.tsimproves from96.4% → 97.6%branch and96.77% → 100%function.✅ Checklist
npx nx run @tanstack/query-core:test:libandnpx nx run @tanstack/query-core:test:types.🚀 Release Impact
Summary by CodeRabbit
Note: No user-facing changes; updates are limited to internal test coverage.