test(svelte-query/queryOptions): add runtime test for identity function#10178
test(svelte-query/queryOptions): add runtime test for identity function#10178sukvvon wants to merge 1 commit intoTanStack:mainfrom
Conversation
|
📝 WalkthroughWalkthroughA new test file was added to verify that the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 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 |
|
View your CI Pipeline Execution ↗ for commit 47746e6
☁️ 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: 1
🧹 Nitpick comments (1)
packages/svelte-query/tests/queryOptions/queryOptions.svelte.test.ts (1)
1-5: The.svelte.test.tsextension doesn't match the test's contents.This test is a pure unit test with no Svelte-specific code (no
flushSync,$state, component mounting, etc.). While the entire svelte-query test suite consistently uses.svelte.test.tsfor uniformity, this file could be renamed to.test.tsfor accuracy, since other files in the package that actually use Svelte runtime features share the same extension. Consider renaming to.test.tsunless there's a specific reason to maintain the package-wide.svelte.test.tsconvention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/svelte-query/tests/queryOptions/queryOptions.svelte.test.ts` around lines 1 - 5, The test file uses only plain unit tests for the exported function queryOptions and therefore should be renamed from queryOptions.svelte.test.ts to queryOptions.test.ts to reflect that it contains no Svelte-specific code; locate the test that imports queryOptions (symbol: queryOptions) and rename the file extension, and if any project scripts or test-indexing rely on the .svelte.test.ts convention update them accordingly so the test runner still discovers the renamed file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/svelte-query/tests/queryOptions/queryOptions.svelte.test.ts`:
- Line 11: The test currently asserts structural equality but should assert
reference identity; update the assertion in the test for queryOptions to use
toBe instead of toStrictEqual so it verifies that queryOptions(object) returns
the exact same object reference (i.e.,
expect(queryOptions(object)).toBe(object)); locate the assertion referencing
queryOptions in the queryOptions.svelte.test.ts and replace the matcher
accordingly.
---
Nitpick comments:
In `@packages/svelte-query/tests/queryOptions/queryOptions.svelte.test.ts`:
- Around line 1-5: The test file uses only plain unit tests for the exported
function queryOptions and therefore should be renamed from
queryOptions.svelte.test.ts to queryOptions.test.ts to reflect that it contains
no Svelte-specific code; locate the test that imports queryOptions (symbol:
queryOptions) and rename the file extension, and if any project scripts or
test-indexing rely on the .svelte.test.ts convention update them accordingly so
the test runner still discovers the renamed file.
| queryFn: () => Promise.resolve(5), | ||
| } as const | ||
|
|
||
| expect(queryOptions(object)).toStrictEqual(object) |
There was a problem hiding this comment.
Use toBe instead of toStrictEqual to assert reference identity.
The PR explicitly targets testing the identity function contract — that queryOptions returns the exact same object reference passed in. queryOptions "just returns whatever you pass into it" at runtime, so the test should verify reference equality (===), not structural equality.
toStrictEqual performs a deep value comparison. It would pass even if queryOptions returned a new object with the same shape (same queryFn reference and equal queryKey elements), silently allowing a regression if the implementation ever copies the input instead of returning it directly.
🐛 Proposed fix
- expect(queryOptions(object)).toStrictEqual(object)
+ expect(queryOptions(object)).toBe(object)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(queryOptions(object)).toStrictEqual(object) | |
| expect(queryOptions(object)).toBe(object) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/svelte-query/tests/queryOptions/queryOptions.svelte.test.ts` at line
11, The test currently asserts structural equality but should assert reference
identity; update the assertion in the test for queryOptions to use toBe instead
of toStrictEqual so it verifies that queryOptions(object) returns the exact same
object reference (i.e., expect(queryOptions(object)).toBe(object)); locate the
assertion referencing queryOptions in the queryOptions.svelte.test.ts and
replace the matcher accordingly.
1 participant
🎯 Changes
Add a runtime test for
queryOptionsidentity function in@tanstack/svelte-query, matching the existing test in@tanstack/react-query.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit