test(ui): consolidate similar tests via table-driven it.each (Sonar S5976) - #482
Conversation
…5976)
Replace 20 clusters of near-identical vitest cases across apps/web,
apps/mobile, and packages/shared with parameterized it.each tables. Every
previously-asserted case is preserved (same inputs, same assertions); only
the duplicated scaffolding is collapsed. Fixes all open instances of
typescript:S5976 ("Similar tests should be consolidated").
Refs #243
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #482 — test(ui): consolidate similar tests via table-driven it.each (Sonar S5976)
Scope: PR #482 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
Pure test-refactor PR: 20 SonarCloud typescript:S5976 clusters across 18 test files (14 web, 2 mobile, 2 shared) are collapsed from near-identical it(...) blocks into table-driven it.each / it.each<T> calls. No production/component source files are touched — only __tests__ files — and every previously-asserted case, input, and assertion is preserved under a descriptive per-case name. Spot-checked every changed file against its pre-image in the diff; the mapping from old individual tests to new table rows is 1:1 with matching inputs and expected outputs throughout (action-chips, clarification-card, today-page, conflict-warning, level-up-overlay, streak-celebration, goal-detail-drawer, habit-form-fields, feature-guide-drawer, referral-card, referral-drawer, pro-badge, api-fetch-i18n, advanced, error, streak, error-utils-extended, utils/timezone). No new any, no new comments, no console.log, no i18n/contract/security surface touched.
One legitimate Medium finding: the identical duplicate-test shape this PR fixes on the mobile side of two chat components still exists, untouched, in those components' web test mirrors.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Web mirrors of the two consolidated mobile chat-test files still carry the same S5976-shaped duplication
· dimension: Correctness (#1) / SOLID-DRY (#3, rule 10) — the PR's own stated scope ("every open instance of S5976")
· location: apps/web/tests/components/chat/action-chips.test.tsx:167,188,196,204,212 and apps/web/tests/components/chat/clarification-card.test.tsx:96,108,120,132
· issue: apps/mobile/__tests__/components/chat/action-chips.test.tsx was consolidated in this PR into an it.each covering the "not rendered as Pressable" cases (DeleteHabit, DeleteGoal, Failed, no-handler, null-entityId). Its web mirror has the exact same shape — 5 near-identical it() blocks in the describe('clickable chips', …) block (lines 167, 188, 196, 204, 212), each rendering one ActionChips config and asserting screen.queryByRole('button')).not.toBeInTheDocument() — left as individual tests. Same pattern for clarification-card.test.tsx: the mobile file's 4-case 404/generic/409/410 error cluster was folded into it.each here, but the web mirror's identical 4-case cluster (lines 96, 108, 120, 132 — mock a different status/error, click a quick-action button, assert a different error key) was not touched.
· risk: Not a behavior bug and won't break anything at runtime — but the PR body claims completeness ("every open instance … 20 flagged clusters") and issue #243 may still show these as open if SonarCloud flags them (structurally they are template-identical to what got fixed elsewhere in this same PR). Low cost to fix now while the pattern is fresh; if left, it's easy to forget and needs a second pass later.
· fix: Fold apps/web/__tests__/components/chat/action-chips.test.tsx:167-219 into one it.each keyed on action type/config (mirroring the mobile table added in this PR), and fold apps/web/__tests__/components/chat/clarification-card.test.tsx:96-143 into one it.each keyed on {status, expectedKey} the same way the mobile file now does it.
· reference: PR #482 body ("every open instance of SonarCloud rule S5976"); CLAUDE.md rule 10 (DRY at the right level)
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PARTIAL — see Medium finding above (test-hygiene parity, not behavioral parity; production code unchanged) |
| i18n-syncer | N/A — no i18n keys added or changed |
| contract-aligner | N/A — no packages/shared/src/types/*, endpoints.ts, or orbit-api change |
| security-reviewer | N/A — no orbit-api code touched |
| design-reviewer | N/A — only __tests__ files changed, no rendered UI/token surface |
Validation
| Check | Result |
|---|---|
| Lint | N/A — skipped per CI adaptation (Build/Unit Tests/SonarCloud run as separate required checks on this PR) |
| Type check | N/A — skipped per CI adaptation |
| Tests | N/A — skipped per CI adaptation; PR body states full suites green (web 2239, mobile 1056, shared 1499) |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop): N/A — gate requires changed UI/component source under
apps/*; this diff only touches__tests__files. - Dimension 10 (i18n): N/A — no string/locale-file changes; existing keys are only referenced, not added.
- Dimension 11 (Contract drift + backward-compat): N/A — no
packages/shared/src/types/*or DTO changes. - Dimension 12 (Security): N/A — no orbit-api or trust-boundary code changed.
- Dimension 13 (Backend hard rules): N/A — orbit-api not touched.
- Dimension 14 (FEATURES.md parity): N/A — no user-facing feature/behavior surface changed (test-only diff).
- Issue #243's full closure status against the live SonarCloud dashboard could not be independently verified in this environment (no SonarCloud access from CI) — the Medium finding above is based on direct code inspection of the two mirror files, not a live Sonar query.
- All 18 changed files received a direct correctness verdict (diffed old-test-to-new-row 1:1) in the Summary above; none deferred.
What's good
- Textbook mechanical refactor: every folded case keeps its original assertion, input, and a descriptive name (several files use
'$name'interpolation specifically to preserve the original human-readable test titles in the table). - Net -468 lines with zero coverage loss — a real win against S5976 without sacrificing readability.
- The mobile
action-chips/clarification-cardfolds went a step further than the minimum, absorbing structural twins adjacent to the flagged cluster so no residual near-duplicate group of 3 remains — good instinct that, per the Medium finding, is worth applying symmetrically to the web mirrors of those same two files. - Zero production-code risk: diff is entirely test-file scoped, so this cannot regress runtime behavior.
Recommendation
Approve as-is — the diff is behavior-preserving and net-positive. Open a fast-follow (or fold into #243 directly) to apply the same it.each consolidation to the two web mirror clusters called out above, so the mobile and web test suites reach the same duplication-free state together.
|
1 participant
What
Burns down every open instance of SonarCloud rule
typescript:S5976("Similar tests should be consolidated") inorbit-ui-mobile— 20 flagged clusters across 18 test files — by collapsing each cluster of near-identical vitest cases into a single table-drivenit.each/it.each<T>.Behavior-preserving: every previously-asserted case is kept (same inputs, same assertions, descriptive per-case names). Only duplicated scaffolding is removed. Net −468 lines, test-only.
Clusters fixed (rule + count)
typescript:S5976× 20:apps/web/.../onboarding/feature-guide-drawer.test.tsxapps/web/.../pages/error.test.tsxapps/web/.../pages/streak.test.tsxapps/web/.../app/today-page.test.tsxapps/web/.../chat/conflict-warning.test.tsxapps/web/.../gamification/level-up-overlay.test.tsxapps/web/.../gamification/streak-celebration.test.tsxapps/web/.../lib/api-fetch-i18n.test.tsapps/web/.../pages/advanced.test.tsxapps/web/.../goals/goal-detail-drawer.test.tsxapps/web/.../habits/habit-form-fields.test.tsxapps/web/.../referral/referral-card.test.tsxapps/web/.../referral/referral-drawer.test.tsxapps/web/.../ui/pro-badge.test.tsxapps/mobile/.../chat/clarification-card.test.tsxapps/mobile/.../chat/action-chips.test.tsxpackages/shared/.../error-utils-extended.test.tspackages/shared/.../utils.test.tsThe two mobile clusters (
action-chips,clarification-card) fold the flagged group plus its structural twins so no residual group of 3 remains.Verification
tsc --noEmitclean in all three workspaces;eslint(web/shared) +expo lint(mobile) clean.--blocking error --scope changed: 0 issues on 15 changed files.Refs #243