test(db): extract reusable trace runner - #1718
Conversation
📝 WalkthroughWalkthroughThe PR adds a typed asynchronous trace runner with checkpoint and cleanup handling. Structural and materialization property-test scenarios now use trace drivers and projections. Tests cover lifecycle ordering, synchronous checkpoints, type constraints, and cleanup failures. ChangesTrace runner property tests
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant runTrace
participant TraceDriver
participant TraceProjection
runTrace->>TraceDriver: setup()
runTrace->>TraceProjection: observe() and recompute() state
runTrace->>TraceProjection: assertEqual()
runTrace->>TraceDriver: apply(step, checkpoint)
TraceDriver-->>runTrace: step completed
runTrace->>TraceProjection: assertEqual() after checkpoint
runTrace->>TraceDriver: cleanup()
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/db/tests/query/includes-oracle.property.test.ts (2)
945-960: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: build the increment steps with
filterand spread.The second loop repeats the
leaf-1/leaf-2test and the id mapping. An array-method form states the intent in one pass.♻️ Proposed refactor
function createMaterializeTraceSteps( insertOrder: Array<MaterializeInsert>, ): Array<MaterializeTraceStep> { - const steps: Array<MaterializeTraceStep> = insertOrder.map((insert) => ({ - type: `insert`, - insert, - })) - - for (const insert of insertOrder) { - if (insert === `leaf-1` || insert === `leaf-2`) { - steps.push({ type: `incrementLeaf`, id: insert === `leaf-1` ? 1 : 2 }) - } - } - - return steps + const leafId = (insert: MaterializeInsert): number => + insert === `leaf-1` ? 1 : 2 + + return [ + ...insertOrder.map( + (insert): MaterializeTraceStep => ({ type: `insert`, insert }), + ), + ...insertOrder + .filter((insert) => insert === `leaf-1` || insert === `leaf-2`) + .map((insert): MaterializeTraceStep => ({ + type: `incrementLeaf`, + id: leafId(insert), + })), + ] }As per coding guidelines: "Use array methods like
filter()instead of manual loops for array transformations" and "Use the spread operator for combining arrays instead of manual loops with push".🤖 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 945 - 960, Refactor createMaterializeTraceSteps to build incrementLeaf steps with filter/map and combine them with the initial insert steps using spread, removing the manual loop and push while preserving the existing leaf-to-ID mapping.Source: Coding guidelines
777-787: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth projections type the observed value as
unknown.assertEqualthen compares an untyped value against a precisely typed expectation, so a shape drift between the live query result and the oracle result no longer fails type checking. IfstripVirtualPropertiesreturns a typed value, pass that type instead ofunknown. If it returns a loose type, add a small typed wrapper for the observed shape.
packages/db/tests/query/includes-oracle.property.test.ts#L777-L787: replace theunknowntype argument ofstructuralProjectionwith the observed structural shape, for exampleArray<OracleNode>.packages/db/tests/query/includes-oracle.property.test.ts#L1000-L1016: replace theunknowntype argument ofmaterializeProjectionwith the observed materialization shape, for exampleArray<MaterializeTree>.As per coding guidelines: "Always provide the most precise return type annotation; avoid
unknownoranyreturn types unless truly necessary".🤖 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 777 - 787, Replace the unknown observed-type arguments in both projections with their precise observed shapes: use Array<OracleNode> for structuralProjection at packages/db/tests/query/includes-oracle.property.test.ts:777-787 and Array<MaterializeTree> for materializeProjection at packages/db/tests/query/includes-oracle.property.test.ts:1000-1016, preserving the existing observation and equality logic.Source: Coding guidelines
packages/db/tests/trace-runner.ts (1)
44-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider preserving the original failure if
cleanupalso throws.If a checkpoint assertion fails and
driver.cleanupthen rejects, the cleanup error replaces the assertion error. The oracle drivers callincremental.cleanup()andcleanupSources(sources), which can reject after a failed step. The property test then reports a teardown error instead of the mismatch that caused it.♻️ Proposed change to keep the primary error
try { await driver.start?.(context) checkpoint() for (const step of steps) { await driver.apply(step, context, checkpoint) checkpoint() } + } catch (error) { + try { + await driver.cleanup(context) + } catch { + // Keep the original trace failure as the reported error. + } + throw error } finally { - await driver.cleanup(context) + // cleanup already ran on the error path }A simpler variant is a
let failed = falseflag, orPromise.allSettled-style handling, if you prefer to keep a singlefinally.🤖 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/trace-runner.ts` around lines 44 - 54, Update the trace runner’s try/finally flow around driver.start, step execution, and checkpoint so cleanup failures do not replace the original execution or assertion error. Preserve the primary failure, while still awaiting driver.cleanup(context) and surfacing its error when no earlier failure occurred.packages/db/tests/trace-runner.test.ts (1)
45-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd corner-case tests for the runner lifecycle.
The current suite covers the happy path and a failing checkpoint. Three cases remain untested, and all three are reachable through the oracle drivers:
steps: []. The runner must still check after startup and callcleanuponce.- An async driver.
setup,start,apply, andcleanupeach may return a promise. Assert that the runner awaits them in order.driver.applythrowing. Assert thatcleanupstill runs.Note also that
applyon Line 53 never executes in this test, because the startup checkpoint throws first.💚 Proposed additional tests
it(`cleans up when a checkpoint fails`, async () => {it(`checks after startup and cleans up for an empty trace`, async () => { const checkpoints: Array<number> = [] const cleanup = vi.fn() await runTrace({ steps: [], driver: { setup: () => ({ observed: 0, expected: 0 }), apply: () => undefined, cleanup, }, projection: { observe: (context) => context.observed, recompute: (context) => context.expected, assertEqual: (observed, expected) => { expect(observed).toBe(expected) checkpoints.push(observed) }, }, }) expect(checkpoints).toEqual([0]) expect(cleanup).toHaveBeenCalledOnce() }) it(`awaits an asynchronous driver in order`, async () => { const calls: Array<string> = [] const tick = async () => { await Promise.resolve() } await runTrace({ steps: [1], driver: { setup: async () => { await tick() calls.push(`setup`) return { observed: 0, expected: 0 } }, start: async () => { await tick() calls.push(`start`) }, apply: async (step, context) => { await tick() context.observed += step context.expected += step calls.push(`apply`) }, cleanup: async () => { await tick() calls.push(`cleanup`) }, }, projection: { observe: (context) => context.observed, recompute: (context) => context.expected, assertEqual: (observed, expected) => { expect(observed).toBe(expected) calls.push(`check`) }, }, }) expect(calls).toEqual([`setup`, `start`, `check`, `apply`, `check`, `cleanup`]) }) it(`cleans up when apply throws`, async () => { const cleanup = vi.fn() await expect( runTrace({ steps: [1], driver: { setup: () => ({ observed: 0, expected: 0 }), apply: () => { throw new Error(`apply failed`) }, cleanup, }, projection: { observe: (context) => context.observed, recompute: (context) => context.expected, assertEqual: (observed, expected) => { expect(observed).toBe(expected) }, }, }), ).rejects.toThrow(`apply failed`) expect(cleanup).toHaveBeenCalledOnce() })As per coding guidelines: "Test corner cases including: empty arrays/sets, single-element collections, undefined vs null values, resolved promises, async race conditions, and limit/offset edge cases".
🤖 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/trace-runner.test.ts` around lines 45 - 67, Add the three missing lifecycle tests alongside the existing trace-runner tests: verify empty steps perform the startup checkpoint and call cleanup once, verify async setup/start/apply/cleanup methods are awaited in the order setup, start, check, apply, check, cleanup, and verify cleanup runs when driver.apply throws while preserving the original error. Use the existing runTrace, driver, and projection symbols and assert the specified checkpoint, call-order, and cleanup behavior.Sources: Coding guidelines, Learnings
🤖 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.
Nitpick comments:
In `@packages/db/tests/query/includes-oracle.property.test.ts`:
- Around line 945-960: Refactor createMaterializeTraceSteps to build
incrementLeaf steps with filter/map and combine them with the initial insert
steps using spread, removing the manual loop and push while preserving the
existing leaf-to-ID mapping.
- Around line 777-787: Replace the unknown observed-type arguments in both
projections with their precise observed shapes: use Array<OracleNode> for
structuralProjection at
packages/db/tests/query/includes-oracle.property.test.ts:777-787 and
Array<MaterializeTree> for materializeProjection at
packages/db/tests/query/includes-oracle.property.test.ts:1000-1016, preserving
the existing observation and equality logic.
In `@packages/db/tests/trace-runner.test.ts`:
- Around line 45-67: Add the three missing lifecycle tests alongside the
existing trace-runner tests: verify empty steps perform the startup checkpoint
and call cleanup once, verify async setup/start/apply/cleanup methods are
awaited in the order setup, start, check, apply, check, cleanup, and verify
cleanup runs when driver.apply throws while preserving the original error. Use
the existing runTrace, driver, and projection symbols and assert the specified
checkpoint, call-order, and cleanup behavior.
In `@packages/db/tests/trace-runner.ts`:
- Around line 44-54: Update the trace runner’s try/finally flow around
driver.start, step execution, and checkpoint so cleanup failures do not replace
the original execution or assertion error. Preserve the primary failure, while
still awaiting driver.cleanup(context) and surfacing its error when no earlier
failure occurred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2723ea87-aec6-4372-8a73-8a03f2c8f117
📒 Files selected for processing (3)
packages/db/tests/query/includes-oracle.property.test.tspackages/db/tests/trace-runner.test.tspackages/db/tests/trace-runner.ts
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: 128 kB ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 3.81 kB ℹ️ View Unchanged
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/db/tests/query/includes-oracle.property.test.ts (2)
1143-1149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
itinstead offcTestfor this fixed trace.
fullRowBatchTraceis a fixed literal array. This case takes no fast-check arbitraries, so it is an example-based test, not a property test. Usingitstates the intent more precisely and avoids the fast-check runner overhead.🤖 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 1143 - 1149, Change the test declaration for the fixed fullRowBatchTrace case from fcTest to it. Keep the existing runTrace call, driver, projection, and assertions unchanged.
776-782: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuarantee source cleanup when the live query cleanup fails.
cleanupStructuralTraceawaitsincremental.cleanup()first. If that call rejects,cleanupSources(sources)never runs and the five source collections stay alive. A property test executes this path many times, so the leak accumulates.♻️ Proposed fix
async function cleanupStructuralTrace({ incremental, sources, }: StructuralTraceContext): Promise<void> { - await incremental.cleanup() - await cleanupSources(sources) + try { + await incremental.cleanup() + } finally { + await cleanupSources(sources) + } }🤖 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 776 - 782, Update cleanupStructuralTrace so cleanupSources(sources) always runs even when incremental.cleanup() rejects, using guaranteed cleanup control flow such as a finally block. Preserve the existing cleanup order by attempting incremental.cleanup() first, then source cleanup, while allowing the original failure to propagate.packages/db/tests/trace-runner.test.ts (1)
122-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for asynchronous driver results.
All drivers in this file return synchronously. The
isPromiseLikebranches inrunTraceforsetup,start,apply, andcleanupare never exercised. Add one test with a rejected asynccleanupand one test with a non-Promisethenablesetup. This pins the thenable detection that the runner advertises.Based on learnings, tests should cover corner cases including "resolved promises" and "async race conditions".
🧪 Proposed additional tests
it(`rejects with an asynchronous cleanup failure`, async () => { const cleanupError = new Error(`cleanup failed`) await expect( runTrace({ steps: [], driver: { setup: () => ({ observed: 0, expected: 0 }), apply: () => undefined, cleanup: () => Promise.reject(cleanupError), }, projection: { observe: (context) => context.observed, recompute: (context) => context.expected, assertEqual: (observed, expected) => { expect(observed).toBe(expected) }, }, }), ).rejects.toBe(cleanupError) }) it(`awaits a thenable setup result`, async () => { const context = { observed: 1, expected: 1 } await expect( runTrace({ steps: [], driver: { setup: () => ({ then: (resolve: (value: typeof context) => void) => resolve(context), }), apply: () => undefined, cleanup: () => undefined, }, projection: { observe: (value) => value.observed, recompute: (value) => value.expected, assertEqual: (observed, expected) => { expect(observed).toBe(expected) }, }, }), ).resolves.toBeUndefined() })🤖 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/trace-runner.test.ts` around lines 122 - 144, Add coverage in the trace-runner tests for asynchronous driver results: add a test where cleanup returns a rejected Promise and assert runTrace rejects with that error, then add a test where setup returns a non-Promise thenable resolving to a valid context and assert runTrace resolves successfully. Keep the existing synchronous cleanup-failure test and exercise the isPromiseLike handling in runTrace.Source: Learnings
🤖 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.
Nitpick comments:
In `@packages/db/tests/query/includes-oracle.property.test.ts`:
- Around line 1143-1149: Change the test declaration for the fixed
fullRowBatchTrace case from fcTest to it. Keep the existing runTrace call,
driver, projection, and assertions unchanged.
- Around line 776-782: Update cleanupStructuralTrace so cleanupSources(sources)
always runs even when incremental.cleanup() rejects, using guaranteed cleanup
control flow such as a finally block. Preserve the existing cleanup order by
attempting incremental.cleanup() first, then source cleanup, while allowing the
original failure to propagate.
In `@packages/db/tests/trace-runner.test.ts`:
- Around line 122-144: Add coverage in the trace-runner tests for asynchronous
driver results: add a test where cleanup returns a rejected Promise and assert
runTrace rejects with that error, then add a test where setup returns a
non-Promise thenable resolving to a valid context and assert runTrace resolves
successfully. Keep the existing synchronous cleanup-failure test and exercise
the isPromiseLike handling in runTrace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b4d57f3-e19d-4a85-8cb3-caf5b7c19ece
📒 Files selected for processing (4)
packages/db/tests/query/includes-oracle.property.test.tspackages/db/tests/trace-runner.test-d.tspackages/db/tests/trace-runner.test.tspackages/db/tests/trace-runner.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/db/tests/query/includes-oracle.property.test.ts (2)
892-901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse precise observed types for both projections.
structuralProjectionandmaterializeProjectionuseunknownforTObserved. This removes compile-time checking between the live query shape and the oracle shape. UseArray<OracleNode>andArray<MaterializeTree>, and type or narrowstripVirtualPropertiesat the observation boundary. (raw.githubusercontent.com)Proposed type refinement
const structuralProjection: TraceProjection< StructuralTraceContext, - unknown, + Array<OracleNode>, Array<OracleNode> > = { const materializeProjection: TraceProjection< MaterializeTraceContext, - unknown, + Array<MaterializeTree>, Array<MaterializeTree> > = {As per coding guidelines: “Always provide the most precise return type annotation; avoid
unknownoranyreturn types unless truly necessary.”Also applies to: 1122-1139
🤖 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 892 - 901, Replace the unknown observed types in both structuralProjection and materializeProjection with Array<OracleNode> and Array<MaterializeTree>, respectively. Update or narrow stripVirtualProperties at the observation boundary so its result conforms to the precise projection type while preserving the existing observation behavior and type checking.Source: Coding guidelines
1059-1075: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the manual loop and
pushwith array methods.
createMaterializeTraceStepsmaps insert steps, then scansinsertOrderagain and mutatesstepswithpush. Build the leaf steps withfilter().map()and combine both arrays with spread. (raw.githubusercontent.com)Proposed refactor
function createMaterializeTraceSteps( insertOrder: Array<MaterializeInsert>, ): Array<MaterializeTraceStep> { - const steps: Array<MaterializeTraceStep> = insertOrder.map((insert) => ({ - type: `insert`, - insert, - })) + const leafId = (insert: MaterializeInsert): number => + insert === `leaf-1` ? 1 : 2 - for (const insert of insertOrder) { - if (insert === `leaf-1` || insert === `leaf-2`) { - steps.push({ type: `incrementLeaf`, id: insert === `leaf-1` ? 1 : 2 }) - } - } - - return steps + return [ + ...insertOrder.map( + (insert): MaterializeTraceStep => ({ type: `insert`, insert }), + ), + ...insertOrder + .filter((insert) => insert === `leaf-1` || insert === `leaf-2`) + .map((insert): MaterializeTraceStep => ({ + type: `incrementLeaf`, + id: leafId(insert), + })), + ] }As per coding guidelines: “Use array methods like
filter()instead of manual loops for array transformations” and “Use the spread operator for combining arrays instead of manual loops with push.”🤖 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 1059 - 1075, Refactor createMaterializeTraceSteps to avoid the manual loop and mutation: keep the existing insert-step map, derive incrementLeaf steps from insertOrder with filter().map(), and return the two arrays combined via spread while preserving the current leaf-1/leaf-2 IDs and ordering.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.
Nitpick comments:
In `@packages/db/tests/query/includes-oracle.property.test.ts`:
- Around line 892-901: Replace the unknown observed types in both
structuralProjection and materializeProjection with Array<OracleNode> and
Array<MaterializeTree>, respectively. Update or narrow stripVirtualProperties at
the observation boundary so its result conforms to the precise projection type
while preserving the existing observation behavior and type checking.
- Around line 1059-1075: Refactor createMaterializeTraceSteps to avoid the
manual loop and mutation: keep the existing insert-step map, derive
incrementLeaf steps from insertOrder with filter().map(), and return the two
arrays combined via spread while preserving the current leaf-1/leaf-2 IDs and
ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea388cbb-542e-400b-80e0-9b5bf0a54a94
📒 Files selected for processing (1)
packages/db/tests/query/includes-oracle.property.test.ts
Extract the includes recompute-oracle loop into a reusable trace runner, then validate the driver/projection boundary with structural histories, scalar materialization, and full-row sync batches. This is test-only, and the expanded scalar driver has already exposed a new deterministic incremental-materialization bug.
Reviewer guidance
Root cause
The includes oracle had two hand-written execution loops. That made each new mutation or lifecycle domain repeat setup, checkpoint, and cleanup behavior instead of sharing one contract.
The first extracted runner also awaited every hook unconditionally. Awaiting a synchronous
applyyields to the microtask queue before the checkpoint, which could hide stale state repaired by a queued microtask. Its cleanup path could replace the useful trace failure, andassertEqual: () => voidallowed an async assertion whose promise the runner ignored.Approach
undefined; a type test rejects async assertions.middle.sharedIdupdates the visible key but leaves the old nested shared row materialized.Key invariants
Non-goals
Trade-offs
Assertions remain synchronous to preserve exact checkpoint timing. The batch driver uses a short deterministic trace: it is enough to test the abstraction with a distinct mutation model while leaving the larger fuzz-domain expansion for later RFC slices. The new bug stays as an assertion-specific expected failure so this test-infrastructure PR remains green without hiding unrelated failures.
Verification
From
packages/db:git diff --checkpass.Files changed
packages/db/tests/trace-runner.ts: adds the shared runner and its timing and error-handling guarantees.packages/db/tests/trace-runner.test.ts: tests checkpoints, same-turn timing, and cleanup failure precedence.packages/db/tests/trace-runner.test-d.ts: proves async projection assertions are rejected.packages/db/tests/query/includes-oracle.property.test.ts: migrates the existing oracle drivers, adds the full-row batch driver, and records the nested-reference failure.Release impact
Related work: RFC #1658, #1716, #1717.
Summary by CodeRabbit