Slider (v8): convert tests to use testing-library by TristanWatanabe · Pull Request #21919 · microsoft/fluentui

TristanWatanabe · 2022-03-02T04:43:50Z

Current Behavior

v8 Slider tests don't use testing-library

New Behavior

Convert Slider to fully use testing-library

Related Issue(s)

part of #21663

codesandbox-ci · 2022-03-02T04:48:57Z

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:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

size-auditor · 2022-03-02T04:55:59Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: f52f47c2c8db89c01f91e8f9b5107f39e4a8c28b (build)

fabricteam · 2022-03-02T04:57:49Z

📊 Bundle size report

_{🤖 This report was generated against f52f47c2c8db89c01f91e8f9b5107f39e4a8c28b}

fabricteam · 2022-03-02T05:11:01Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
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

ecraig12345 · 2022-03-02T21:08:16Z

packages/react/src/components/Slider/Slider.test.tsx

-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';


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

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}):

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.

Nevermind on this error, just needed to change my approach on testing it

ecraig12345

Looking good--thanks for doing this! A few suggestions.

ecraig12345 · 2022-03-02T21:12:05Z

packages/react/src/components/Slider/Slider.test.tsx

    // 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];


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.

Yup that's what I initially had in mind but apparently the label element doesn't have a corresponding aria role

Oh interesting...I didn't see any other suggestions from a brief online search, so I guess we can stick with this.

ecraig12345 · 2022-03-02T21:14:38Z

packages/react/src/components/Slider/Slider.test.tsx

    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} />);


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.

If I'm understanding this correctly, I think this should be covered by the renders correct aria-valuetext test below

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)

ecraig12345 · 2022-03-02T21:16:11Z

packages/react/src/components/Slider/Slider.test.tsx

    const onChanged = jest.fn();
    const slider = React.createRef<ISlider>();
-    wrapper = mount(<Slider onChange={onChange} defaultValue={5} onChanged={onChanged} componentRef={slider} />);
+    const { container, getByRole } = render(


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.

packages/react/src/components/Slider/Slider.test.tsx

TristanWatanabe added 3 commits March 1, 2022 20:35

convert Slider to testing-library

1475b07

update Slider snapshots

18b2bed

remove comment

7c35c94

msft-fluent-ui-bot added the Component: Slider label Mar 2, 2022

msft-fluent-ui-bot assigned micahgodbolt Mar 2, 2022

TristanWatanabe mentioned this pull request Mar 2, 2022

[Epic]: Migrate from enzyme to testing-library #21663

Closed

12 tasks

nit: remove usage of screen for consistency

1c3d646

TristanWatanabe marked this pull request as ready for review March 2, 2022 16:54

TristanWatanabe requested a review from a team as a code owner March 2, 2022 16:54

ecraig12345 reviewed Mar 2, 2022

View reviewed changes

TristanWatanabe added 2 commits March 2, 2022 16:41

use userEvent for keyboard events

992b8a3

clarify test names

ff6de02

ecraig12345 approved these changes Mar 3, 2022

View reviewed changes

TristanWatanabe merged commit e6fd843 into microsoft:master Mar 3, 2022

TristanWatanabe deleted the convert-slider-to-testing-lib branch March 3, 2022 02:34

TristanWatanabe changed the title ~~Slider: convert tests to use testing-library~~ Slider (v8): convert tests to use testing-library Mar 5, 2022

TristanWatanabe mentioned this pull request Apr 21, 2022

List of v8 components that need to be converted to use testing-library #22588

Closed

45 tasks

Conversation

TristanWatanabe commented Mar 2, 2022

Current Behavior

New Behavior

Related Issue(s)

Uh oh!

codesandbox-ci bot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

size-auditor bot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

fabricteam commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

fabricteam commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecraig12345 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codesandbox-ci bot commented Mar 2, 2022 •

edited

Loading

size-auditor bot commented Mar 2, 2022 •

edited

Loading

fabricteam commented Mar 2, 2022 •

edited

Loading

fabricteam commented Mar 2, 2022 •

edited

Loading