test(components): add tests for Button, Badge, Divider, Link, Typography - #216
Conversation
Batch 1 of issue #214's component test rollout. Adds a shared test-utils render helper (wraps EDSProvider), an @expo/vector-icons mock to avoid act() warnings from its async font-loading state, and @types/react-test-renderer to fix RNTL's ReactTestInstance types resolving to `error` under tsc/eslint. Also narrows jest.config.cjs's untested-component checklist: Dialog, ErrorBoundary, Icon, Label, Portal, Scrim, and _internal are internal pieces covered indirectly via their consumers, and Paper/PressableHighlight are excluded since both are slated for removal/replacement.
|
@claude review |
|
Claude finished @Chibuzor-Nwemambu's task in 1m 54s —— View job Code review — PR #216
Nicely done — the tests exercise real behavior (roles, press handling, disabled state, icon rendering) rather than snapshots, and the shared infra is well thought through and thoroughly commented. A few observations, all non-blocking: Strengths worth calling out
Suggestions (optional)
None of these block merging. The core question for a test PR — do the tests assert meaningful behavior and is the infra sound? — is a clear yes. |
…ering Addresses Claude's automated review on PR #216: the it.each(...) variant loops for Badge/Typography only proved each value rendered without throwing, not that the prop actually did anything. Auditing the same pattern surfaced a bigger gap in Button and Link: their tone/size/variant and size axes had zero test coverage at all, not even crash-only. Brought all four components to the same standard: an it.each crash check per axis value, plus one real style-effect assertion per axis (comparing two representative values via a flattened style, not hardcoded exact token values, so these don't churn if design tokens change). Also found and fixed a test bug along the way: UNSAFE_getByProps matches the first node with a given prop, which for Button's leading/trailing icons is ButtonIcon itself (the wrapper, which also carries `name` but no style) rather than the actual rendered icon several layers deeper — UNSAFE_getAllByProps + the last match gets the real one.
|
Addressed the review feedback:
Also worth flagging: while writing batch-2's tests, found and fixed an unrelated but related-in-kind bug — |
…r axis Previous commit added a separate it.each crash loop (testing every enum value) alongside each effect assertion, ballooning the suite from 42 to 70 tests. Comparing two representative values already implicitly proves neither crashes, and the enum-to-token-key mapping is backed by TypeScript's own type system (Record<T, ...> satisfies constraints), so exhaustively sweeping every value added little beyond that. Merged each pair into a single comparative test: 70 -> 36 tests.
|
Quick correction to my last comment: the fix for point 1 initially added a separate `it.each` crash-loop (every enum value) alongside each effect assertion, which ballooned the suite to 70 tests. On reflection that was overkill — comparing two representative values already implicitly proves neither crashes, and the enum→token-key mapping is backed by TypeScript's own `satisfies Record<...>` constraints, so exhaustively sweeping every value added little real coverage. Merged each pair into one comparative test per axis in ce53a81: 70 → 36 tests. |
…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)
…utton/Button.Icon Covers #221's fix: a caller-supplied accessibilityState merges with the component's own computed disabled value, with disabled winning on conflict. The Button.Icon case specifically targets the bug #221's review caught — verified this exact test fails against that pre-fix IconButton code (disabled: true, caller's conflicting value winning) and passes against the fix.
|
@claude review |
|
Claude finished @Chibuzor-Nwemambu's task in 1m 48s —— View job Code review — PR #216
The re-review looks good. All four prior suggestions were handled sensibly — the smoke tests now assert real style effects (comparing two representative values via flattened style rather than hardcoded token values, so they survive token churn), and points 2–4 were reasonable judgment calls to defer/track. I re-verified the new assertions against the sources:
One observation (non-blocking)The two- // Option A — bind queries to each render explicitly, no reliance on screen rebinding
const { getByRole } = render(<Button label="Save" tone="accent" />);
const accentBorder = flattenStyle(getByRole("button"))?.borderColor;or a small helper that renders, extracts one value, and unmounts — since all these tests follow the identical "render → read one style prop → discard" shape, that'd also cut the duplication. Purely a maintainability call; the current tests are correct and green. Everything else — infra soundness, the |
Summary
test-utils.tsx(render wrapped inEDSProvider), an@expo/vector-iconsmock injest.setup.ts(fixes act() warnings from async font-loading state, while preserving real static properties like.fontsouseEDS.tsisn't silently broken), and@types/react-test-renderer(fixes RNTL query results resolving to TS'serrortype).jest.config.cjs's untested-component checklist: removes Button/Badge/Divider/Link/Typography (now tested), and also drops Dialog/ErrorBoundary/Icon/Label/Portal/Scrim/_internal(internal pieces covered indirectly via consumers) and Paper/PressableHighlight (slated for removal/replacement, not worth testing now).Button/Button.Icon'saccessibilityStatemerge — theButton.Iconcase specifically targets the JSX-ordering bug that PR fix(components): make component's own disabled state win accessibilityState merges #221's review caught, verified to fail against that pre-fix code.Contributes to #214 — does not close it (batches 2, SelectionControls, and the new-component-workflow update are still outstanding).
Test plan
pnpm test— 38/38 passing across 5 suitespnpm lint— cleanpnpm check-types— cleanpnpm build— clean, confirmedtest-utils.tsxdoesn't leak intodist/