Skip to content

test(components): add tests for Switch, Radio, Checkbox - #223

Merged
Chibuzor-Nwemambu merged 7 commits into
mainfrom
214-batch-3-tests
Jul 9, 2026
Merged

test(components): add tests for Switch, Radio, Checkbox#223
Chibuzor-Nwemambu merged 7 commits into
mainfrom
214-batch-3-tests

Conversation

@Chibuzor-Nwemambu

Copy link
Copy Markdown
Collaborator

Summary

  • Batch 3 of test: add component test infrastructure #214's component test rollout: behavior tests for Switch, Radio, and Checkbox (SelectionControls) — 22 tests total, plus the shared test infra (test-utils.tsx, @expo/vector-icons mock, @types/react-test-renderer) duplicated from batch-1/2's branches, since this branch was created independently, off main post-fix(components): make component's own disabled state win accessibilityState merges #221, before either batch-1 or batch-2 merged.
  • Narrows jest.config.cjs's untested-component checklist the same way batch-1/2 did.
  • Along the way, investigated a suspected accessibilityState bug in Radio/Checkbox (same category as fix(components): component's own disabled state should always win over caller-supplied accessibilityState.disabled #220 — component's real disabled state not reflected in accessibilityState). Turned out not to be a real bug: React Native's Pressable already auto-overrides accessibilityState.disabled from its own disabled prop internally, so the apparent mismatch never manifested at the rendered level. A fix was written, found to be unnecessary once a regression test for it passed against both the pre- and post-fix code, and reverted (commits c04a643/3798816). The underlying test coverage for the "no onPress" case was kept (40e5559) since it's still valid, correct behavior worth covering.

Contributes to #214 — does not close it (the workflow-docs update, batch 4, is still outstanding).

Test plan

  • pnpm test — 22/22 passing across 3 suites
  • pnpm lint — clean
  • pnpm check-types — clean
  • pnpm build — clean

Batch 3 of issue #214's component test rollout — 20 tests across 3 suites.

Duplicates the shared test infra from batch-1/2 (test-utils render helper,
@expo/vector-icons mock, @types/react-test-renderer) since this branch was
created independently, off main post-#221, before either batch-1 or
batch-2 merged.

Narrows jest.config.cjs's untested-component checklist the same way
batch-1/2 did: removes SelectionControls (now tested) and reapplies the
permanent exclusions (Dialog/ErrorBoundary/Icon/Label/Portal/Scrim/
_internal/Paper/PressableHighlight).

Radio and Checkbox tests intentionally exclude the "no onPress provided"
scenario for now — see follow-up commit for why.
…cessibilityState

Both components force the underlying Pressable's disabled to true when no
onPress is provided (disabled={!onPress || disabled}), since a control
with nothing to call on press can't meaningfully be interactive. But
accessibilityState.disabled only reflected the raw disabled prop, not
this computed value — so a <Radio checked={false} /> with no onPress was
actually non-interactive while telling assistive technology it wasn't
disabled at all. Same category of bug as #220, different root cause (a
missing factor in the computation, not a merge-order issue).
Verifies a Radio/Checkbox rendered without onPress is safely
non-interactive (press is a no-op) and correctly reports disabled to
assistive technology. Not a regression test for a fix — React Native's
Pressable already normalizes accessibilityState.disabled from its own
disabled prop internally (Pressable.js), so this was correct before and
after the accessibilityState.disabled computation was touched. An earlier
commit on this branch "fixed" this and was reverted once that became
clear; this commit is just the leftover, still-valid test coverage.
@Chibuzor-Nwemambu Chibuzor-Nwemambu linked an issue Jul 7, 2026 that may be closed by this pull request
5 tasks
@Chibuzor-Nwemambu

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @Chibuzor-Nwemambu's task in 1m 51s —— View job


PR Review — Switch/Radio/Checkbox tests

  • Gather context (diff, test files, config)
  • Review test files (Switch, Radio, Checkbox)
  • Review shared infra & config changes
  • Post review feedback

Reviewed the full diff against origin/main and the components under test. This is a well-crafted, high-quality batch — the tests assert behavior (a11y roles/state, callback contracts, disabled gating) rather than snapshots, the shared infra is thoughtfully written, and the accessibilityState investigation described in the PR body is sound. No blocking issues.

What's good ✅

  • Behavior-focused, not brittle. Tests query by accessibilityRole and assert accessibilityState, the onPress/onChange contract (toHaveBeenCalledWith(!checked)), and disabled gating — the things that actually matter for these controls.
  • The jest.setup.ts icon mock is genuinely nice. Using Object.assign to copy static props (.font, etc.) off the real component onto the mock host is the right call — a naive () => <Text/> mock would silently break useEDS's ...MaterialCommunityIcons.font spread. The inline require + eslint-disable with an explanatory comment is correct given jest factory hoisting.
  • toBeDisabled() over asserting accessibilityState.disabled for the no-onPress case. This is the right choice and dovetails with the reverted-fix investigation: Checkbox/Radio author accessibilityState={{ ..., disabled }} (the prop), but the Pressable's own disabled={!onPress || disabled} overrides accessibilityState.disabled at render time. Testing the effective rendered value via toBeDisabled() avoids coupling to that quirk. Good instinct, and the revert (c04a6433798816) was justified.
  • Config changes are tidy and self-documenting — the jest.config.cjs checklist narrowing plus the new comment block explaining why Dialog/Icon/Paper/etc. are intentionally off the list is a helpful addition. The test-utils moduleNameMapper ↔ tsconfig paths mirroring is kept in sync with comments on both sides.

Minor observations (non-blocking, no changes required)

  1. Redundant test title wording. "reflects a different checked state via accessibilityState and icon for a different checked value" says "different checked" twice. Something like "reflects checked state via accessibilityState and icon" reads cleaner. Purely cosmetic.

  2. Coverage asymmetry between Switch and Radio/Checkbox (worth a note, not a fix). Radio/Checkbox get a "non-interactive … when onPress is not provided" test because they set disabled={!onPress || disabled}. Switch has no analog because onChange defaults to () => null and disabled={disabled} only — so Switch is always interactive unless disabled is explicitly set. That's a genuine component-design inconsistency across the three controls (Switch can't be made non-interactive by omitting its handler), but it's pre-existing and out of scope here. If it's not already tracked, it'd be a reasonable entry for #152.

  3. UNSAFE_getByProps({ name: ... }) couples to icon glyph names (checkbox-marked, radiobox-blank, minus-box). This is acceptable — the mapping is stable and the alternative (asserting nothing about the rendered icon) is weaker. Just flagging that a future icon-name refactor would need to touch these tests. No action needed.

Verification

I did not re-run the suite locally, but the assertions line up with the component source (Checkbox.tsx:98-118, Radio.tsx:84-101, Switch.tsx:55-73), and the PR reports pnpm test (22/22), lint, check-types, and build all clean.

Verdict: LGTM 👍 — merge-ready. The two substantive notes (Switch interactivity asymmetry, icon-name coupling) are follow-up/tracking material, not changes to this PR.
214-batch-3-tests

Dialog was previously treated as an internal component with no test
file expected, but it still uses the old token system and hasn't been
migrated. Add it to the Slice 2-4 exclude lists (tsconfig.json,
eslint.config.js, jest.config.cjs) instead.
@Chibuzor-Nwemambu

Copy link
Copy Markdown
Collaborator Author

Added Dialog to the Slice 2-4 exclude lists in tsconfig.json, eslint.config.js, and jest.config.cjs. It was initially miscategorized as an internal component (like ErrorBoundary/Icon/Label/Portal/Scrim) that wouldn't get a dedicated test file, but it turns out Dialog still uses the old token system and hasn't actually been migrated yet, so it belongs on the "excluded until migrated" list instead.

@pomfrida pomfrida left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 👍

@Chibuzor-Nwemambu
Chibuzor-Nwemambu merged commit a07a807 into main Jul 9, 2026
7 checks passed
@Chibuzor-Nwemambu
Chibuzor-Nwemambu deleted the 214-batch-3-tests branch July 9, 2026 10:55
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.

test: add component test infrastructure

2 participants