Slider (v8): convert tests to use testing-library #21919
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 ff6de02:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: f52f47c2c8db89c01f91e8f9b5107f39e4a8c28b (build) |
📊 Bundle size report🤖 This report was generated against f52f47c2c8db89c01f91e8f9b5107f39e4a8c28b |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1007 | 1055 | 5000 | |
| BaseButton | mount | 1080 | 1102 | 5000 | |
| Breadcrumb | mount | 2881 | 2905 | 1000 | |
| ButtonNext | mount | 568 | 555 | 5000 | |
| Checkbox | mount | 1804 | 1801 | 5000 | |
| CheckboxBase | mount | 1568 | 1582 | 5000 | |
| ChoiceGroup | mount | 5672 | 5619 | 5000 | |
| ComboBox | mount | 1081 | 1143 | 1000 | |
| CommandBar | mount | 11334 | 11350 | 1000 | |
| ContextualMenu | mount | 9223 | 9293 | 1000 | |
| DefaultButton | mount | 1322 | 1348 | 5000 | |
| DetailsRow | mount | 4292 | 4360 | 5000 | |
| DetailsRowFast | mount | 4367 | 4354 | 5000 | |
| DetailsRowNoStyles | mount | 4018 | 4064 | 5000 | |
| Dialog | mount | 2574 | 2552 | 1000 | |
| DocumentCardTitle | mount | 213 | 213 | 1000 | |
| Dropdown | mount | 3742 | 3748 | 5000 | |
| FluentProviderNext | mount | 2098 | 2013 | 5000 | |
| FluentProviderWithTheme | mount | 179 | 199 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 130 | 144 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 224 | 224 | 10 | |
| FocusTrapZone | mount | 2068 | 2127 | 5000 | |
| FocusZone | mount | 2021 | 2054 | 5000 | |
| IconButton | mount | 2092 | 2061 | 5000 | |
| Label | mount | 425 | 424 | 5000 | |
| Layer | mount | 3465 | 3573 | 5000 | |
| Link | mount | 573 | 573 | 5000 | |
| MakeStyles | mount | 1843 | 1833 | 50000 | |
| MenuButton | mount | 1761 | 1756 | 5000 | |
| MessageBar | mount | 2204 | 2232 | 5000 | |
| Nav | mount | 3873 | 3815 | 1000 | |
| OverflowSet | mount | 1241 | 1277 | 5000 | |
| Panel | mount | 2482 | 2436 | 1000 | |
| Persona | mount | 980 | 1013 | 1000 | |
| Pivot | mount | 1651 | 1644 | 1000 | |
| PrimaryButton | mount | 1487 | 1519 | 5000 | |
| Rating | mount | 9168 | 9232 | 5000 | |
| SearchBox | mount | 1627 | 1629 | 5000 | |
| Shimmer | mount | 3025 | 2942 | 5000 | |
| Slider | mount | 2241 | 2230 | 5000 | |
| SpinButton | mount | 5699 | 5714 | 5000 | |
| Spinner | mount | 517 | 514 | 5000 | |
| SplitButton | mount | 3698 | 3620 | 5000 | |
| Stack | mount | 655 | 663 | 5000 | |
| StackWithIntrinsicChildren | mount | 2850 | 2806 | 5000 | |
| StackWithTextChildren | mount | 6457 | 6398 | 5000 | |
| SwatchColorPicker | mount | 13169 | 13171 | 5000 | |
| TagPicker | mount | 3144 | 3025 | 5000 | |
| TeachingBubble | mount | 14634 | 14814 | 5000 | |
| Text | mount | 540 | 538 | 5000 | |
| TextField | mount | 1622 | 1676 | 5000 | |
| ThemeProvider | mount | 1396 | 1372 | 5000 | |
| ThemeProvider | virtual-rerender | 701 | 695 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 2188 | 2185 | 5000 | |
| Toggle | mount | 948 | 994 | 5000 | |
| buttonNative | mount | 181 | 169 | 5000 |
| import { resetIds, KeyCodes } from '@fluentui/utilities'; | ||
| import { create } from '@fluentui/utilities/lib/test'; | ||
| import { resetIds, KeyCodes } from '../../Utilities'; | ||
| import { fireEvent, render } from '@testing-library/react'; |
There was a problem hiding this comment.
This is going to be in the testing guidance, but it's generally recommended to use userEvent rather than fireEvent, since it more realistically simulates the full sequence of events triggered by user interaction. https://www.npmjs.com/package/@testing-library/user-event
There was a problem hiding this comment.
I can port some of the mouseclick behaviors to use userEvent but there are some tests (such as checking if onChange triggers on a mousedown click while onChanged does not) where using fireEvent.mousedown was needed. Also, for the keyboard tests, using userEvent.type(element, '{arrowdown}') syntax to simulate a keydown was throwing the error below which is why I settled on using fireEvent.keydown(element, {keyCode}):
There was a problem hiding this comment.
Oh interesting, I bet it's because it tries to do a click first. The docs mention there's a skipClick option--what happens if you use that?
type will click the element before typing. To disable this, set the skipClick option to true.
There was a problem hiding this comment.
Nevermind on this error, just needed to change my approach on testing it
|
|
||
| // properly associates label with custom id | ||
| const label = wrapper.find('label.test_label'); | ||
| expect(label.prop('htmlFor')).toBe('test_id'); | ||
| const label = container.getElementsByClassName('test_label')[0]; |
There was a problem hiding this comment.
I think it might be slightly better/more idiomatic to get the label by role, then test its class and for. Something like this (haven't run this in TS and might be remembering property names incorrectly).
const label = getByRole('label');
expect(label.className).toMatch(/test_label/);
expect(label.getAttribute('for')).toBe('test_id');And similar in the next test.
There was a problem hiding this comment.
Yup that's what I initially had in mind but apparently the label element doesn't have a corresponding aria role
There was a problem hiding this comment.
Oh interesting...I didn't see any other suggestions from a brief online search, so I guess we can stick with this.
| expect(slider.current!.value).toEqual(0); | ||
| }); | ||
|
|
||
| it('handles zero value', () => { | ||
| const slider = React.createRef<ISlider>(); | ||
|
|
||
| wrapper = mount(<Slider value={0} min={-100} max={100} componentRef={slider} />); | ||
| render(<Slider value={0} min={-100} max={100} componentRef={slider} />); |
There was a problem hiding this comment.
Not a new issue but it would be nice if we could test something in the DOM to make sure the correct value is rendered as well. Since this implementation doesn't use a native input, you'd have to look if there are any aria attributes or something that you could check.
There was a problem hiding this comment.
If I'm understanding this correctly, I think this should be covered by the renders correct aria-valuetext test below
There was a problem hiding this comment.
Right, I guess it just concerns me somewhat that we're checking the internal component state without checking what it's actually rendering (in case there's a mismatch)
| expect(slider.current!.value).toEqual(0); | ||
| }); | ||
|
|
||
| it('calls onChange and onChanged when slider value changes with mouse', () => { | ||
| const onChange = jest.fn(); | ||
| const onChanged = jest.fn(); | ||
| const slider = React.createRef<ISlider>(); | ||
| wrapper = mount(<Slider onChange={onChange} defaultValue={5} onChanged={onChanged} componentRef={slider} />); | ||
| const { container, getByRole } = render( |
There was a problem hiding this comment.
I wonder if this would be a good candidate for doing an additional test in cypress to ensure it works in real scenarios as well. But that's probably lower priority if the jest test is reliable.
TristanWatanabe
changed the title
Current Behavior
Slidertests don't use testing-libraryNew Behavior
Related Issue(s)
part of #21663