test(db): fuzz visible includes relationship transitions - #1722
Open
KyleAMathews wants to merge 4 commits into
Open
test(db): fuzz visible includes relationship transitions#1722KyleAMathews wants to merge 4 commits into
KyleAMathews wants to merge 4 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe includes-oracle property tests now model configurable branches and relationship transitions at arbitrary depths. Full-row normalization preserves existing child relationships. Deterministic and property-based cases cover reparenting, rekeying, scalar updates, and expected deep-transition failures. ChangesNested relationship tests
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This test-only change does not alter runtime behavior, and no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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.
🧹 Nitpick comments (2)
packages/db/tests/query/includes-oracle.property.test.ts (2)
1272-1281: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGenerate the empty-noise case.
noiseusesminLength: 1, so the scenario with no scalar updates around the transition is never generated. That case is the simplest and most valuable shrink target. SetminLength: 0.Based on the retrieved learning "Test corner cases including: empty arrays/sets, single-element collections, undefined vs null values, resolved promises, async race conditions, and limit/offset edge cases".
♻️ Proposed change
- { minLength: 1, maxLength: 10 }, + { minLength: 0, maxLength: 10 },🤖 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 1272 - 1281, Update the noise array arbitrary in the property test to use minLength: 0 instead of minLength: 1, while preserving its existing maxLength and record generation so empty-noise transition cases are generated and shrinkable.Source: Learnings
1857-1897: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake seeds unique per combination and document the failure heuristic.
Two problems in this loop:
seed: 1721 + depth + targetLevelcollides across combinations.reparentandrekeyshare the same seed for the same(depth, targetLevel), and different pairs with equal sums collide too (depth 2 / level 2 and depth 3 / level 1 both give 1725). WithnumRuns: 4, the generated corpus repeats instead of widening coverage. Derive the seed from all three inputs.expectsFailure = transition === 'rekey' && targetLevel + 2 <= depthencodes a known defect without stating why. Add a short comment that a rekey disconnects descendants two or more levels below the changed row, so a reader knows what to delete when the defect is fixed.♻️ Proposed change
for (let targetLevel = 1; targetLevel <= depth; targetLevel++) { + // A rekey changes the parent key of the target row only. Descendants + // two or more levels below keep the stale `parentGroup`, which the + // incremental path currently fails to detach. const expectsFailure = transition === `rekey` && targetLevel + 2 <= depth fcTest.prop( @@ { numRuns: 4, - seed: 1721 + depth + targetLevel, + seed: + 1721 + + depth * 100 + + targetLevel * 10 + + (transition === `rekey` ? 1 : 0), },🤖 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 1857 - 1897, Update the property-test loop around expectsFailure so the seed incorporates transition, depth, and targetLevel, producing distinct seeds for every combination rather than relying only on numeric inputs. Add a brief comment beside the expectsFailure heuristic explaining that rekey disconnects descendants two or more levels below the changed row, and retain the existing failure behavior until that defect is fixed.
🤖 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 1272-1281: Update the noise array arbitrary in the property test
to use minLength: 0 instead of minLength: 1, while preserving its existing
maxLength and record generation so empty-noise transition cases are generated
and shrinkable.
- Around line 1857-1897: Update the property-test loop around expectsFailure so
the seed incorporates transition, depth, and targetLevel, producing distinct
seeds for every combination rather than relying only on numeric inputs. Add a
brief comment beside the expectsFailure heuristic explaining that rekey
disconnects descendants two or more levels below the changed row, and retain the
existing failure behavior until that defect is fixed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6077b4b5-a714-4fbd-b510-cab141cfa9ff
📒 Files selected for processing (1)
packages/db/tests/query/includes-oracle.property.test.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: |
Contributor
|
Size Change: 0 B Total Size: 132 kB ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 3.79 kB ℹ️ View Unchanged
|
12 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/db/tests/query/includes-oracle.property.test.ts (1)
1304-1314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit return type.
scalarNoiseArbitraryhas no return type annotation. Add: fc.Arbitrary<VisibleScalarNoise>.As per coding guidelines, “Always provide the most precise return type annotation.”
🤖 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 1304 - 1314, Update the scalarNoiseArbitrary helper’s declaration to explicitly return fc.Arbitrary<VisibleScalarNoise>, while preserving its existing parameters and generated record behavior.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 1897-1911: Update the
rejects-overlapping-visible-relationship-keys fixture in fcTest to use disjoint
idBase values while retaining colliding groupBase values, so it specifically
exercises relationship-group overlap. Add a separate case where only rekeyGroup
collides with an existing group, ensuring ID keys remain disjoint.
---
Nitpick comments:
In `@packages/db/tests/query/includes-oracle.property.test.ts`:
- Around line 1304-1314: Update the scalarNoiseArbitrary helper’s declaration to
explicitly return fc.Arbitrary<VisibleScalarNoise>, while preserving its
existing parameters and generated record behavior.
🪄 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: 8b01d34a-d5b2-4289-a867-45d7bc077cdb
📒 Files selected for processing (1)
packages/db/tests/query/includes-oracle.property.test.ts
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
1 participant
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 state-aware property scenarios for visible
includesrelationship transitions across query depths and target levels. It expands recompute-oracle coverage and pins known divergences; it does not change runtime behavior or fix the bugs it exposes.Approach
Known divergence boundaries
targetLevel + 2 <= depth.Each expected failure is pinned to the exact checkpoint. If the implementation stops failing there, the test fails and prompts us to turn the trace into a passing regression.
Key invariants
Non-goals
Trade-offs
The matrix enumerates all depth/target-level combinations and uses a small deterministic fuzz sample within each pair. This gives structural coverage without making the suite slow or leaving rare target levels to chance.
Verification
Focused oracle tests: 40 passed. Package suite: 2,620 passed and 5 skipped.
Files changed
packages/db/tests/query/includes-oracle.property.test.ts— adds the state-aware relationship scenario builder, generated depth/level matrix, exact expected-failure boundaries, and the newly found moved-subtree trace.Refs #1658
Summary by CodeRabbit