test(tools): add unit tests for annotation state, math utilities, store filters, and synchronizers#2790
Merged
Merged
Conversation
…modality camera math Adds 279 jest tests across six new test files targeting the most critical undertested areas of the GenericViewport architecture: - genericViewportBase.jest.js: GenericViewport base class binding lifecycle, mount races (stale requests, destroy mid-load, source role demotion), presentation merge/fan-out, view state propagation, camera events, destroy semantics, and genericViewportDisplaySetAccess - planarSliceBasis.jest.js: slice basis derivation for stack images and volumes across orientations, anisotropic spacing, rotated and flipped direction matrices, and image id index resolution - planarViewReference.jest.js: planar view reference build/match APIs and PlanarViewReferenceController navigation (world point to slice, opposite-normal index flip, plane-restriction reorientation) - planarRenderCamera.jest.js: resolvePlanarICamera and derivePlanarPresentation round-trips (zoom, pan, rotation, flips, display areas), renderer/actor application, clipping ranges, and PlanarVolumeResolvedView parity with the existing stack suite - modalityViewportCameras.jest.js: Video/ECG/Volume3D viewport camera math (canvas mappings, scale modes, anchor/pan round-trips) - wsiTransformUtils.jest.js: WSI world/index/canvas transforms and color transform branches Coverage over src/RenderingEngine/GenericViewport rises from 33.7% to 44.0% lines and 26.3% to 38.1% branches, with the targeted modules at 89-100% lines.
…rnerstoneImprovedTesting
…re filters, and synchronizers Adds 427 jest tests across eight new test files targeting the most critical undertested pure-logic areas of packages/tools (karma browser tests continue to own interactive tool coverage): - annotationStateManagement.jest.js: annotation state CRUD through the FrameOfReferenceSpecificAnnotationManager, save/restore round-trips, locking/selection/visibility semantics and event payloads, annotation groups, parent/child relationships - polylineMath.jest.js + polylineBooleanOps.jest.js: polyline predicates (containsPoint, winding, closedness, AABB), convex hull, decimation, segment intersection variants, robust intersection, and the intersect/subtract/merge boolean-op workhorses verified via area and point-membership properties - geometryPrimitives.jest.js: ellipse/circle/line/rectangle/aabb/ sphere/angle/point/vec2/vec3 primitives including pointInEllipse, liangBarksyClip, and distance helpers - synchronizerCore.jest.js: Synchronizer class lifecycle with real DOM event dispatch, SynchronizerManager factories, and camera/VOI/slab/ presentation sync callbacks - viewportFilters.jest.js: frame-of-reference, orientation, parallel normal, and tool-enabled render fan-out filters - storeToolFilters.jest.js: per-mouse-move tool/handle hit-test filters, cancelActiveManipulations, addTool/state registry - planarAndBounds.jest.js: annotation slice/plane display filtering, world width/height derivation, plane-AABB intersection, bounding box helpers Tools package jest coverage rises from 6.3% to 12.6% lines overall, with the targeted areas at: utilities/math 86%, annotation state 72%, viewportFilters 98%, boundingBox 100%, synchronizers 81%. Several latent product bugs were found and pinned by regression tests without changing behavior; see the test files for details (notably: manager getNumberOfAnnotations group-count dead path, missing null-guards in annotation visibility/selection, removeTool unreachable throw, getBoundingBoxAroundShapeWorld inverted clamp, boolean-op vertex-coincident corner mispairing, liangBarksyClip boundary rejection).
📝 WalkthroughWalkthroughThis PR adds eight new Jest test files under packages/tools/test covering annotation state management, store tool filters, the Synchronizer system, geometry primitives, planar/bounding-box utilities, polyline boolean operations, polyline math, and viewport filters, with no source code or public API changes. ChangesNew Jest test suites
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/tools/test/utilities/polylineMath.jest.js (1)
31-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
beforeEachimport from@jest/globals.Line 701 uses
beforeEachbut it is not imported at line 31 where only{ describe, it, expect }are imported from@jest/globals. If Jest globals are not enabled in the config, this will throw aReferenceError. Even if globals happen to be available, the import style is inconsistent — all other Jest globals are explicitly imported.♻️ Proposed fix
import { describe, it, expect } from '`@jest/globals`'; +import { beforeEach } from '`@jest/globals`';Or consolidate into a single import:
-import { describe, it, expect } from '`@jest/globals`'; +import { describe, it, expect, beforeEach } from '`@jest/globals`';Also applies to: 701-701
🤖 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/tools/test/utilities/polylineMath.jest.js` around lines 31 - 32, The test file is using beforeEach without importing it from `@jest/globals`, which can cause a ReferenceError and is inconsistent with the existing explicit Jest imports. Update the import at the top of polylineMath.jest.js to include beforeEach alongside describe, it, and expect, and keep the test setup in the same style used by the rest of the file so the beforeEach call near the test block is covered.packages/tools/test/utilities/geometryPrimitives.jest.js (1)
372-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the misleading
it()description.The test name begins with "is 0 for a point inside the rectangle" but the assertion expects
5, not0. While the parenthetical clarifies the actual behavior, the leading text is confusing in test failure output — a developer sees "is 0" next to "Expected: 5".♻️ Suggested rename
- it('is 0 for a point inside the rectangle (closest edge distance still computed from segments, so on-boundary is 0, but strictly-inside is > 0 by design)', () => { + it('returns positive distance to nearest edge for a strictly-interior point (not signed inside distance)', () => {🤖 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/tools/test/utilities/geometryPrimitives.jest.js` around lines 372 - 378, Rename the misleading test description in geometryPrimitives.jest.js for the `rectangleDistanceToPoint` case so it matches the asserted behavior: the current `it()` title says the result is 0 for an inside point, but the expectation is `5`. Update the description to clearly state that a strictly inside point returns the distance to the nearest edge, keeping the existing assertion and helper reference (`rectangleDistanceToPoint`) unchanged.
🤖 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/tools/test/utilities/geometryPrimitives.jest.js`:
- Around line 372-378: Rename the misleading test description in
geometryPrimitives.jest.js for the `rectangleDistanceToPoint` case so it matches
the asserted behavior: the current `it()` title says the result is 0 for an
inside point, but the expectation is `5`. Update the description to clearly
state that a strictly inside point returns the distance to the nearest edge,
keeping the existing assertion and helper reference (`rectangleDistanceToPoint`)
unchanged.
In `@packages/tools/test/utilities/polylineMath.jest.js`:
- Around line 31-32: The test file is using beforeEach without importing it from
`@jest/globals`, which can cause a ReferenceError and is inconsistent with the
existing explicit Jest imports. Update the import at the top of
polylineMath.jest.js to include beforeEach alongside describe, it, and expect,
and keep the test setup in the same style used by the rest of the file so the
beforeEach call near the test block is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a4a895a6-a3da-4d1d-961e-c308b2dde7aa
📒 Files selected for processing (8)
packages/tools/test/annotationStateManagement.jest.jspackages/tools/test/storeToolFilters.jest.jspackages/tools/test/synchronizerCore.jest.jspackages/tools/test/utilities/geometryPrimitives.jest.jspackages/tools/test/utilities/planarAndBounds.jest.jspackages/tools/test/utilities/polylineBooleanOps.jest.jspackages/tools/test/utilities/polylineMath.jest.jspackages/tools/test/utilities/viewportFilters.jest.js
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.
Context
Second round of the test-coverage campaign (round 1: #2789, packages/core GenericViewport). packages/tools had only 6.3% jest line coverage — the karma browser suite owns interactive tool testing, but the pure logic underneath (state management, geometry math, hit-test filters, synchronization) was nearly untested. This PR targets the 5 most critical jsdom-testable areas, chosen from a per-file coverage audit.
What is added
427 new jest tests across eight files in
packages/tools/test/, no source changes:annotationStateManagement.jest.jsutilities/polylineMath.jest.js+utilities/polylineBooleanOps.jest.jsutilities/geometryPrimitives.jest.jssynchronizerCore.jest.jsutilities/viewportFilters.jest.jsstoreToolFilters.jest.jsutilities/planarAndBounds.jest.jsCoverage over
packages/tools/srcOverall jest coverage: 6.3% -> 12.6% lines, 7.5% -> 15.0% branches.
utilities/mathstateManagement/annotationutilities/viewportFiltersutilities/boundingBoxsynchronizersutilities/planarstoreLatent bugs surfaced (pinned by regression tests documenting current behavior; no behavior changed)
FrameOfReferenceSpecificAnnotationManager.getNumberOfAnnotations(groupKey)without a toolName always returns 0 — the map has no.length, so the count-all summation loop is unreachable.annotationVisibility.show/hideand single-UIDdeselectAnnotationlack the null-guard thatannotationLockinghas, and throw a TypeError when the annotation was already removed from the manager.store/addTool.tsremoveTool:if (!state.tools[toolName] !== undefined)is always true, making the documented "not added" throw unreachable.getBoundingBoxAroundShapeWorld: the world-space clamp derives both clip bounds from the samedimensions[axis]scalar, producing an inverted min/max range whenever clamping applies (the function has zero callers in src).intersectPolylines/subtractPolylines: when overlap corners land exactly on existing vertices of the other polygon (e.g. two 10x10 squares offset by 5), augmented-list consolidation mispairs intersections and the traced boundary is garbled.liangBarksyCliprejects segments lying exactly on a clip-window edge as outside (strict inequality in the degenerate branch).ellipse/pointInEllipsoidWithConstraintis an exported stub whose entire body is commented out (always returns undefined).addCanvasPointsToArray: strayconsole.login production source; point-count math divides both x and y distances byspacing[0], never usingspacing[1].A follow-up PR can address these individually; each has a test adjacent to the behavior making the fix straightforward to verify.
Verification
Full
packages/toolsjest suite: 33 suites, 618 passed, 2 skipped. No changes underpackages/tools/srcor to existing tests.Summary by CodeRabbit