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.
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.