-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CommandBar (v8): convert tests to use testing-library #22247
CommandBar (v8): convert tests to use testing-library #22247
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit dcd935f:
|
menuItem.simulate('click'); | ||
|
||
expect(document.querySelector('.SubMenuClass')).toBeTruthy(); | ||
const menuItem = getByRole('menuitem', { hidden: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another case that testing-library deems as "inaccessible" for seemingly no reason 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was bothering me enough that I started looking at the testing-library code, and I figured it out...it's the measurement/overflow logic in some of our components, including CommandBar and Breadcrumb. I don't know the details of how measurement works, but the general idea is that it initially renders stuff hidden and/or off-screen to get the size, then re-renders with appropriate size and overflow items. getByRole
is running against that initial measurement render, where the elements are in fact hidden.
How I figured this out was a little over-complicated, and in retrospect I probably should have tried running the test and stepping through the code first. 😆
What I actually did: I started by looking in @testing-library/dom
's code for the implementation of the role
query and searching for hidden
. That led to the isInaccessible
implementation. I wasn't sure exactly what would cause issues there, so I made a codepen that renders a basic Breadcrumb (since it has the same problem and is a little simpler) and runs isInaccessible
against the list items after render.
Turns out getComputedStyle(element).visibility
was returning 'hidden'
. This seemed weird (since I could see the breadcrumb on the page) but then I remembered that Breadcrumb and CommandBar have measurement with initial hidden rendering. So I tried running isInaccessible
on the items again after a 500ms timeout and it returns false (is accessible) as expected.
https://codepen.io/ecraig12345/pen/RwxZwdN?editors=0011
Anyway...the possible solutions for the tests are:
- keep
{ hidden: true }
and add comments that it's due to the measurement logic - use fake timers and run timers before doing
getByRole
on items
Definitely any tests that involve overflow should use fake timers, but in other cases it's up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow - the initial hidden rendering of some of our components is quite the gotcha in this case😆. Thanks a lot for looking into this!
📊 Bundle size report🤖 This report was generated against 50b0659126a88c9690b9fdfccf755d08617e395e |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 50b0659126a88c9690b9fdfccf755d08617e395e (build) |
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
BaseButton | mount | 931 | 985 | 5000 | |
Breadcrumb | mount | 2800 | 2819 | 1000 | |
Checkbox | mount | 1525 | 1518 | 5000 | |
CheckboxBase | mount | 1305 | 1314 | 5000 | |
ChoiceGroup | mount | 4834 | 4863 | 5000 | |
ComboBox | mount | 1000 | 1047 | 1000 | |
CommandBar | mount | 10758 | 10805 | 1000 | |
ContextualMenu | mount | 11878 | 12140 | 1000 | |
DefaultButton | mount | 1182 | 1178 | 5000 | |
DetailsRow | mount | 3927 | 3915 | 5000 | |
DetailsRowFast | mount | 3978 | 4049 | 5000 | |
DetailsRowNoStyles | mount | 3833 | 3776 | 5000 | |
Dialog | mount | 2270 | 2275 | 1000 | |
DocumentCardTitle | mount | 153 | 182 | 1000 | |
Dropdown | mount | 3371 | 3377 | 5000 | |
FocusTrapZone | mount | 1880 | 1897 | 5000 | |
FocusZone | mount | 1872 | 1925 | 5000 | |
IconButton | mount | 1937 | 1800 | 5000 | |
Label | mount | 352 | 362 | 5000 | |
Layer | mount | 3060 | 3061 | 5000 | |
Link | mount | 476 | 504 | 5000 | |
MenuButton | mount | 1527 | 1509 | 5000 | |
MessageBar | mount | 2218 | 2253 | 5000 | |
Nav | mount | 3352 | 3361 | 1000 | |
OverflowSet | mount | 1104 | 1114 | 5000 | |
Panel | mount | 2247 | 2181 | 1000 | |
Persona | mount | 1037 | 1035 | 1000 | |
Pivot | mount | 1465 | 1471 | 1000 | |
PrimaryButton | mount | 1288 | 1317 | 5000 | |
Rating | mount | 7855 | 7871 | 5000 | |
SearchBox | mount | 1379 | 1368 | 5000 | |
Shimmer | mount | 2530 | 2534 | 5000 | |
Slider | mount | 1932 | 1969 | 5000 | |
SpinButton | mount | 5215 | 5095 | 5000 | |
Spinner | mount | 438 | 446 | 5000 | |
SplitButton | mount | 3229 | 3235 | 5000 | |
Stack | mount | 519 | 546 | 5000 | |
StackWithIntrinsicChildren | mount | 2454 | 2314 | 5000 | |
StackWithTextChildren | mount | 5455 | 5346 | 5000 | |
SwatchColorPicker | mount | 11993 | 12802 | 5000 | |
TagPicker | mount | 2806 | 2721 | 5000 | |
TeachingBubble | mount | 100883 | 100565 | 5000 | |
Text | mount | 427 | 426 | 5000 | |
TextField | mount | 1437 | 1471 | 5000 | |
ThemeProvider | mount | 1206 | 1192 | 5000 | |
ThemeProvider | virtual-rerender | 649 | 652 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 1924 | 1910 | 5000 | |
Toggle | mount | 832 | 820 | 5000 | |
buttonNative | mount | 123 | 150 | 5000 |
Current Behavior
CommandBar
tests don't use testing-libraryNew Behavior
CommandBar
to fully use testing-libraryRelated Issue(s)
part of #21663