fix: remove hardcoded default testIDs from components - #5088
Conversation
|
@k0ndee we've got a bunch of conflicts |
13e6bdf to
10ebef0
Compare
| elevated = false, | ||
| theme: themeOverrides, | ||
| testID = 'appbar-header', | ||
| testID, |
There was a problem hiding this comment.
The removed defaults need a bullet in the migration guide's "Test IDs" section (docs/6.x/docs/guides/migration.md) —- it already lists the ones Surface and Appbar.Header lost. Otherwise anyone selecting button, card or search-bar in their e2e tests loses those matches silently.
There was a problem hiding this comment.
Good point. Added information to migration guide.
| theme={theme} | ||
| elevation={elevation} | ||
| testID={`${testID}-container`} | ||
| testID={testID ? `${testID}-container` : undefined} |
There was a problem hiding this comment.
Five other components build child testIDs without this guard, so they render a literal
undefined-text - visible in the committed snapshots today. Checkbox/CheckboxItem.tsx:177, DataTable/DataTableCell.tsx:111, List/ListItem.tsx:245, SegmentedButtons/SegmentedButtonItem.tsx:227 and Snackbar.tsx:341. Worth including them?
There was a problem hiding this comment.
Missed them, included in the new commit.
satya164
left a comment
satya164
left a comment
There was a problem hiding this comment.
I think there many test IDs added to views to solely test the styles or props in unit tests, even though they are internal elements. Majority of them are things that add text to testID, e.g. ${testID}-icon-wrapper.
Let's remove them as well. Test IDs should only be present on elements that users need to access for public behavior tests, e.g. tap/interact with an element or assert content of an element.
@satya164 there is a separate task for derived testIDs and I already created a draft PR #5099 that addresses that. If You'd like I can merge those two together so both default and derived testID are handled in one PR. |
|
@k0ndee then i'll merge this one and check that one separately |
main removed the hardcoded default testIDs from Chip and IconButton (callstack#5088). Guard Chip's close-icon testID the same way its container already is, and pass explicit testID props in the hitSlop/close-icon tests that relied on the old defaults.
main removed the hardcoded default testIDs from Chip and IconButton (callstack#5088). Guard Chip's close-icon testID the same way its container already is, and pass explicit testID props in the hitSlop/close-icon tests that relied on the old defaults.
- Drop `accessible` from the two List.Sections. With it, iOS collapsed each
section into a single accessibility element (XCUITest tree evidence in
evidence/a11y/). `testID` alone resolves `screenshot --crop-on` on both
platforms with identical rects, so the example app's accessibility tree
is unchanged by the hook.
- Rename the test ids to surface-example-{elevated,flat} so they cannot be
confused with the library defaults removed in #5088 / #5099. Baselines
renamed to __baselines__/<platform>/<story>.png on both platforms.
- Commit the evidence: raw diff JSON behind every table, break diff images,
issues.md, the a11y trees, and the final script runs.
- run.mjs: env.json mismatch and crop-size mismatch are hard failures
(exit 2 / 3) unless --force; --update can create a missing baseline;
always relaunch so the bundle is fresh (Fast Refresh was seen to stop
reaching Android); handle the Expo dev menu and dev launcher; because
the example app persists navigation state, go Back to the list root and
press the Surface row by position, not the header by label; --force-full
on every snapshot; summary.json written on every exit; --out.
- Re-measure the realistic break three times per platform at pre-declared
thresholds: bit-identical (iOS 10,179 px, Android 9,336 px at 0.02;
0 at 0.1) and identical to the original single captures.
- README: status, evidence links, a11y section, re-measurement section,
issues 13-18, updated Running-it.
3 participants
Motivation
Several components fell back to a hardcoded default
testID(e.g.testID = 'button',testID = 'card',testID = 'search-bar') whenever a consumer didn't pass one explicitly. This meant every instance of a component rendered without an explicittestIDstill emitted a fixed, non-configurable testID into the tree leading to duplicate/colliding testIDs when multiple instances of the same component render on screen.This PR removes all hardcoded default
testIDvalues so thattestIDisundefinedunless a consumer explicitly provides one. Suffixed child testIDs (e.g.`${testID}-container`) are now only rendered when atestIDis explicitly given, instead of always deriving from the removed default.Along the way this surfaced two latent bugs that the hardcoded defaults had been masking:
IconButtonnever forwarded itstestIDdown into the inner icon, relying onCrossFadeIcon's own default instead. Now it forwards${testID}-iconexplicitly.FAB.Menu's trigger wrapper puttestIDon a non-interactive positioningViewinstead of the actual pressableShell, which only worked before becauseShellhad its own hardcoded default in the right place.Affected components:
Button,Card,Chip,Surface,Modal,Menu,Menu.Item,IconButton,Searchbar,CrossFadeIcon,ProgressBar,Appbar.Header,Appbar.Content,Drawer.CollapsedItem,FAB,FAB.Extended,FAB.Menu,BottomNavigation,BottomNavigationBar.Test plan
yarn typecheckpasses.yarn lintpasses.yarn jestpasses (55 suites, 732 tests, 169 snapshots — snapshots updated to reflect the removed default testID attributes).Button,Card,Menu,Menu.Item,Drawer.CollapsedItem,FAB.Menu,ListItem,Appbar,BottomNavigation) to pass an explicittestIDwhere the assertion actually needs one.testID, only the implicit-default fallback is removed.