Skip to content

test(components): add tests for Input, TextField, TextArea, Search, EDSProvider - #218

Merged
Chibuzor-Nwemambu merged 8 commits into
mainfrom
214-batch-2-tests
Jul 8, 2026
Merged

test(components): add tests for Input, TextField, TextArea, Search, EDSProvider#218
Chibuzor-Nwemambu merged 8 commits into
mainfrom
214-batch-2-tests

Conversation

@Chibuzor-Nwemambu

@Chibuzor-Nwemambu Chibuzor-Nwemambu commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

Note: this PR previously included an isolated fix for #217 (Input.tsx's accessibilityState clobbering caller-supplied values) plus its regression test. Both were reverted — the fix turned out to be incomplete (it stopped caller values from being clobbered, but still let a caller's disabled win over the component's own computed value on conflict). That's now fixed properly, in one place, by #221, which superseded and closed #217 as well.

Contributes to #214 — does not close it (SelectionControls and the new-component-workflow update are still outstanding).

Test plan

  • pnpm test — 48/48 passing across 5 suites
  • pnpm lint — clean
  • pnpm check-types — clean
  • pnpm build — clean

…DSProvider

Batch 2 of issue #214's component test rollout — 46 tests across 5 suites.

Duplicates the shared test infra from batch-1 (test-utils render helper,
@expo/vector-icons mock, @types/react-test-renderer) since this branch was
deliberately created independently of batch-1's branch, off main post-#215.

Narrows jest.config.cjs's untested-component checklist the same way batch-1
did: drops Input, Search, TextArea, TextField, EDSProvider (now tested) plus
the permanently-excluded internal/deprecated components.
…ng it

Input set accessibilityState={{ disabled }} after spreading {...rest}, so
any caller-supplied accessibilityState was silently discarded. Button and
TextArea already merge the caller's value in — Input didn't.

Fixes #217
…merge

Verified this test fails against the pre-fix Input.tsx and passes against
the fix from the previous commit.
@Chibuzor-Nwemambu Chibuzor-Nwemambu linked an issue Jul 6, 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 6, 2026

Copy link
Copy Markdown

Claude finished @Chibuzor-Nwemambu's task in 2m 3s —— View job


Review — PR #218

  • Read changed files & gather context
  • Review test files for correctness & coverage
  • Review the accessibilityState fix
  • Review config/infra changes
  • Post review feedback

This is a well-crafted PR. The tests are behavior-focused (query by placeholder/label/text rather than internals), the comments explain the non-obvious bits (pointerEvents overlay on Cancel, the readOnly vs editable distinction, why RNTL's toBeDisabled() can't assert the readOnly case), and the jest.config.cjs guard that fails loudly when a test file lands but its ignore-entry lingers is a genuinely nice touch. CI is green (PR Check + build both ✅), so I focused on correctness and design rather than re-running the suite (I couldn't pnpm install in this sandbox).

LGTM. A few observations, none blocking.

1. The accessibilityState fix lets a caller silently override the component's own disabled state — low priority

Input.tsx:126-129

accessibilityState={{
    disabled,
    ...rest.accessibilityState,
}}

The fix correctly stops discarding caller state (fixes #217 👍). But because the spread comes after disabled, a caller passing accessibilityState={{ disabled: true }} while the disabled prop is false gets editable={true} (driven by the prop) but accessibilityState.disabled: true announced to assistive tech — the two can now disagree.

disabled is a first-class prop with real behavior wired to it (editable, styling), so arguably the component's computed value should win: { ...rest.accessibilityState, disabled }. That said, this exactly mirrors the existing TextArea pattern (TextArea.tsx:160), so the current ordering is at least consistent across the library — which is a fine reason to leave it. Flagging mostly so the choice is deliberate. If you agree it's worth tightening, it'd be a follow-up touching both components, not this PR.

2. TextArea/TextField don't have a merge-regression test like Input does

Input.test.tsx:69 added a great regression test for the merge. TextArea has the same merge logic ({ disabled, ...userAccessibilityState }) and TextField/Search inherit Input's behavior — but none assert the merge survives. Not necessary for this PR (the shared code path is covered by Input's test), just noting the asymmetry in case you want parity later.

3. Minor: two nearly-identical accessibilityHint join expressions

TextArea.tsx:155-158 and TextField.tsx:70-75 both compute [description, helperMessage].filter(Boolean).join(". ") || undefined, and the tests assert it in both places. Not a test issue — just an observation that the join logic is duplicated across components and could eventually live in a shared helper. Out of scope here.


Nothing here needs to block the merge — items 1–3 are all follow-up-or-ignore. The test infra and coverage are solid, and the scope (Batch 2 of #214, fixes #217, correctly not closing #214) matches the description.
· 214-batch-2-tests

Chibuzor-Nwemambu added a commit that referenced this pull request Jul 7, 2026
…yState merges (#221)

## Summary
- Flips the `accessibilityState` merge order in `Button`, `IconButton`,
`Input`, and `TextArea` so the component's own computed `disabled`
always wins over a caller-supplied `accessibilityState.disabled`,
instead of the other way around.
- `disabled` drives real behavior (`editable`, styling) — letting a
caller's `accessibilityState` disagree with it can mislead assistive
technology users specifically (e.g. a screen reader announcing
"disabled" on a field that's actually fully interactive, or vice versa).
No realistic legitimate use case was found for allowing that
disagreement.
- Other `accessibilityState` keys (`selected`, `expanded`, `checked`,
etc.) are untouched and remain fully caller-overridable — this only
changes `disabled`.
- `TextField`/`Search` both wrap `Input` and forward
`accessibilityState` through `{...rest}`, so they inherit the fix
automatically with no changes of their own.
- `Input.tsx` here is written directly as the final, complete form
(merges *and* the component's `disabled` wins) — `main` never had #217's
partial fix, only `214-batch-2-tests` did, and that partial fix has now
been reverted there in favor of this being the single, canonical fix.
This PR fully supersedes and closes #217.
- Source-only change, no test files touched — regression tests are being
added as a follow-up directly into PR #216 (`Button.test.tsx`, currently
zero coverage of this) and PR #218
(`Input.test.tsx`/`TextArea.test.tsx`, currently zero coverage of this
after the #217 partial-fix revert) once this PR merges.

Fixes #220. Fixes #217.

## Test plan
- [x] `pnpm lint` — clean
- [x] `pnpm check-types` — clean
- [x] `pnpm build` — clean
- [ ] Regression tests to be added to PR #216 and PR #218 as a follow-up
once this merges (this PR intentionally has no test changes, per the
agreed atomic-commit split)
…nput/TextArea

Covers #221's fix (now merged): a caller-supplied accessibilityState
merges with the component's own computed disabled value, with disabled
winning on conflict. One consolidated test per component rather than
separate #217/#220 tests, since they're one fix now.

Verified each test fails against its respective pre-#221 source (Input's
original no-merge-at-all code, TextArea's original caller-wins-on-conflict
code) and passes against the fix.
@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 36s —— View job


Re-review — PR #218


Re-reviewed since my first pass. Two of the three follow-ups I raised have been resolved by the new commits, and CI is green (PR Check ✅ ×2 + build ✅). LGTM — nothing blocking.

✅ Prior item #1 (caller could override the component's own disabled) — resolved by #221

Input.tsx:126-129 now spreads the caller state first and lets the component's computed value win:

accessibilityState={{
    ...rest.accessibilityState,
    disabled,
}}

TextArea.tsx:160 matches ({ ...userAccessibilityState, disabled }), so the ordering is consistent across both components — and now it's the correct order, not just a consistent one. The new regression tests pin exactly this: caller passes { selected: true, disabled: true } with the disabled prop false, and both assert { selected: true, disabled: false } — proving the merge preserves unrelated keys (selected) and the component's own disabled wins on conflict. (Input.test.tsx:69, TextArea.test.tsx:89.) Good, tight tests.

✅ Prior item #2 (TextArea lacked a merge-regression test) — resolved

TextArea.test.tsx:89 now has parity with Input.test.tsx:69. TextField/Search still inherit Input's path and are covered transitively — reasonable.

ℹ️ Prior item #3 (duplicated accessibilityHint join) — still open, still out of scope

TextArea.tsx:154-158 and TextField.tsx both compute [description, helperMessage].filter(Boolean).join(". ") || undefined. Purely an observation about the source (not the tests); a shared helper would be a separate refactor.

Notes on the test suite itself

  • Behavior-focused throughout — queries by placeholder/label/text/display-value, not internals. The UNSAFE_*ByProps({ name: "alert-circle" }) icon checks are the pragmatic exception and are fine.
  • The non-obvious cases are well-commented: the readOnly vs disabled editable/accessibilityState distinction (Input.test.tsx:59, TextArea.test.tsx:78), why toBeDisabled() can't assert the readOnly case, the Cancel button's pointerEvents overlay requiring a focus first (Search.test.tsx:78), and the expected console.error in the EDSProvider throw test (EDSProvider.test.tsx:77).
  • EDSProvider.test.tsx correctly imports from @testing-library/react-native directly rather than test-utils — right call, since wrapping the provider-under-test in the test-utils provider would defeat the point.
  • The jest.config.cjs guard that throws at config-load if a test file lands while its ignore-entry lingers remains a genuinely nice touch — it turns a silent "green checkmark, no coverage" footgun into a loud failure.

Scope matches the description: Batch 2 of #214, regression coverage for the merged #221 fix, correctly not closing #214. Clean.
· 214-batch-2-tests

pomfrida
pomfrida previously approved these changes Jul 8, 2026

@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 👍 One thing - the jest setup is in both this and #216 so this'll conflict once one is merged. Can we merge #216 first and rebase this after? Just flagging so the checklist conflict gets resolved carefully rather than auto-merged.

Comment thread packages/components/src/components/Search/Search.test.tsx
@Chibuzor-Nwemambu
Chibuzor-Nwemambu merged commit b2fa65c into main Jul 8, 2026
7 checks passed
@Chibuzor-Nwemambu
Chibuzor-Nwemambu deleted the 214-batch-2-tests branch July 8, 2026 07:57
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.

fix(components): Input clobbers a caller-supplied accessibilityState instead of merging it test: add component test infrastructure

2 participants