test(query-core/queriesObserver): add test for 'trackResult' property tracking synchronization across all observers#10185
Conversation
… tracking synchronization across all observers
|
|
View your CI Pipeline Execution ↗ for commit 9a7d894
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughAdds a new unit test to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
535-539:getCombinedResult(trackedResults)is unnecessary whencombineisundefined; assert ontrackedResultsdirectly.With no
combinefunction, passing the proxy-wrappedtrackedResultsintogetCombinedResultonly to check.lengthis redundant: it risks triggering extra proxy get-traps that affect the spy count, and the same assertion can be made ontrackedResultsitself.♻️ Proposed refactor
- const trackedResults = trackResult() - const combinedResult = getCombinedResult(trackedResults) - - expect(combinedResult).toHaveLength(2) + const trackedResults = trackResult() + + expect(trackedResults).toHaveLength(2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/queriesObserver.test.tsx` around lines 535 - 539, The test unnecessarily calls getCombinedResult(trackedResults) when no combine function is provided, which can trigger proxy get-traps and alter spy counts; change the assertion to check trackedResults directly by removing the getCombinedResult call and the combinedResult variable and replace expect(combinedResult).toHaveLength(2) with expect(trackedResults).toHaveLength(2) (references: getCombinedResult, trackResult, trackedResults, combinedResult, combine).
🤖 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__/queriesObserver.test.tsx`:
- Around line 535-539: The test unnecessarily calls
getCombinedResult(trackedResults) when no combine function is provided, which
can trigger proxy get-traps and alter spy counts; change the assertion to check
trackedResults directly by removing the getCombinedResult call and the
combinedResult variable and replace expect(combinedResult).toHaveLength(2) with
expect(trackedResults).toHaveLength(2) (references: getCombinedResult,
trackResult, trackedResults, combinedResult, combine).
…lt' call in 'trackResult' test
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/query-core/src/__tests__/queriesObserver.test.tsx (1)
525-547:mockRestore()is not guaranteed to run if an assertion throws — considerafterEachortry/finally.If any
expect(...)call at lines 544–545 fails,trackPropSpy.mockRestore()is never reached, leavingQueryObserver.prototype.trackProppermanently patched for any tests that run after this one in the suite.♻️ Proposed fix using try/finally
const trackPropSpy = vi.spyOn(QueryObserver.prototype, 'trackProp') + try { const [, , trackResult] = observer.getOptimisticResult( [ { queryKey: key1, queryFn: queryFn1 }, { queryKey: key2, queryFn: queryFn2 }, ], undefined, ) const trackedResults = trackResult() expect(trackedResults).toHaveLength(2) void trackedResults[0]!.status expect(trackPropSpy).toHaveBeenCalledWith('status') expect(trackPropSpy).toHaveBeenCalledTimes(3) - - trackPropSpy.mockRestore() + } finally { + trackPropSpy.mockRestore() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/query-core/src/__tests__/queriesObserver.test.tsx` around lines 525 - 547, The test installs a spy on QueryObserver.prototype.trackProp (trackPropSpy) but calls trackPropSpy.mockRestore() only at the end, which may be skipped if an assertion throws; wrap the assertion block in a try/finally (or move restore to an afterEach) so that trackPropSpy.mockRestore() always runs; specifically, ensure the spy created for QueryObserver.prototype.trackProp around the getOptimisticResult / trackedResults assertions is restored in a finally block (or restored in a test-level afterEach) to avoid leaking the patched method to other tests.
🤖 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__/queriesObserver.test.tsx`:
- Around line 525-547: The test installs a spy on
QueryObserver.prototype.trackProp (trackPropSpy) but calls
trackPropSpy.mockRestore() only at the end, which may be skipped if an assertion
throws; wrap the assertion block in a try/finally (or move restore to an
afterEach) so that trackPropSpy.mockRestore() always runs; specifically, ensure
the spy created for QueryObserver.prototype.trackProp around the
getOptimisticResult / trackedResults assertions is restored in a finally block
(or restored in a test-level afterEach) to avoid leaking the patched method to
other tests.
1 participant
🎯 Changes
Add a test to verify that
trackResultsynchronizes property tracking across all observers inQueriesObserver.When a property (e.g.,
status) is accessed on one tracked result,trackPropis called on all observers — 1 direct call from the accessed observer's proxy + 2 synchronized calls fromonPropTrackedcallback (one per observer).✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit