Skip to content

Tests/duplicated contour navigation issue#6033

Draft
diattamo wants to merge 19 commits into
OHIF:masterfrom
diattamo:tests/duplicated-contour-navigation-issue
Draft

Tests/duplicated contour navigation issue#6033
diattamo wants to merge 19 commits into
OHIF:masterfrom
diattamo:tests/duplicated-contour-navigation-issue

Conversation

@diattamo
Copy link
Copy Markdown
Contributor

@diattamo diattamo commented May 21, 2026

Context

This PR adds coverage for issue #6030.
It's temporarily skipped until issue is fixed.

The skipped test checks that selecting a duplicated contour navigates to the same instance number as the original contour and confirms geometry parity via matching SVG path data.

Changes & Results

Testing

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • [] My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] OS:
  • [] Node version:
  • [] Browser:

Greptile Summary

This PR cleans up two previously-flagged issues (the deep getSvgAttribute import and unused page fixtures) and preserves test coverage for issue #6030 by adding a test.skip that documents the expected instance-navigation and SVG-geometry parity behaviour for duplicated contour segments.

  • The barrel import for getSvgAttribute is now correctly sourced from ./utils, and both unused page fixture destructurings have been removed from the non-skipped tests.
  • A new test.skip block captures the full scenario: clicking the original segment, duplicating it, navigating away, then verifying the duplicated segment navigates back to the same instance and renders the same SVG path data.

Confidence Score: 5/5

Safe to merge — the only active changes are a corrected utility import and removal of unused fixtures; the new test is fully skipped and cannot affect CI.

The non-skipped changes are mechanical cleanup with no runtime impact. The skipped test adds documentation value but runs no code in CI until re-enabled.

No files require special attention beyond the skipped test's known assertion gaps, which were already flagged in prior review threads.

Important Files Changed

Filename Overview
tests/ContourSegmentDuplicate.spec.ts Fixes the getSvgAttribute deep import and removes two unused page fixtures; adds a new test.skip that documents the expected navigation behaviour for duplicated contour segments.

Reviews (4): Last reviewed commit: "code review updates" | Re-trigger Greptile

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@netlify
Copy link
Copy Markdown

netlify Bot commented May 21, 2026

Deploy Preview for ohif-dev failed. Why did it fail? →

Name Link
🔨 Latest commit 4bee07f
🔍 Latest deploy log https://app.netlify.com/projects/ohif-dev/deploys/6a0f4e2bdff13b00086e385e

Comment thread tests/ContourSegmentDuplicate.spec.ts Outdated
Comment thread tests/ContourSegmentDuplicate.spec.ts
Comment thread tests/ContourSegmentDuplicate.spec.ts
Comment thread tests/ContourSegmentDuplicate.spec.ts Outdated
Comment thread tests/ContourSegmentDuplicate.spec.ts Outdated
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 21, 2026

Want your agent to iterate on Greptile's feedback? Try greploops.

@diattamo diattamo marked this pull request as draft May 21, 2026 18:30
@diattamo
Copy link
Copy Markdown
Contributor Author

@jbocce @GhadeerAlbattarni fyi I created this PR off of the changes I was making in the duplicate testing. I have it marked as a draft right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant