test(core): add unit tests for GenericViewport base class and planar/modality camera math#2789
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.
📝 WalkthroughWalkthroughThis PR adds six new Jest test files under packages/core/test covering GenericViewport lifecycle and display-set access, video/ECG/volume3D camera math, planar render camera geometry, planar slice-basis computation, planar view reference/controller logic, and WSI transform utilities. No production code or exported entities are changed. 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 (3)
packages/core/test/planarRenderCamera.jest.js (1)
21-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared geometry-assertion helpers.
expectPoint2Close/expectPoint3Close(and similarly-shaped helpers) are re-declared per spec file — the sameexpectPoint3Closereappears inplanarSliceBasis.jest.js(lines 217-221). Centralizing these in a shared test-utils module would keep precision defaults consistent across the new planar spec files and ease future maintenance.🤖 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/core/test/planarRenderCamera.jest.js` around lines 21 - 57, The point-close assertion helpers are duplicated across planar spec files, including expectPoint2Close and expectPoint3Close here and the same pattern in planarSliceBasis.jest.js. Move these shared geometry assertions into a common test-utils module and update the planar tests to import them so the precision default stays consistent and future maintenance is centralized.packages/core/test/planarViewReference.jest.js (1)
29-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture/mock setup duplicated with
planarSliceBasis.jest.js.
createImage(lines 29-54) and themetaData.getmock configuration (lines 128-179) closely mirror the equivalent setup inpackages/core/test/planarSliceBasis.jest.js(lines 27-54, 223-261). Extracting a shared fixtures/mock module for this test cohort would reduce drift as more planar spec files are added.Also applies to: 128-179
🤖 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/core/test/planarViewReference.jest.js` around lines 29 - 54, The test setup in createImage and the metaData.get mock is duplicated with the planarSliceBasis spec, so factor this shared fixture/mock logic into a common test helper used by both planarViewReference.jest.js and planarSliceBasis.jest.js. Move the repeated image factory and metadata mock configuration into a shared module, then import and reuse it from the existing test files to keep the planar test cohort in sync and reduce drift.packages/core/test/modalityViewportCameras.jest.js (1)
502-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract
makeCanvasandmetricsto theecgViewportCameradescribe scope.The
makeCanvashelper (line 502) and themetricsobject (line 515) are defined inside theresolveECGCanvasMappingdescribe block but duplicated inline across thegetPanForECGCanvasMappingandgetAnchorWorldForCanvasPointblocks (lines 598–606, 607–614, 623–631, 632–639, 664–672, 673–680, 696–704, 705–712). Moving both to the outerecgViewportCameradescribe scope would eliminate ~40 lines of repetition and keep the fixtures DRY.♻️ Proposed refactor
describe('ecgViewportCamera', () => { const timeRange = [0, 2000]; const valueRange = [-1, 1]; + + function makeCanvas(clientWidth, clientHeight) { + const canvas = document.createElement('canvas'); + Object.defineProperty(canvas, 'clientWidth', { + configurable: true, + value: clientWidth, + }); + Object.defineProperty(canvas, 'clientHeight', { + configurable: true, + value: clientHeight, + }); + return canvas; + } + + const metrics = { + ecgWidth: 2000, + ecgHeight: 400, + channelScale: 10, + worldToCanvasRatio: 0.1, + xOffsetCanvas: 0, + yOffsetCanvas: 0, + }; describe('createDefaultECGViewState', () => {Then remove the inline
makeCanvasandmetricsdefinitions from each nested describe block.Also applies to: 596-620, 622-660, 662-693, 695-742
🤖 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/core/test/modalityViewportCameras.jest.js` around lines 502 - 513, The `makeCanvas` helper and `metrics` fixture are duplicated inside the nested `resolveECGCanvasMapping` tests; move both definitions up to the outer `ecgViewportCamera` describe scope so they can be reused by `getPanForECGCanvasMapping` and `getAnchorWorldForCanvasPoint`. Then remove the inline copies from each nested describe block and update the affected tests to reference the shared `makeCanvas` and `metrics` symbols.
🤖 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/core/test/modalityViewportCameras.jest.js`:
- Around line 502-513: The `makeCanvas` helper and `metrics` fixture are
duplicated inside the nested `resolveECGCanvasMapping` tests; move both
definitions up to the outer `ecgViewportCamera` describe scope so they can be
reused by `getPanForECGCanvasMapping` and `getAnchorWorldForCanvasPoint`. Then
remove the inline copies from each nested describe block and update the affected
tests to reference the shared `makeCanvas` and `metrics` symbols.
In `@packages/core/test/planarRenderCamera.jest.js`:
- Around line 21-57: The point-close assertion helpers are duplicated across
planar spec files, including expectPoint2Close and expectPoint3Close here and
the same pattern in planarSliceBasis.jest.js. Move these shared geometry
assertions into a common test-utils module and update the planar tests to import
them so the precision default stays consistent and future maintenance is
centralized.
In `@packages/core/test/planarViewReference.jest.js`:
- Around line 29-54: The test setup in createImage and the metaData.get mock is
duplicated with the planarSliceBasis spec, so factor this shared fixture/mock
logic into a common test helper used by both planarViewReference.jest.js and
planarSliceBasis.jest.js. Move the repeated image factory and metadata mock
configuration into a shared module, then import and reuse it from the existing
test files to keep the planar test cohort in sync and reduce drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2555d81-5cc3-41a8-9f3f-0a3788df05c9
📒 Files selected for processing (6)
packages/core/test/genericViewportBase.jest.jspackages/core/test/modalityViewportCameras.jest.jspackages/core/test/planarRenderCamera.jest.jspackages/core/test/planarSliceBasis.jest.jspackages/core/test/planarViewReference.jest.jspackages/core/test/wsiTransformUtils.jest.js
This was referenced Jul 10, 2026
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
The GenericViewport architecture in
packages/corehad large critical areas with little or no unit test coverage. This PR is a first round targeting the 5 most important undertested aspects, chosen from a per-file jest coverage audit (legacy Stack/Volume viewports and WebGL/VTK render paths intentionally excluded).What is added
279 new jest tests across six files in
packages/core/test/:genericViewportBase.jest.jsGenericViewportbase class +genericViewportDisplaySetAccessplanarSliceBasis.jest.jsplanarSliceBasisslice geometryplanarViewReference.jest.jsplanarViewReference+PlanarViewReferenceControllerplanarRenderCamera.jest.jsplanarRenderCamera,setVtkCameraClippingRange,PlanarVolumeResolvedViewmodalityViewportCameras.jest.jswsiTransformUtils.jest.jsHighlights:
addData, remount supersede, source-role demotion of other bindings.Coverage over
src/RenderingEngine/GenericViewportOverall: 33.7% -> 44.0% lines, 26.3% -> 38.1% branches.
GenericViewport.tsplanarViewReference.tsPlanarViewReferenceController.tsplanarRenderCamera.tsplanarSliceBasis.tsvideoViewportCamera.tswsiTransformUtils.tsgenericViewportDisplaySetAccess.tsFindings surfaced while testing (no source changes in this PR)
wsiTransformUtils.ts:canvasToIndexForWSImultiplies the input canvas point bydevicePixelRatiobutindexToCanvasForWSIdivides the entire transformed point (center translation included) by it — asymmetric DPR handling worth a look.planarSliceBasis.ts(getGeometricImageVolumeCenter): the corner-averaging fallback loop is unreachable dead code becausebuildImageVolumeCornersreturns[]wheneverimageDatais falsy.planarViewReference.ts: for stack bindings asliceIndexspecifier override changesreferencedImageIdbut notcameraFocalPoint(camera geometry comes from the single mounted image) — looks intentional, documented in a test.Verification
Full
packages/corejest suite: 30 suites, 514 passed, 2 skipped. No changes underpackages/core/srcor to existing tests.Summary by CodeRabbit