test(db): add controlled includes oracle drivers - #1720
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds checkpoint-aware trace assertion errors and a reusable expected-failure helper. It expands includes oracle tests with deterministic batch, relationship-transition, and temporal-loading scenarios. ChangesIncludes oracle coverage
Estimated code review effort: 4 (Complex) | ~60 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
packages/db/tests/query/includes-temporal-oracle.test.ts (4)
171-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the live-collection type once.
ReturnType<typeof createLiveQueryCollection>instantiates the generic with its constraint, so the resulting row type is maximally loose andlive.get(...)results carry no useful shape. The expression also repeats in three context types. Introduce one alias, and consider parameterizing it with the projected row type where the test reads rows.♻️ Proposed refactor
+type LiveCollection = ReturnType<typeof createLiveQueryCollection>Then use
live: LiveCollectioninReadinessContext,DemandCancellationContext, andProgressiveContext.As per coding guidelines: "Always provide the most precise return type annotation".
Also applies to: 268-268, 419-419
🤖 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-temporal-oracle.test.ts` at line 171, Define a single LiveCollection type alias for the live-query collection, parameterized with the projected row type used by the test so live.get(...) retains a useful shape. Replace the repeated ReturnType<typeof createLiveQueryCollection> annotations in ReadinessContext, DemandCancellationContext, and ProgressiveContext with LiveCollection.Source: Coding guidelines
216-220: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAn unfulfilled load turns a trace failure into a suite timeout.
startawaitscontext.parentLoaded.promise, andcreateColdPostsresolves that deferred only insideloadSubset. If the outer query never demands the parent subset, this trace hangs until the Vitest timeout instead of failing at a checkpoint. The same shape applies tocontext.childLoadStarted.promiseincreateDemandCancellationDriverandcontext.startReached.promiseincreateProgressiveDriver.Consider racing these waits against a bounded timer so a regression reports a diagnosable failure.
🤖 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-temporal-oracle.test.ts` around lines 216 - 220, Update the async drivers around start, createDemandCancellationDriver, and createProgressiveDriver so waits on context.parentLoaded.promise, context.childLoadStarted.promise, and context.startReached.promise race against bounded timers. Reject or otherwise fail with a diagnostic checkpoint error when the timer expires, while preserving successful deferred-resolution behavior.
285-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the late-bound
removeclosure.
removeis reassigned insidesyncand exposed through the wrapper() => remove(). The wrapper is required so that callers observe the reassignment. Add a short comment, because a later refactor can replace the wrapper withremoveand silently capture the throwing placeholder.As per coding guidelines: "Keep comments that explain non-obvious behavior, such as return value signals or closure captures".
🤖 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-temporal-oracle.test.ts` around lines 285 - 306, In the helper that creates the temporal removable post collection, add a short comment documenting that the returned wrapper around remove must remain late-bound so callers observe the reassignment performed inside sync rather than capturing the initial throwing placeholder. Keep the existing remove initialization and reassignment behavior unchanged.Source: Coding guidelines
230-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated projection tail.
The three projections repeat the same
recomputeandassertEqualimplementations. Extract one helper and reuse it, so the comparison rule stays in one place.♻️ Proposed refactor
+function expectedProjection<TObserved>( + observe: (context: { expected: TObserved }) => TObserved, +): TraceProjection<{ expected: TObserved }, TObserved> { + return { + observe, + recompute: ({ expected }) => expected, + assertEqual: (observed, expected) => { + expect(observed).toEqual(expected) + return undefined + }, + } +}Then each projection only supplies
observe:-const readinessProjection: TraceProjection< - ReadinessContext, - ReadinessObservation -> = { - observe: ({ live, loads, preload }) => ({ ... }), - recompute: ({ expected }) => expected, - assertEqual: (observed, expected) => { - expect(observed).toEqual(expected) - return undefined - }, -} +const readinessProjection = expectedProjection<ReadinessObservation>( + ({ live, loads, preload }) => ({ ... }), +)As per coding guidelines: "Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places".
Also applies to: 378-393, 608-622
🤖 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-temporal-oracle.test.ts` around lines 230 - 246, Extract the shared recompute and assertEqual behavior from the three projections into a single helper, then spread or otherwise reuse that helper in readinessProjection and the projections near the other referenced sections. Keep each projection responsible only for its observe implementation while preserving the existing comparison semantics.Source: Coding guidelines
packages/db/tests/expected-failure.test.ts (1)
22-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the negative assertions specific.
rejects.toBeInstanceOf(Error)passes for any thrown error, including an unrelated failure insideexpectAssertionFailureitself. Assert on the failure content so the negative cases keep their meaning.♻️ Proposed refinement
- await expect(guarded()).rejects.toBeInstanceOf(Error) + await expect(guarded()).rejects.toMatchObject({ + name: `AssertionError`, + })🤖 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/expected-failure.test.ts` around lines 22 - 44, Update both tests around expectAssertionFailure to assert the specific rejection content rather than only Error type: verify “startup mismatch” for the wrong-checkpoint case and “projection failed” for the runtime-error case, while preserving their existing rejection expectations.packages/db/tests/query/includes-oracle.property.test.ts (3)
1657-1675: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant standalone coverage test.
This test samples
visibleRelationshipScenarioArbitrary(depth, 'reparent')with seed1721 + depth. The property test at Lines 1717-1744 uses the same arbitrary and the same seed, and it already assertsexpect(result).not.toEqual(beforeTransition)for every generated scenario. The standalone test adds runtime without adding a distinct assertion.🤖 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 1657 - 1675, Remove the standalone `covers a visible relationship transition at every depth` test, including its scenario sampling and loop; the existing property test using `visibleRelationshipScenarioArbitrary` already covers the same seeded scenarios and assertion.
117-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared action-type list.
The two
fc.constantFrombranches repeat three of four action types. Extract the child list and derive the root list from it, so a new action type only needs one edit.♻️ Proposed refactor
function actionArbitrary(depth: IncludeDepth): fc.Arbitrary<HistoryAction> { + const childTypes = [ + `put`, + `delete`, + `optimisticConfirm`, + `optimisticRollback`, + ] as const + const rootTypes = childTypes.filter((type) => type !== `delete`) return levelArbitrary(depth).chain((level) => fc.record({ // Root delete/reinsert has a deterministic expected-failure trace below. // Keep the green fuzz corpus from rediscovering the same defect class. - type: - level === 0 - ? fc.constantFrom( - `put` as const, - `optimisticConfirm` as const, - `optimisticRollback` as const, - ) - : fc.constantFrom( - `put` as const, - `delete` as const, - `optimisticConfirm` as const, - `optimisticRollback` as const, - ), + type: fc.constantFrom(...(level === 0 ? rootTypes : childTypes)), level: fc.constant(level),🤖 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 117 - 141, Refactor the action-type definitions in the level arbitrary builder around the two fc.constantFrom branches: define the shared child action list once, then derive the root list by excluding delete, preserving the existing root and child behavior while allowing new shared actions to be added in one place.
2119-2119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch test-owned data in the expected-failure guard.
/deeply equal/is Vitest boilerplate, so anytoEqualmismatch can satisfy this guard. Match data specific to the duplicate-alias failure instead.🤖 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` at line 2119, Update the expected-failure guards in packages/db/tests/query/includes-oracle.property.test.ts at lines 2119 and 2199 to match duplicate-alias test-owned data rather than generic Vitest “deeply equal” boilerplate; apply the same targeted message pattern at both sites so unrelated toEqual mismatches cannot satisfy the guard.
🤖 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/expected-failure.test.ts`:
- Around line 58-66: Extend the expected-failure tests around
expectAssertionFailure to cover guarded assertions whose underlying promise
resolves, including both the checkpoint and message option branches. Assert that
the guard itself rejects when the wrapped assertion resolves, while preserving
the existing runtime-error message collision case.
---
Nitpick comments:
In `@packages/db/tests/expected-failure.test.ts`:
- Around line 22-44: Update both tests around expectAssertionFailure to assert
the specific rejection content rather than only Error type: verify “startup
mismatch” for the wrong-checkpoint case and “projection failed” for the
runtime-error case, while preserving their existing rejection expectations.
In `@packages/db/tests/query/includes-oracle.property.test.ts`:
- Around line 1657-1675: Remove the standalone `covers a visible relationship
transition at every depth` test, including its scenario sampling and loop; the
existing property test using `visibleRelationshipScenarioArbitrary` already
covers the same seeded scenarios and assertion.
- Around line 117-141: Refactor the action-type definitions in the level
arbitrary builder around the two fc.constantFrom branches: define the shared
child action list once, then derive the root list by excluding delete,
preserving the existing root and child behavior while allowing new shared
actions to be added in one place.
- Line 2119: Update the expected-failure guards in
packages/db/tests/query/includes-oracle.property.test.ts at lines 2119 and 2199
to match duplicate-alias test-owned data rather than generic Vitest “deeply
equal” boilerplate; apply the same targeted message pattern at both sites so
unrelated toEqual mismatches cannot satisfy the guard.
In `@packages/db/tests/query/includes-temporal-oracle.test.ts`:
- Line 171: Define a single LiveCollection type alias for the live-query
collection, parameterized with the projected row type used by the test so
live.get(...) retains a useful shape. Replace the repeated ReturnType<typeof
createLiveQueryCollection> annotations in ReadinessContext,
DemandCancellationContext, and ProgressiveContext with LiveCollection.
- Around line 216-220: Update the async drivers around start,
createDemandCancellationDriver, and createProgressiveDriver so waits on
context.parentLoaded.promise, context.childLoadStarted.promise, and
context.startReached.promise race against bounded timers. Reject or otherwise
fail with a diagnostic checkpoint error when the timer expires, while preserving
successful deferred-resolution behavior.
- Around line 285-306: In the helper that creates the temporal removable post
collection, add a short comment documenting that the returned wrapper around
remove must remain late-bound so callers observe the reassignment performed
inside sync rather than capturing the initial throwing placeholder. Keep the
existing remove initialization and reassignment behavior unchanged.
- Around line 230-246: Extract the shared recompute and assertEqual behavior
from the three projections into a single helper, then spread or otherwise reuse
that helper in readinessProjection and the projections near the other referenced
sections. Keep each projection responsible only for its observe implementation
while preserving the existing comparison semantics.
🪄 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: 5129c761-7925-439b-8c3a-04d21b860b91
📒 Files selected for processing (6)
packages/db/tests/expected-failure.test.tspackages/db/tests/expected-failure.tspackages/db/tests/query/includes-oracle.property.test.tspackages/db/tests/query/includes-temporal-oracle.test.tspackages/db/tests/trace-runner.test.tspackages/db/tests/trace-runner.ts
Comment on lines
+58
to
+66
| it(`rejects runtime errors that happen to have the expected message`, async () => { | ||
| const runtimeError = new TypeError(`expected value is missing`) | ||
| const guarded = expectAssertionFailure(() => Promise.reject(runtimeError), { | ||
| message: /expected/, | ||
| }) | ||
|
|
||
| await expect(guarded()).rejects.toBeInstanceOf(Error) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a case where the guarded assertion resolves.
The guard exists to lock in known failures. If the underlying defect is fixed, the guarded assertion resolves and the guard must fail. That path is not covered for either the checkpoint branch or the message branch.
Based on learnings, test corner cases including resolved promises.
💚 Proposed additional tests
it(`rejects runtime errors that happen to have the expected message`, async () => {
const runtimeError = new TypeError(`expected value is missing`)
const guarded = expectAssertionFailure(() => Promise.reject(runtimeError), {
message: /expected/,
})
await expect(guarded()).rejects.toBeInstanceOf(Error)
})
+
+ it(`rejects an assertion that unexpectedly passes`, async () => {
+ const guardedCheckpoint = expectAssertionFailure(
+ () => Promise.resolve(),
+ { checkpoint: 2 },
+ )
+ const guardedMessage = expectAssertionFailure(() => Promise.resolve(), {
+ message: /expected/,
+ })
+
+ await expect(guardedCheckpoint()).rejects.toBeInstanceOf(Error)
+ await expect(guardedMessage()).rejects.toBeInstanceOf(Error)
+ })
})📝 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.
Suggested change
| it(`rejects runtime errors that happen to have the expected message`, async () => { | |
| const runtimeError = new TypeError(`expected value is missing`) | |
| const guarded = expectAssertionFailure(() => Promise.reject(runtimeError), { | |
| message: /expected/, | |
| }) | |
| await expect(guarded()).rejects.toBeInstanceOf(Error) | |
| }) | |
| }) | |
| it(`rejects runtime errors that happen to have the expected message`, async () => { | |
| const runtimeError = new TypeError(`expected value is missing`) | |
| const guarded = expectAssertionFailure(() => Promise.reject(runtimeError), { | |
| message: /expected/, | |
| }) | |
| await expect(guarded()).rejects.toBeInstanceOf(Error) | |
| }) | |
| it(`rejects an assertion that unexpectedly passes`, async () => { | |
| const guardedCheckpoint = expectAssertionFailure( | |
| () => Promise.resolve(), | |
| { checkpoint: 2 }, | |
| ) | |
| const guardedMessage = expectAssertionFailure(() => Promise.resolve(), { | |
| message: /expected/, | |
| }) | |
| await expect(guardedCheckpoint()).rejects.toBeInstanceOf(Error) | |
| await expect(guardedMessage()).rejects.toBeInstanceOf(Error) | |
| }) | |
| }) |
🤖 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/expected-failure.test.ts` around lines 58 - 66, Extend the
expected-failure tests around expectAssertionFailure to cover guarded assertions
whose underlying promise resolves, including both the checkpoint and message
option branches. Assert that the guard itself rejects when the wrapped assertion
resolves, while preserving the existing runtime-error message collision case.
Source: Learnings
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: |
Contributor
|
Size Change: 0 B Total Size: 128 kB ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 3.81 kB ℹ️ View Unchanged
|
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds deterministic state and temporal oracle coverage for nested includes. It does not change production behavior; instead, it records known failures precisely so later fixes can proceed in small, independently verified PRs.
Approach
Key invariants
loadSubsetcalls.Non-goals
Trade-offs
The tests use small deterministic run counts and fixed seeds. This keeps CI stable and failures reproducible while retaining generated histories around guaranteed visible transitions.
Verification
Focused result: 45 tests passed.
Files changed
packages/db/tests/query/includes-oracle.property.test.ts— expands the visible relationship matrix and pins all known state failures to exact checkpoints.packages/db/tests/query/includes-temporal-oracle.test.ts— adds readiness, progressive fast-path, and obsolete-demand drivers.packages/db/tests/trace-runner.ts— labels assertion failures with their trace checkpoint while preserving non-assertion errors.packages/db/tests/trace-runner.test.ts— covers checkpoint labeling, cleanup suppression, and error boundaries.packages/db/tests/expected-failure.ts— centralizes assertion-specific expected-failure guards.packages/db/tests/expected-failure.test.ts— proves those guards reject wrong checkpoints and runtime failures.Refs #1658
Refs #1510
Refs #1533
Summary by CodeRabbit
New Features
Tests