test(contour): add Freehand and Spline contour drawing tool E2E tests#6117
Draft
diattamo wants to merge 9 commits into
Draft
test(contour): add Freehand and Spline contour drawing tool E2E tests#6117diattamo wants to merge 9 commits into
diattamo wants to merge 9 commits into
Conversation
…sed addSegmentButton
…e to split by tools
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTwo new Playwright specs add RTSTRUCT contour persistence coverage for freehand and spline tools. Both hydrate slice 1/47, draw contours, switch segments, return to slice 1, and verify the contour remains present. ChangesContour Segmentation Persistence Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/FreehandContourSegmentation.spec.ts (1)
87-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the page-object segment count helper here.
Counting
panel.rowscouples the test to sidebar DOM structure. This project already prefers segment totals through the page object helper.Proposed refactor
- await expect(panel.rows, 'Expected 4 segments to start').toHaveCount(4); + expect(await panel.getSegmentCount(), 'Expected 4 segments to start').toBe(4); await rightPanelPageObject.contourSegmentationPanel.addSegmentButton.click(); - await expect(panel.rows, 'Expected a new segment row to be added').toHaveCount(5); + expect(await panel.getSegmentCount(), 'Expected a new segment row to be added').toBe(5);Based on learnings, segment counts in OHIF Playwright tests should use the panel’s
getSegmentCount()helper instead of locator counts.🤖 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 `@tests/FreehandContourSegmentation.spec.ts` around lines 87 - 90, The segment count assertions in FreehandContourSegmentation.spec should use the page object helper instead of counting panel.rows directly, since the test is coupled to sidebar DOM structure. Update the assertions around ContourSegmentationPanel to call the existing getSegmentCount() helper on the rightPanelPageObject.contourSegmentationPanel for both the initial and post-add-segment checks, keeping the test aligned with the page-object pattern.Source: Learnings
tests/SplineContourSegmentation.spec.ts (1)
87-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the page-object segment count helper here.
Counting
panel.rowscouples the test to sidebar DOM structure. This project already prefers segment totals through the page object helper.Proposed refactor
- await expect(panel.rows, 'Expected 4 segments to start').toHaveCount(4); + expect(await panel.getSegmentCount(), 'Expected 4 segments to start').toBe(4); await rightPanelPageObject.contourSegmentationPanel.addSegmentButton.click(); - await expect(panel.rows, 'Expected a new segment row to be added').toHaveCount(5); + expect(await panel.getSegmentCount(), 'Expected a new segment row to be added').toBe(5);Based on learnings, segment counts in OHIF Playwright tests should use the panel’s
getSegmentCount()helper instead of locator counts.🤖 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 `@tests/SplineContourSegmentation.spec.ts` around lines 87 - 90, Replace direct row-count assertions in the SplineContourSegmentation test with the page-object segment count helper. In the test around rightPanelPageObject.contourSegmentationPanel, use getSegmentCount() instead of panel.rows so the assertions verify segment totals through the page object rather than sidebar DOM structure. Keep the same expectations for 4 then 5 segments, but route both checks through the helper.Source: Learnings
🤖 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 `@tests/FreehandContourSegmentation.spec.ts`:
- Around line 39-50: The FreehandContourSegmentation.spec assertions currently
verify only that a second SVG path exists and has a d attribute, so they miss
cases where the contour is hidden. Update the affected test blocks in
FreehandContourSegmentation.spec to assert actual viewport visibility for the
contour rendered by freehandContour and normalizedPathDragAt, using the existing
viewportPageObject/defaultViewport helpers rather than only getSvgAttribute and
toHaveCount. Keep the checks tied to the contour path element but ensure they
fail if the node is present in the DOM and not visible on the viewport.
In `@tests/SplineContourSegmentation.spec.ts`:
- Around line 39-50: The spline contour spec is only asserting that a second SVG
path exists and has a d attribute, which does not verify viewport visibility.
Update the relevant checks in SplineContourSegmentation.spec.ts to assert that
the contour is actually visible on the viewport after drawing, using the
existing path lookup helpers and the viewport/page object state rather than just
DOM persistence. Apply the same visibility-focused assertion pattern to the
other affected blocks in this spec so the tests match the PR goal of remaining
visible on the viewport.
---
Nitpick comments:
In `@tests/FreehandContourSegmentation.spec.ts`:
- Around line 87-90: The segment count assertions in
FreehandContourSegmentation.spec should use the page object helper instead of
counting panel.rows directly, since the test is coupled to sidebar DOM
structure. Update the assertions around ContourSegmentationPanel to call the
existing getSegmentCount() helper on the
rightPanelPageObject.contourSegmentationPanel for both the initial and
post-add-segment checks, keeping the test aligned with the page-object pattern.
In `@tests/SplineContourSegmentation.spec.ts`:
- Around line 87-90: Replace direct row-count assertions in the
SplineContourSegmentation test with the page-object segment count helper. In the
test around rightPanelPageObject.contourSegmentationPanel, use getSegmentCount()
instead of panel.rows so the assertions verify segment totals through the page
object rather than sidebar DOM structure. Keep the same expectations for 4 then
5 segments, but route both checks through the helper.
🪄 Autofix (Beta)
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: 5de5bffc-654c-4088-bb61-324e52399675
📒 Files selected for processing (2)
tests/FreehandContourSegmentation.spec.tstests/SplineContourSegmentation.spec.ts
Comment on lines
+39
to
+50
| await expect(paths, 'Expected the starting number of paths to be 1').toHaveCount(1); | ||
| await rightPanelPageObject.contourSegmentationPanel.tools.freehandContour.click(); | ||
| await defaultViewport.normalizedPathDragAt({ path: dragShape }); | ||
| await expect(paths, 'Expected the freehand contour to be added on slice 1').toHaveCount(2); | ||
|
|
||
| const drawnPathD = await getSvgAttribute({ | ||
| viewportPageObject, | ||
| svgInnerElement: 'path', | ||
| attributeName: 'd', | ||
| nth: 1, | ||
| }); | ||
| expect(drawnPathD, 'Expected the drawn freehand contour to render an SVG path').not.toBeNull(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert contour visibility, not just path persistence.
These checks only prove that a second <path> exists and still has a d attribute. A regression that keeps the node in the DOM but hides it would still pass, which misses the stated “remain visible on the viewport” goal.
Proposed fix
await expect(paths, 'Expected the freehand contour to be added on slice 1').toHaveCount(2);
+ await expect(paths.nth(1), 'Expected the freehand contour to be visible on slice 1').toBeVisible();
const drawnPathD = await getSvgAttribute({
viewportPageObject,
@@
await defaultViewport.sliceNavigation.toFirstSlice();
await expect(sliceIndicator, 'Expected to scroll back to slice 1').toHaveText(FIRST_SLICE_OVERLAY);
await expect(paths, 'Expected the freehand contour to re-render on slice 1').toHaveCount(2);
+ await expect(paths.nth(1), 'Expected the freehand contour to still be visible on slice 1').toBeVisible();
@@
await rightPanelPageObject.contourSegmentationPanel.tools.freehandContour.click();
await defaultViewport.normalizedPathDragAt({ path: dragShape });
await expect(paths, 'Expected the freehand contour to be added to the new segment').toHaveCount(2);
+ await expect(paths.nth(1), 'Expected the freehand contour to be visible in the new segment').toBeVisible();
@@
await expect(
sliceIndicator,
'Expected returning to the added segment to land back on slice 1'
).toHaveText(FIRST_SLICE_OVERLAY);
await expect(paths, 'Expected the freehand contour to re-render on slice 1').toHaveCount(2);
+ await expect(paths.nth(1), 'Expected the freehand contour to still be visible after switching back').toBeVisible();Based on PR objectives, this suite is supposed to verify viewport visibility, not only SVG node persistence.
Also applies to: 58-69, 96-104, 114-123
🤖 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 `@tests/FreehandContourSegmentation.spec.ts` around lines 39 - 50, The
FreehandContourSegmentation.spec assertions currently verify only that a second
SVG path exists and has a d attribute, so they miss cases where the contour is
hidden. Update the affected test blocks in FreehandContourSegmentation.spec to
assert actual viewport visibility for the contour rendered by freehandContour
and normalizedPathDragAt, using the existing viewportPageObject/defaultViewport
helpers rather than only getSvgAttribute and toHaveCount. Keep the checks tied
to the contour path element but ensure they fail if the node is present in the
DOM and not visible on the viewport.
Comment on lines
+39
to
+50
| await expect(paths, 'Expected the starting number of paths to be 1').toHaveCount(1); | ||
| await rightPanelPageObject.contourSegmentationPanel.tools.splineContour.click(); | ||
| await defaultViewport.normalizedClickAt(dragShape); | ||
| await expect(paths, 'Expected the spline contour to be added on slice 1').toHaveCount(2); | ||
|
|
||
| const drawnPathD = await getSvgAttribute({ | ||
| viewportPageObject, | ||
| svgInnerElement: 'path', | ||
| attributeName: 'd', | ||
| nth: 1, | ||
| }); | ||
| expect(drawnPathD, 'Expected the drawn spline contour to render an SVG path').not.toBeNull(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert contour visibility, not just path persistence.
These checks only prove that a second <path> exists and still has a d attribute. A regression that keeps the node in the DOM but hides it would still pass, which misses the stated “remain visible on the viewport” goal.
Proposed fix
await expect(paths, 'Expected the spline contour to be added on slice 1').toHaveCount(2);
+ await expect(paths.nth(1), 'Expected the spline contour to be visible on slice 1').toBeVisible();
const drawnPathD = await getSvgAttribute({
viewportPageObject,
@@
await defaultViewport.sliceNavigation.toFirstSlice();
await expect(sliceIndicator, 'Expected to scroll back to slice 1').toHaveText(FIRST_SLICE_OVERLAY);
await expect(paths, 'Expected the spline contour to re-render on slice 1').toHaveCount(2);
+ await expect(paths.nth(1), 'Expected the spline contour to still be visible on slice 1').toBeVisible();
@@
await rightPanelPageObject.contourSegmentationPanel.tools.splineContour.click();
await defaultViewport.normalizedClickAt(dragShape);
await expect(paths, 'Expected the spline contour to be added to the new segment').toHaveCount(2);
+ await expect(paths.nth(1), 'Expected the spline contour to be visible in the new segment').toBeVisible();
@@
await expect(
sliceIndicator,
'Expected returning to the added segment to land back on slice 1'
).toHaveText(FIRST_SLICE_OVERLAY);
await expect(paths, 'Expected the spline contour to re-render on slice 1').toHaveCount(2);
+ await expect(paths.nth(1), 'Expected the spline contour to still be visible after switching back').toBeVisible();Based on PR objectives, this suite is supposed to verify viewport visibility, not only SVG node persistence.
Also applies to: 58-69, 96-104, 114-123
🤖 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 `@tests/SplineContourSegmentation.spec.ts` around lines 39 - 50, The spline
contour spec is only asserting that a second SVG path exists and has a d
attribute, which does not verify viewport visibility. Update the relevant checks
in SplineContourSegmentation.spec.ts to assert that the contour is actually
visible on the viewport after drawing, using the existing path lookup helpers
and the viewport/page object state rather than just DOM persistence. Apply the
same visibility-focused assertion pattern to the other affected blocks in this
spec so the tests match the PR goal of remaining visible on the viewport.
…on spline added-segment
diattamo
force-pushed
the
test/contour-drawing-tools
branch
from
d25c725 to
9dca707
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/SplineContourSegmentation.spec.ts (1)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the added segment index from the baseline count.
panel.nthSegment(4)hard-codes the current fixture having exactly four existing rows. This test already captures the pre-add count, so use that to select the newly created row and avoid brittle failures if the RTSTRUCT fixture gains or reorders seeded segments.♻️ Proposed change
- const addedSegment = panel.nthSegment(4); + const addedSegment = panel.nthSegment(initialSegmentCount);🤖 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 `@tests/SplineContourSegmentation.spec.ts` at line 73, The test in SplineContourSegmentation.spec.ts hard-codes the new row index in nthSegment(4), which makes it brittle if the seeded RTSTRUCT segments change. Update the assertion setup around panel.nthSegment to derive the added segment index from the existing baseline count captured earlier in the test, so the newly created row is selected relative to the pre-add state rather than by a fixed number.
🤖 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 `@tests/SplineContourSegmentation.spec.ts`:
- Line 73: The test in SplineContourSegmentation.spec.ts hard-codes the new row
index in nthSegment(4), which makes it brittle if the seeded RTSTRUCT segments
change. Update the assertion setup around panel.nthSegment to derive the added
segment index from the existing baseline count captured earlier in the test, so
the newly created row is selected relative to the pre-add state rather than by a
fixed number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4526c4b3-1b25-4133-b7aa-55e3fe13a1d0
📒 Files selected for processing (2)
tests/FreehandContourSegmentation.spec.tstests/SplineContourSegmentation.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/FreehandContourSegmentation.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/SplineContourSegmentation.spec.ts (1)
91-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the newly added segment instead of hardcoding index
4.This ties the test to the fixture always starting with exactly four segments. If that seed data changes, the test will click the wrong row before it even reaches the persistence assertions. Prefer using the panel API to select the last segment after creation.
Suggested change
- const addedSegment = panel.nthSegment(4); // newly added segment, index 4 + const segmentCount = await panel.getSegmentCount(); + const addedSegment = panel.nthSegment(segmentCount - 1);🤖 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 `@tests/SplineContourSegmentation.spec.ts` at line 91, The test in SplineContourSegmentation.spec.ts is hardcoding the newly created segment as panel.nthSegment(4), which assumes the fixture always has four initial segments. Update the test to derive the created segment from the panel API after adding it, using the last segment (or otherwise selecting the most recently added segment) instead of a fixed index so the interaction stays correct if seed data changes.Source: Learnings
🤖 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 `@tests/SplineContourSegmentation.spec.ts`:
- Line 91: The test in SplineContourSegmentation.spec.ts is hardcoding the newly
created segment as panel.nthSegment(4), which assumes the fixture always has
four initial segments. Update the test to derive the created segment from the
panel API after adding it, using the last segment (or otherwise selecting the
most recently added segment) instead of a fixed index so the interaction stays
correct if seed data changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 344bdf59-4540-4d7d-90fc-eceb976aac57
📒 Files selected for processing (2)
tests/FreehandContourSegmentation.spec.tstests/SplineContourSegmentation.spec.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.
Context
This PR adds E2E coverage for the contour drawing tools (Freehand, Spline) — verifying
that a drawn contour persists unchanged when you navigate away from its slice/segment
and come back.
Tests are organized one file per tool so each tool's file can grow with future
tool-specific behavior beyond persistence.
Changes & Results
Test-only change — no production code touched. Reuses existing infrastructure already on
master: the
visitStudyAndHydrateutil, thegetSvgAttributeutil, and theRightPanelPageObjectcontour tool handlers (freehandContour,splineContour).tests/FreehandContourSegmentation.spec.ts(2 tests)tests/SplineContourSegmentation.spec.ts(2 tests)Each spec hydrates an RTSTRUCT contour segmentation and verifies, for both an
un-pre-selected segment and a freshly added segment:
Testing
All 4 tests pass locally (Chromium).
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Summary by CodeRabbit