test(measurements): add DOM assertions for measurement annotation text#6129
test(measurements): add DOM assertions for measurement annotation text#6129GhadeerAlbattarni wants to merge 3 commits into
Conversation
Add expectAnnotationStatsText and shared measurementTextFormatters so measurement E2E tests verify displayed text from cachedStats instead of relying on screenshots alone. Mirrors the multi-surface assertion pattern from ArrowAnnotate.spec.ts for stats-based tools (Length, Bidirectional, Angle, CobbAngle, Circle, Ellipse, Rectangle, Probe). Includes a vendored roundNumber helper (copied from platform/core) because the Playwright harness cannot import @ohif/core at runtime.
…annotation-text-assertions
![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
📝 WalkthroughWalkthroughA shared Playwright utility module ( ChangesAnnotation text assertion utilities and tool test integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
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.
🧹 Nitpick comments (1)
tests/utils/expectAnnotationText.ts (1)
244-244: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
cobbAngleSvgLineandprobeIndexSvgLinethrow on undefined stats, unlike other formatters.Most formatters route through
roundNumber, which returns'NaN'for undefined/null/empty values. These two call.toFixed()and.join()directly onas-cast values, producing aTypeErrorinstead of a readable assertion failure if the stat is missing.🛡️ Optional guard for consistent error behavior
cobbAngleSvgLine: (stats: TargetStats) => - `${(stats.angle as number).toFixed(2)} \u00B0`, + `${roundNumber(stats.angle as number, 2)} \u00B0`,Note:
roundNumberuses adaptive precision, not fixed 2 d.p., so this would change the output for angles outside the 1–9.9° range. If fixed 2 d.p. is required, add a guard instead:cobbAngleSvgLine: (stats: TargetStats) => - `${(stats.angle as number).toFixed(2)} \u00B0`, + `${(Number(stats.angle ?? NaN)).toFixed(2)} \u00B0`,probeIndexSvgLine: (stats: TargetStats) => `(${(stats.index as number[]).join(', ')})`, + probeIndexSvgLine: (stats: TargetStats) => `(${((stats.index as number[] | undefined) ?? []).join(', ')})`,Also applies to: 276-276
🤖 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/utils/expectAnnotationText.ts` at line 244, The `cobbAngleSvgLine` and `probeIndexSvgLine` formatters are bypassing the shared undefined/null handling used by the other helpers and can throw TypeError when `stats` is missing. Update these formatters in `expectAnnotationText` to use the same safe formatting path as the other svg line helpers, or add an explicit guard before calling `.toFixed()`/`.join()` so missing values yield a readable assertion result instead of crashing.
🤖 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/utils/expectAnnotationText.ts`:
- Line 244: The `cobbAngleSvgLine` and `probeIndexSvgLine` formatters are
bypassing the shared undefined/null handling used by the other helpers and can
throw TypeError when `stats` is missing. Update these formatters in
`expectAnnotationText` to use the same safe formatting path as the other svg
line helpers, or add an explicit guard before calling `.toFixed()`/`.join()` so
missing values yield a readable assertion result instead of crashing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a9144a39-5501-4507-9bd9-ec9bc85a9375
📒 Files selected for processing (11)
tests/Angle.spec.tstests/ArrowAnnotate.spec.tstests/Bidirectional.spec.tstests/Circle.spec.tstests/CobbAngle.spec.tstests/Ellipse.spec.tstests/Length.spec.tstests/Probe.spec.tstests/Rectangle.spec.tstests/utils/expectAnnotationText.tstests/utils/index.ts
| ], | ||
| assertStats: stats => { | ||
| expect(stats.areaUnit).toBe('mm²'); | ||
| expect(Math.round(stats.area as number)).toBe(16778); |
There was a problem hiding this comment.
@jbocce I found the value for this area is different from the value that is in the current screenshot. If this is the right one, then we might need to update the screenshot as well.
| assertStats: stats => { | ||
| expect(stats.unit).toBe('mm'); | ||
| expect(Math.round(stats.length as number)).toBe(195); | ||
| expect(Math.round(stats.width as number)).toBe(130); |
There was a problem hiding this comment.
@jbocce I do think It's good to pass hardcoded numbers to assert the stats as well. If you agree with that I can update the rest of the tests in this PR to do the same.
1 participant
Context
Adds DOM-based annotation text assertions for measurement tools, following the pattern established in
ArrowAnnotate.spec.ts.Changes & Results
tests/utils/expectAnnotationText.tswithexpectAnnotationStatsTextandmeasurementTextFormattersto assert measurement text fromcachedStatsin the panel and viewport SVGArrowAnnotate.spec.tstests/utils/index.tsImplementation notes
Area:SVG prefix; CobbAngle panel usesroundNumber, SVG usestoFixed(2)).assertStatsin each spec adds numeric/unit sanity checks on rawcachedStats.roundNumberis copied rather than imported because the root Playwright E2E harness does not resolve@ohif/coreworkspace imports in Node.Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit