fix: remove hardcoded default testIDs from components - #5088
Open
k0ndee wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.