test(db): tighten includes oracle failure checks - #1717
Conversation
📝 WalkthroughWalkthroughThe includes oracle now uses shared put normalization and explicit ChangesOracle and sync test updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: 0 B Total Size: 125 kB ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 4.22 kB ℹ️ View Unchanged
|
![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/db/tests/query/includes-oracle.property.test.ts (1)
133-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass normalization inputs explicitly.
normalizePutcloses overaction,keys, andpositionsinsidehistory.map. Extract it as a small function with explicit parameters. This makes the normalization contract easier to test and makes state changes visible.As per coding guidelines: “Prefer explicit function parameters over closures to improve testability and clarify data flow dependencies.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/query/includes-oracle.property.test.ts` around lines 133 - 151, Extract normalizePut from the history.map closure and give it explicit parameters for the current action, keys, and positions state. Update each call site in the normalization logic, including optimistic confirmations, optimistic rollbacks, puts, and the empty-keys fallback, to pass those values explicitly while preserving the existing state updates and returned action.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/tests/query/includes-oracle.property.test.ts`:
- Around line 603-611: Update expectAssertionFailure so it only accepts the
marked chai.AssertionError produced by assertMatches, rather than any rejection
with an AssertionError name. Add a unique property to the assertMatches failure
and require that marker alongside Vitest’s chai.AssertionError; add regression
coverage for resolved promises, ordinary errors, unrelated assertions, and null
or undefined rejections.
---
Nitpick comments:
In `@packages/db/tests/query/includes-oracle.property.test.ts`:
- Around line 133-151: Extract normalizePut from the history.map closure and
give it explicit parameters for the current action, keys, and positions state.
Update each call site in the normalization logic, including optimistic
confirmations, optimistic rollbacks, puts, and the empty-keys fallback, to pass
those values explicitly while preserving the existing state updates and returned
action.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4eb6bef5-cfb7-45bd-93e8-9d41486c8822
📒 Files selected for processing (1)
packages/db/tests/query/includes-oracle.property.test.ts
| function expectAssertionFailure<TArgs extends Array<unknown>>( | ||
| assertion: (...args: TArgs) => Promise<void>, | ||
| ): (...args: TArgs) => Promise<void> { | ||
| return async (...args) => { | ||
| await expect(assertion(...args)).rejects.toMatchObject({ | ||
| name: `AssertionError`, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'expectAssertionFailure|expectScenarioMatches|assertMatches|toEqual|cleanup' \
packages/db/tests/query/includes-oracle.property.test.ts
fd -a -t f 'package.json|pnpm-lock.yaml|yarn.lock|package-lock.json' . \
-x rg -n -C 3 'vitest|AssertionError' {}Repository: TanStack/db
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- helper and call sites ---'
sed -n '580,615p;736,770p;1160,1210p;1240,1260p;1315,1335p' \
packages/db/tests/query/includes-oracle.property.test.ts
printf '%s\n' '--- Vitest imports and local dependency metadata ---'
rg -n -C 3 'from [`'\"'\"'vitest|from [`'\"'\"'`@vitest`|vitest' \
packages/db/tests/query/includes-oracle.property.test.ts package.json packages/db/package.json pnpm-lock.yaml \
| head -160
printf '%s\n' '--- installed package availability (read-only) ---'
if [ -d node_modules/vitest ]; then
rg -n -C 4 'AssertionError|AssertionErrorConstructor|chai' node_modules/vitest node_modules/@vitest 2>/dev/null | head -200
else
echo 'node_modules/vitest is absent'
fiRepository: TanStack/db
Length of output: 5099
🌐 Web query:
Vitest 3.2.4 source AssertionError expect rejects toMatchObject error type
💡 Result:
In Vitest, when an assertion using expect(...).rejects.toMatchObject(...) fails, it throws an AssertionError [1][2]. This error type originates from the Chai assertion library (which Vitest uses internally) or Vitest's custom implementation designed to provide detailed failure messages and diffs [2][3][4]. When.rejects is used, Vitest tracks the promise state; if the promise resolves instead of rejecting, or if the rejection value does not match the provided object structure, an AssertionError is generated with a message detailing the expected and actual values [2][3][5]. Key technical points regarding this behavior: - AssertionError Source: Vitest uses AssertionError to signify that a specific expectation was not met [1][5]. The error object typically contains properties such as expected, actual, and showDiff, which allow the test runner to display the characteristic colored diff output in the console [2][3]. - Rejection Mismatch: When the promise rejects, Vitest compares the rejected error (or value) against the object passed to toMatchObject [2][6]. If the properties do not match as expected, Vitest constructs an AssertionError with a message formatted as "expected [actual] to match object [expected]" (or the negated version) [1][2]. - Promise Resolution Failure: If the promise resolves when it was expected to reject, Vitest throws an AssertionError with a message such as 'promise resolved "..." instead of rejecting' [2][5]. You can rely on this error type being an instance of the class AssertionError, which is common across the Vitest/Chai ecosystem for handling test failures [1][3].
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/9423dc08/test/core/test/jest-expect.test.ts
- 2: https://github.com/vitest-dev/vitest/blob/206e8cff/packages/expect/src/jest-expect.ts
- 3: https://github.com/vitest-dev/vitest/blob/9423dc08/packages/expect/src/jest-expect.ts
- 4: https://github.com/vitest-dev/vitest/blob/9423dc08/packages/expect/src/utils.ts
- 5: https://github.com/vitest-dev/vitest/blob/9423dc08/test/cli/test/stacktraces.test.ts
- 6: https://vitest.dev/api/expect.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- imports and package metadata ---'
rg -n -C 4 'from .*vitest|from .*`@vitest`|vitest' \
packages/db/tests/query/includes-oracle.property.test.ts \
package.json packages/db/package.json pnpm-lock.yaml \
| head -220
printf '%s\n' '--- exact Vitest 3.2.4 source references ---'
for path in \
packages/expect/src/jest-expect.ts \
packages/expect/src/utils.ts \
packages/utils/src/error.ts \
packages/utils/src/index.ts
do
url="https://raw.githubusercontent.com/vitest-dev/vitest/v3.2.4/$path"
body="$(curl --fail --silent --show-error "$url" || true)"
if [ -n "$body" ]; then
printf '%s\n' "--- $path ---"
printf '%s\n' "$body" | rg -n -C 5 'AssertionError|rejects|toMatchObject|chai' | head -180
fi
done
printf '%s\n' '--- standalone acceptance probe for the current predicate ---'
node - <<'JS'
function currentPredicate(value) {
return value !== null
&& typeof value === 'object'
&& value.name === 'AssertionError'
}
const values = [
{ label: 'resolved promise', value: undefined },
{ label: 'normal Error', value: new Error('runtime') },
{ label: 'unrelated named error', value: Object.assign(new Error('setup'), { name: 'AssertionError' }) },
{ label: 'plain object with name', value: { name: 'AssertionError' } },
{ label: 'null rejection', value: null },
{ label: 'undefined rejection', value: undefined },
]
for (const { label, value } of values) {
console.log(label, currentPredicate(value))
}
JSRepository: TanStack/db
Length of output: 17582
🌐 Web query:
site:github.com/vitest-dev/vitest/tree/v3.2.4 packages/expect AssertionError
💡 Result:
Found the requested directory:
Vitest v3.2.4 — packages/expect
Open on GitHub [1]
It contains the src directory plus README.md, package.json, rollup.config.js, and tsconfig.json. [1]
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Vitest package exports and error class ---'
for path in \
packages/expect/src/index.ts \
packages/expect/package.json \
packages/vitest/src/index.ts \
packages/vitest/package.json \
packages/utils/src/error.ts \
packages/utils/src/index.ts
do
url="https://raw.githubusercontent.com/vitest-dev/vitest/v3.2.4/$path"
body="$(curl --fail --silent --show-error "$url")"
printf '%s\n' "--- $path ---"
printf '%s\n' "$body" | rg -n -C 6 'AssertionError|export|from .chai|dependencies|`@vitest/utils`' | head -220
done
printf '%s\n' '--- direct dependency declarations for Chai ---'
python3 - <<'PY'
import json
for path in ('package.json', 'packages/db/package.json'):
with open(path) as f:
data = json.load(f)
deps = {**data.get('dependencies', {}), **data.get('devDependencies', {})}
print(path, 'chai' in deps, deps.get('chai'))
PY
printf '%s\n' '--- exact current predicate behavior ---'
node - <<'JS'
function currentPredicate(value) {
return value !== null
&& typeof value === 'object'
&& value.name === 'AssertionError'
}
const values = [
['resolved promise sentinel', undefined],
['normal Error', new Error('runtime')],
['unrelated named error', Object.assign(new Error('setup'), { name: 'AssertionError' })],
['plain object with name', { name: 'AssertionError' }],
['null rejection', null],
['undefined rejection', undefined],
]
for (const [label, value] of values) {
console.log(`${label}: ${currentPredicate(value)}`)
}
JSRepository: TanStack/db
Length of output: 1450
🌐 Web query:
Vitest 3.2.4 AssertionError public export import from vitest or chai
💡 Result:
In Vitest 3.2.4, AssertionError is not directly exported as a public API from the vitest package. Vitest integrates Chai to provide its assertion capabilities [1][2][3]. If you are encountering an AssertionError, it is typically an instance of the class thrown by Chai when an assertion fails [4][5][6]. Because Vitest relies on Chai internally, you can access the AssertionError constructor by importing it from the chai package directly if needed for type checking or instance verification: import { AssertionError } from 'chai'; Vitest does provide the following public exports related to assertions via the vitest package [3]: 1. expect: The primary function for creating assertions [1][3]. 2. assert: The re-exported assert API from Chai [1][7][8]. 3. chai: The underlying chai object [3]. If you are trying to catch or identify assertion errors, you should use the AssertionError from the chai dependency rather than expecting it to be exported from Vitest itself [4][6].
Citations:
- 1: https://vitest.dev/api/expect.html
- 2: https://tangled.org/vitest.dev/vitest/blob/dbbbe4304ad2eda3077a680aa550d2895498a416/packages/vitest/src/integrations/chai/index.ts
- 3: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/packages/vitest/src/public/index.ts
- 4: Node v25 breaks tests with Web Storage API vitest-dev/vitest#8757
- 5:
vi.mock()doesn't work if module is already imported vitest-dev/vitest#8246 - 6: [vitest-pool-workers]
onAfterTryTaskcallsensurePoppedActiveTryStoragebeforeafterEachhooks are run cloudflare/workers-sdk#11690 - 7: https://github.com/vitest-dev/vitest/blob/v4.1.10/docs/api/assert.md
- 8: https://www.getbook.com/en/book/vitest-3/api/assert
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for path in \
packages/vitest/src/public/index.ts \
packages/vitest/package.json \
packages/vitest/src/integrations/chai/index.ts \
packages/utils/src/error.ts
do
url="https://raw.githubusercontent.com/vitest-dev/vitest/v3.2.4/$path"
body="$(curl --fail --silent --show-error "$url" || true)"
printf '%s\n' "--- $path ---"
if [ -n "$body" ]; then
printf '%s\n' "$body" | rg -n -C 8 'AssertionError|chai|export' | head -240
else
echo 'not found'
fi
done
printf '%s\n' '--- direct dependency declarations ---'
python3 - <<'PY'
import json
for path in ('package.json', 'packages/db/package.json'):
with open(path) as f:
data = json.load(f)
deps = {**data.get('dependencies', {}), **data.get('devDependencies', {})}
print(path, {'vitest': deps.get('vitest'), 'chai': deps.get('chai')})
PY
printf '%s\n' '--- current predicate acceptance table ---'
node - <<'JS'
function currentPredicate(value) {
return value !== null
&& typeof value === 'object'
&& value.name === 'AssertionError'
}
const values = [
['resolved promise sentinel', undefined],
['normal Error', new Error('runtime')],
['unrelated named error', Object.assign(new Error('setup'), { name: 'AssertionError' })],
['plain object with name', { name: 'AssertionError' }],
['null rejection', null],
['undefined rejection', undefined],
]
for (const [label, value] of values) {
console.log(`${label}: ${currentPredicate(value)}`)
}
JSRepository: TanStack/db
Length of output: 15186
🌐 Web query:
site:github.com/vitest-dev/vitest/blob/v3.2.4/packages/vitest/src/public/index.ts chai AssertionError
💡 Result:
In Vitest v3.2.4, packages/vitest/src/public/index.ts does not directly import or export Chai’s AssertionError. It re-exports Chai integrations here:
export { assert, chai, createExpect, expect, should } from '../integrations/chai'See lines 102–103. (raw.githubusercontent.com)
The relevant AssertionError definition is therefore likely in Chai or Vitest’s integration/expect packages, not this file.
Citations:
Restrict expected failures to the oracle mismatch.
toMatchObject({ name: \AssertionError` })accepts any object or error with that property. This includes the setup assertion at Line 752, so the test can pass without reachingassertMatches. Mark the assertMatchesfailure with a unique property and require that marker pluschai.AssertionError, available through Vitest’s chaiexport. Add regression tests for resolved promises, normal errors, unrelated assertions, andnullorundefined` rejections.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/tests/query/includes-oracle.property.test.ts` around lines 603 -
611, Update expectAssertionFailure so it only accepts the marked
chai.AssertionError produced by assertMatches, rather than any rejection with an
AssertionError name. Add a unique property to the assertMatches failure and
require that marker alongside Vitest’s chai.AssertionError; add regression
coverage for resolved promises, ordinary errors, unrelated assertions, and null
or undefined rejections.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/db/tests/utils.ts (1)
241-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated pending-sync lifecycle.
Both factories define the same pending-promise creation and cleanup logic. Extract this state machine into one focused utility. This keeps rejection handling consistent and prevents future divergence. Add explicit return types to the extracted callbacks, such as
(): Promise<void>and(): void.As per coding guidelines: Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places, and prefer small, focused utility functions.
Also applies to: 341-349
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/db/tests/utils.ts` around lines 241 - 249, Extract the shared pending-sync promise creation and cleanup state machine used by both factories into one focused utility function, then have each factory reuse it instead of maintaining duplicate lifecycle logic. Preserve the existing resolve, reject, finally-cleanup, and rejection-handling behavior, and add explicit return types to the extracted callbacks, including Promise<void> for promise-producing callbacks and void for cleanup callbacks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/db/tests/utils.ts`:
- Around line 241-242: Update both mockSyncCollectionOptions and
mockSyncCollectionOptionsNoInitialState in packages/db/tests/utils.ts at lines
241-242 and 341-342 to attach a rejection handler to the cleanup chain returned
by finally(), ensuring rejected pending syncs do not produce unhandled derived
rejections while preserving clearPendingSync execution.
---
Nitpick comments:
In `@packages/db/tests/utils.ts`:
- Around line 241-249: Extract the shared pending-sync promise creation and
cleanup state machine used by both factories into one focused utility function,
then have each factory reuse it instead of maintaining duplicate lifecycle
logic. Preserve the existing resolve, reject, finally-cleanup, and
rejection-handling behavior, and add explicit return types to the extracted
callbacks, including Promise<void> for promise-producing callbacks and void for
cleanup callbacks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 867f54e3-8014-42af-b048-0fd3e95cc660
📒 Files selected for processing (2)
packages/db/tests/query/includes-oracle.property.test.tspackages/db/tests/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/tests/query/includes-oracle.property.test.ts
| void syncPendingPromise.finally(clearPendingSync) | ||
| return syncPendingPromise |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target helpers ---'
sed -n '210,260p' packages/db/tests/utils.ts
sed -n '310,360p' packages/db/tests/utils.ts
printf '%s\n' '--- rejection test context ---'
sed -n '1800,1850p' packages/db/tests/collection.test.ts
printf '%s\n' '--- all relevant usages ---'
rg -n -C 3 'mockSyncCollectionOptions(NoInitialState)?|syncPendingPromise|clearPendingSync' packages/db/testsRepository: TanStack/db
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- rejection helpers ---'
rg -n -C 8 'function withExpectedRejection|const withExpectedRejection|withExpectedRejection|function flushPromises|const flushPromises' packages/db/tests/utils.ts packages/db/tests/collection.test.ts
printf '%s\n' '--- existing cleanup patterns ---'
rg -n -C 3 '\.finally\(|\.catch\(\(\) *=>|then\(.*clear|void .*Promise' packages/db --glob '*.{ts,tsx,js}'
printf '%s\n' '--- Promise.finally rejection probe ---'
node - <<'JS'
'use strict'
const events = []
process.on('unhandledRejection', (reason) => {
events.push(reason.message)
})
async function probe() {
let reject
const original = new Promise((_, r) => {
reject = r
})
void original.finally(() => {})
original.catch(() => {})
reject(new Error('pending sync rejected'))
await new Promise((resolve) => setImmediate(resolve))
await new Promise((resolve) => setImmediate(resolve))
console.log(JSON.stringify({ unhandledRejections: events }))
}
probe()
JSRepository: TanStack/db
Length of output: 25351
Handle rejected promises returned by finally(). Add a rejection handler to the cleanup chain in both mockSyncCollectionOptions and mockSyncCollectionOptionsNoInitialState. Otherwise, a rejected pending sync creates an unhandled derived rejection even when the original promise is handled.
📍 Affects 1 file
packages/db/tests/utils.ts#L241-L242(this comment)packages/db/tests/utils.ts#L341-L342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/db/tests/utils.ts` around lines 241 - 242, Update both
mockSyncCollectionOptions and mockSyncCollectionOptionsNoInitialState in
packages/db/tests/utils.ts at lines 241-242 and 341-342 to attach a rejection
handler to the cleanup chain returned by finally(), ensuring rejected pending
syncs do not produce unhandled derived rejections while preserving
clearPendingSync execution.
2 participants
Tightens the recompute oracle added in #1716 so known-bug cases accept only the intended assertion mismatch. It also restores repeated optimistic rollback coverage and fixes generator normalization; there are no runtime or API changes.
Root cause
fcTest.failstreats every thrown error as success. A setup, cleanup, runtime, or assertion error could therefore make a known-bug case look healthy.Generated actions also had two state-machine gaps:
putoperations bypassed stable-position bookkeeping, which let the known reorder bug enter the required green property.Approach
.failswith normal properties wrapped byexpectAssertionFailure, which accepts only VitestAssertionErrormismatches.putthrough one normalization helper.Key invariants
putpreserves an existing row's generated position, including converted actions.Non-goals
This small follow-up does not fix the include bugs represented by the known-failure cases or broaden the structural oracle. RFC #1658 now assigns these cases to the generalized trace-oracle follow-up:
The explicit depth-specific query shapes also remain in place so TypeScript checks the real public DSL at depths 1–4.
Trade-offs
The review suggested asserting each exact current wrong output. Matching the assertion type keeps the correct recompute result as the sole semantic reference instead of encoding implementation-specific wrong states.
Required green histories still keep correlation keys and existing-row positions stable. Reparenting and reordering include known-broken classes with deterministic failing cases; enabling the whole transition domain now would make required random CI fail until the runtime fixes land. The generalized runner will add those domains as a separate, controlled track.
Verification
Red/green checks:
.fails; it fails underexpectAssertionFailure.1665011958, where a converted put changed an existing position; normalization fixes it.Full local DB suite: 106 files passed, 2,493 tests passed, 5 skipped, no type errors. All required checks on this PR are green.
Files changed
packages/db/tests/query/includes-oracle.property.test.ts— narrows expected failures, normalizes converted puts, restores repeated rollback coverage, improves generator signal, and documents scope.packages/db/tests/utils.ts— clears mock mutation state after both resolve and reject.Related: #1716, #1658, #1495.