test(query): three conjunctions no fixture could tell from their halves - #3131
Conversation
…overlap Mutation sweep of packages/spatial and packages/query. QueryBuilder's ofType()/withProperty() each build currentFilter as `previousFilter(entity) && <own check>`, but no existing fixture ever had an entity that satisfied the new predicate while failing the prior one — so dropping `previousFilter` from any of the three call sites (ofType, withProperty-with-value, withProperty-without-value) left the full suite green. Added a fixture where a door shares a wall's exact pset/property/value, and a two-ofType() chain, so each conjunction is now independently observable. packages/spatial was already mutation-swept (#2147, #2158, #2312, #2764) — confirmed by running its existing bvh.test.ts suite against a hand-mutated tmax >= 0 -> tmax > 0 and watching it fail. No changes made there.
|
Warning Review limit reachedNext included review available in 35 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
Viewer benchmark✅ No threshold regressions detected. 01_Snowdon_Towers_Sample_Structural(1).ifcBaseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
AC20-FZK-Haus.ifcBaseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.
Refresh the baseline from a CI run: dispatch the Benchmark workflow with |
Mutation-swept
packages/spatialandpackages/query. Test-only. One real gap inquery;spatialturned out to be already hardened and is reported as such rather than padded.The gap: three conjunctions whose halves never disagreed
packages/query/src/fluent-api.ts—QueryBuilder.ofType()and both branches ofwithProperty()each rebuild the filter aspreviousFilter(entity) && <own predicate>.No existing fixture had an entity satisfying the new predicate while failing the prior one, so the conjunction was never actually exercised. Three separate mutants, each dropping the
previousFilter(entity) &&half, all passed the full 218-test baseline:ofType:this.currentFilter = (entity) => entity.type === type;withProperty(key, value): droppedif (!previousFilter(entity)) return false;withProperty(key)(existence form): same dropClassification: untested-but-correct. The source was never wrong; the suite could not tell.
The fix is a fixture where the two match sets genuinely disagree — door #3 shares wall #2's exact
Pset_WallCommon.IsExternal: false, so the property-only set{2,3}and the type-only set{1,2}differ on entity 3. The pre-existing "chain ofType and withProperty" test could never have caught this, because its two sets never diverged. Each mutant is now killed by its own dedicated test.packages/spatial: already swept, and the near-miss is worth recordingThe agent's initial worktree checkout was stale and showed zero test files for
spatial— which it nearly reported as a finding. Re-branching from a freshly fetchedupstream/mainshowedaabb.test.ts,bvh.test.tsandspatial-index-builder.test.ts, 50 tests, withgit logattributing them to four prior sweeps (#2147, #2158, #2312, #2764).That is the same class of error as the branch-drift trap: a stale checkout makes existing work invisible, and the natural conclusion is "untested" rather than "I am looking at the wrong tree."
Rather than take the hardening on trust, it spot-checked by hand: mutating
bvh.ts's ray-behind-origin boundaryreturn tmax >= 0→> 0fails exactly 1 test (bvh.test.ts:175), 49 passing. Restored, 50/50. No changes made tospatial.Negative evidence
entity-query.ts'sexecute()offset-then-limit ordering — swapping the two.slice()calls is caught byshould combine limit and offset.resolve-type-name.ts's fallback is already pinned byentity-node-unknown-type.test.ts.bvh.ts's raycast boundary, above.packages/querywas already substantially hardened too —entity-query.ts,entity-node.tsandresolve-type-name.tsall carry sweep-derived comments and fixtures referencing specific prior bugs. The fluent API was the part that had escaped.Prior-sweep check
~35 candidate branches, each diffed against its own merge-base rather than current
main. Onlyquery-diff-create-sweeptouched either package, and it is already merged (131e3dc84, #3009).Verification
packages/query218 → 221;packages/spatial50 → 50, untouched.check-module-size.mjsexit 0, ratchet not moved. No changeset — test-only, no published behaviour change.Leads not chased:
duckdb-integration.ts(507 lines) is largely worker/AsyncDuckDB plumbing that is not meaningfully unit-testable without the WASM runtime, and its pure helpers already have dedicated tests.entity-node.ts's traversal is pinned by an explicit diamond-graph regression test whose comment already names the exact mutant a plain visited-set would introduce.🤖 Generated with Claude Code