test(db): add includes recompute oracle - #1716
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds property tests for nested ChangesNested includes testing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 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 (6)
packages/db/tests/query/includes-oracle.property.test.ts (6)
891-901: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueGenerate the non-shared scenario directly instead of filtering.
materializeScenarioArbitrarychains fromfc.boolean(). The filter rejects about half of the generated values, so fast-check discards runs to reachnumRuns: 30.Extract the insert-order arbitrary into a function that takes
sharedIntermediate, then call it withfalsehere.🤖 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 891 - 901, Update the materialize scenario arbitrary construction by extracting the insert-order arbitrary into a function accepting sharedIntermediate, then build this property’s input with false directly instead of filtering materializeScenarioArbitrary on !sharedIntermediate. Preserve the existing numRuns and expectMaterializeScenarioMatches behavior.
943-956: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a root-level
orderByto make the comparisons order-independent.None of the four
queryOncecalls order the root rows.toEqualon arrays is order-sensitive. The property then depends on unspecified root iteration order being identical across four separate query executions.Add
.orderBy(({ parent }) => parent.id)to each variant, or sort the results before comparison.🤖 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 943 - 956, Add a deterministic root-level order to all four queryOnce variants, using parent.id via orderBy before executing each query, so their array results compare independently of unspecified iteration order. Keep the existing child ordering and query projections unchanged.
291-504: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReduce duplication between the four depth variants.
The four cases repeat the same nested shape. Depth 1 to depth 3 are prefixes of depth 4. The oracle comparison is only meaningful while all four variants stay identical except for depth. Manual duplication lets one variant drift and silently weaken the property.
Extract the repeated child sub-query into a helper, or build the nesting from an array of level collections. If the builder typing blocks a fully generic version, extract at least the deepest three levels into named helpers.
As per coding guidelines: "Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places" and "Extract logical sections from massive functions into separate functions to improve readability and maintainability".
🤖 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 291 - 504, Reduce duplication in createIncrementalQuery by extracting the repeated nested child-query construction into reusable helpers or by building the nesting from the level collections array. Ensure depths 1–4 remain identical prefixes of the depth-4 shape, preserving each level’s filtering, ordering, and selected fields; if generic typing prevents full extraction, at minimum name and reuse helpers for the deepest three levels.Source: Coding guidelines
1113-1168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider asserting the expected failure mode of each red seed.
fcTest.failspasses when the body throws for any reason. A typo, a renamed export, or a cleanup error also makes these tests green. The seeds then stop tracking the specific bugs in#1444,#1454,#1685, and the discovered correlation-key bug.Consider replacing
.failswith a normal test that asserts the current wrong result, plus a comment that names the expected correct result. That form fails loudly when behavior changes in either direction.Also applies to: 1170-1242, 1244-1310
🤖 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 1113 - 1168, The red-seed tests using fcTest.fails should explicitly assert their current incorrect results instead of passing on any thrown error. Update the correlation-key test and the analogous seed cases to use normal property tests, assert the observed wrong output, and add comments documenting the expected correct output so regressions, unrelated failures, and fixes in either direction are detected.
568-577: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrelation keys are pinned at row creation in both branches of
applyAction. Both branches reusecurrent?.groupandcurrent?.parentGroupfor every action on an existing row, so the generated histories never change a join key. The main property therefore cannot reach the correlation-key update bug that the seed at lines 1113-1168 documents, and nothing in the code states this exclusion.
packages/db/tests/query/includes-oracle.property.test.ts#L568-L577: add a comment above thenextliteral that statesgroupstays fixed on purpose and links the known correlation-key bug.packages/db/tests/query/includes-oracle.property.test.ts#L626-L636: add the same comment forparentGroupandgroup.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-oracle.property.test.ts` around lines 568 - 577, The generated histories intentionally keep correlation keys unchanged, so document this exclusion with comments above both action-result literals in applyAction: at packages/db/tests/query/includes-oracle.property.test.ts lines 568-577, explain that group remains fixed and reference the known correlation-key bug; at lines 626-636, add the corresponding comment explaining that parentGroup and group remain fixed and reference the same bug.Source: Coding guidelines
1105-1116: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the decorative
seedoptions. These properties usefc.constant(...), so the seed cannot change the generated input. Move issue references to the test names or comments.🤖 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 1105 - 1116, Remove the decorative seed options from the two fcTest.fails.prop cases using fc.constant, including the tests for sharedMaterializeSeed and correlation-key-update. Preserve the single-run configuration, and move each issue reference into its test name or an adjacent comment.
🤖 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 1286-1291: Settle or explicitly catch the optimistic transaction
returned by children.collection.update before the test cleanup runs. Follow the
existing convergence test pattern by resolving or rejecting the sync promise and
awaiting transaction.isPersisted.promise, or attach a catch handler to that
promise before finally invokes cleanup().
---
Nitpick comments:
In `@packages/db/tests/query/includes-oracle.property.test.ts`:
- Around line 891-901: Update the materialize scenario arbitrary construction by
extracting the insert-order arbitrary into a function accepting
sharedIntermediate, then build this property’s input with false directly instead
of filtering materializeScenarioArbitrary on !sharedIntermediate. Preserve the
existing numRuns and expectMaterializeScenarioMatches behavior.
- Around line 943-956: Add a deterministic root-level order to all four
queryOnce variants, using parent.id via orderBy before executing each query, so
their array results compare independently of unspecified iteration order. Keep
the existing child ordering and query projections unchanged.
- Around line 291-504: Reduce duplication in createIncrementalQuery by
extracting the repeated nested child-query construction into reusable helpers or
by building the nesting from the level collections array. Ensure depths 1–4
remain identical prefixes of the depth-4 shape, preserving each level’s
filtering, ordering, and selected fields; if generic typing prevents full
extraction, at minimum name and reuse helpers for the deepest three levels.
- Around line 1113-1168: The red-seed tests using fcTest.fails should explicitly
assert their current incorrect results instead of passing on any thrown error.
Update the correlation-key test and the analogous seed cases to use normal
property tests, assert the observed wrong output, and add comments documenting
the expected correct output so regressions, unrelated failures, and fixes in
either direction are detected.
- Around line 568-577: The generated histories intentionally keep correlation
keys unchanged, so document this exclusion with comments above both
action-result literals in applyAction: at
packages/db/tests/query/includes-oracle.property.test.ts lines 568-577, explain
that group remains fixed and reference the known correlation-key bug; at lines
626-636, add the corresponding comment explaining that parentGroup and group
remain fixed and reference the same bug.
- Around line 1105-1116: Remove the decorative seed options from the two
fcTest.fails.prop cases using fc.constant, including the tests for
sharedMaterializeSeed and correlation-key-update. Preserve the single-run
configuration, and move each issue reference into its test name or an adjacent
comment.
🪄 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: 0220438a-403c-48d8-85d8-90b0603f0af8
📒 Files selected for processing (1)
packages/db/tests/query/includes-oracle.property.test.ts
| await live.preload() | ||
| children.collection.update([1, 2], (drafts) => { | ||
| drafts[0]!.position = 1 | ||
| drafts[1]!.position = 0 | ||
| }) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Settle or catch the optimistic transaction before cleanup.
children.collection.update([1, 2], ...) starts an optimistic mutation. The mock onUpdate awaits a sync promise. This test never calls resolveSync or rejectSync, and never awaits or catches transaction.isPersisted.promise. The finally block then calls cleanup() while the mutation is still in flight.
A rejection raised during cleanup has no handler and surfaces as an unhandled rejection, which can fail unrelated tests in the same run. The convergence test at lines 1073-1088 settles its transaction; follow the same pattern here, or attach .catch(() => undefined) to transaction.isPersisted.promise.
🛡️ Proposed fix
- children.collection.update([1, 2], (drafts) => {
- drafts[0]!.position = 1
- drafts[1]!.position = 0
- })
+ const transaction = children.collection.update([1, 2], (drafts) => {
+ drafts[0]!.position = 1
+ drafts[1]!.position = 0
+ })
+ // The sync is never settled here; the assertion below only inspects
+ // the optimistic state, so the pending promise must not go unhandled.
+ transaction.isPersisted.promise.catch(() => undefined)📝 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.
| await live.preload() | |
| children.collection.update([1, 2], (drafts) => { | |
| drafts[0]!.position = 1 | |
| drafts[1]!.position = 0 | |
| }) | |
| await live.preload() | |
| const transaction = children.collection.update([1, 2], (drafts) => { | |
| drafts[0]!.position = 1 | |
| drafts[1]!.position = 0 | |
| }) | |
| // The sync is never settled here; the assertion below only inspects | |
| // the optimistic state, so the pending promise must not go unhandled. | |
| transaction.isPersisted.promise.catch(() => undefined) | |
🤖 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 1286 -
1291, Settle or explicitly catch the optimistic transaction returned by
children.collection.update before the test cleanup runs. Follow the existing
convergence test pattern by resolving or rejecting the sync promise and awaiting
transaction.isPersisted.promise, or attach a catch handler to that promise
before finally invokes cleanup().
2 participants
Adds a property-based recompute oracle for nested
includequeries without changing runtime behavior. It gives laterincluderefactors a small, independent test harness that checks incremental results against full recomputation after each change.Approach
fast-checkhistories across one to four nestedtoArraylevels, including puts, deletes, optimistic confirms, and optimistic rollbacks.materialize(findOne()), optimistic convergence, and metamorphic invariants for alias renaming, sibling order, and unrelated siblings.materializeis dropped when the middle level is shared by more than one parent row #1685, plus newly found parent correlation-key update and confirmed child reorder bugs.Key invariants
.failstest so the seed must be promoted to a normal regression test.Non-goals
Trade-offs
The test spells out the depth-specific query shapes so TypeScript checks the real public query API at each depth. This adds some test duplication, but keeps the oracle independent, type-safe, and easy to shrink and replay.
Verification
All checks pass:
Files changed
packages/db/tests/query/includes-oracle.property.test.ts— adds the recompute model, generated histories, metamorphic checks, scalar materialization coverage, and expected-failure seeds.Related RFC: #1658
Known bug seeds: #1444, #1454, #1685
Summary by CodeRabbit