test(contour): Livewire contour draw coverage - #6206
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
GitHub Actions
Inactive
|
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 ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughExpanded Livewire contour segmentation tests cover lifecycle, slice persistence, segment visibility, disjoint and overlapping contours, and Shift-based holes. Added keyboard, locator, utility export, and screenshot path support. ChangesLivewire contour testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR adds localized Livewire contour coverage without changing production behavior, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
diattamo
changed the title
GitHub Actions
Inactive
…ests and utility functions
….com/diattamo/Viewers into test/livewire-contour-completed-draw
GitHub Actions
Inactive
GitHub Actions
Inactive
…ts and update screenshot paths
| // The data-cy element above is a span wrapper, so its tooltip still | ||
| // fires while the tool is unavailable. The inner button is what | ||
| // carries the disabled state, so assert enablement against this. | ||
| toolButton: page.getByTestId('LivewireContourSegmentationTool-btn'), |
There was a problem hiding this comment.
This is a bit confusing to have "two buttons" for the same page object. Can we consolidate to one? I suspect 'LivewireContourSegmentationTool-btn' could be used for clicking too, but I might be wrong. Thanks.
| await contourPanel.select(); | ||
|
|
||
| await expect( | ||
| contourPanel.tools.livewireContour.toolButton, |
There was a problem hiding this comment.
With regards to my earlier comment regarding "two buttons". I suppose we have to watch for things like this too (i.e. checking for disabled). However we could get around this by adding a page object method like isEnab led or whatever.
There was a problem hiding this comment.
This and the other screenshots have text in them. This is now a no-no. Do we need to update the agent skills to reflect this?
| 'Expected the second contour vertex handles to clear on completion' | ||
| ).toHaveCount(0); | ||
|
|
||
| await checkForScreenshot({ |
There was a problem hiding this comment.
I think we want to check for viewport screenshot here, but shouldn't we also check for a single path too (i.e. path count is exactly one)?
| viewportPageObject, | ||
| }) => { | ||
| // Same overlap geometry as the same-segment merge test; across segments it must not merge. | ||
| const overlappingContourClicks = [ |
There was a problem hiding this comment.
I think this is declared twice. Is it worth refactoring to a constant?
| ).toHaveCount(0); | ||
|
|
||
| // The overlap that merges within one segment must stay two outlines across segments. | ||
| await checkForViewportScreenshot({ |
There was a problem hiding this comment.
Shouldn't we also check for two paths?
| 'Expected the hole vertex handles to clear on completion' | ||
| ).toHaveCount(0); | ||
|
|
||
| await checkForScreenshot({ |
There was a problem hiding this comment.
Maybe also check for one path?
| * throws. A held modifier (e.g. 'Shift') applies to the mouse events the action | ||
| * performs, which is how modifier-dependent tools are driven from a test. | ||
| */ | ||
| export const withKeyHeld = async (params: WithKeyHeldParams) => { |
There was a problem hiding this comment.
Please do a quick check if this could be useful elsewhere (i.e. is there some place else we are doing something similar in the tests already)?
jbocce
left a comment
jbocce
left a comment
There was a problem hiding this comment.
See my suggestions and comments. Thanks so much.
2 participants
Context
LivewireContourSegmentation.spec.tscurrently covers only one path — cancelling anin-progress contour with
Escape. Nothing verified that a Livewire contour can actually becompleted, or how completed contours behave once they land in a segment.
This PR expands the spec to cover the completed-draw behaviors: persistence across slice and
segment navigation, tool enablement, segment ownership, and the three boolean outcomes of
drawing a second contour into the same segment (disjoint, overlapping, and hole-cutting with
Shift).Changes & Results
Test-only change — no production code touched.
tests/LivewireContourSegmentation.spec.ts— 6 new tests, plus the existingEscapetest retuned to a shared 4-point
contourClickspolygon (large enough to leave room for ahole inside it, and clear of neighbouring outlines):
double-clicking the last vertex, captures the SVG
pathdattribute, navigates to thelast slice, then reselects Segment 1 and asserts the re-rendered geometry is byte-identical.
enabled once one is created.
hiding Segment 1 and disappears/reappears with Segment 2's visibility toggle.
two paths.
single path.
Shiftcuts a hole — an inner contour drawn withShiftheld is subtracted into thesame path rather than added as a new one.
tests/pages/RightPanelPageObject.ts— exposelivewireContour.livewireToolButton.The
data-cyelement is a span wrapper (so its tooltip still fires while the tool isunavailable); the inner
-btnelement is what carries the disabled state, so enablement isasserted against that.
tests/utils/keyboardUtils.ts— addkeyDown/keyUpso a modifier can be held acrossmouse interactions (needed for the
Shifthole-cutting flow).tests/utils/screenShotPaths.ts+ 3 baselines — the boolean outcomes (merge, hole) arecanvas-only, so those three tests back the SVG path-count assertion with a
viewport-scoped screenshot compare.
.agents/skills/ohif-test-agent/SKILL.md— the "never substitute a state read for arender assertion" rule now names cornerstone state (
window.cornerstone,window.cornerstoneTools) alongsidewindow.services, and points at a viewport-scopedscreenshot as the default when no DOM/SVG signal captures the result (contour geometry,
merges, holes).
Testing
All 7 tests pass locally (Chromium).
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit