-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Callout, Dialog, Modal: convert tests to use testing-library #21664
Callout, Dialog, Modal: convert tests to use testing-library #21664
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 eba0ae2:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: e07cc826b280603418e2e8bc79b161e459de9741 (build) |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
ContextualMenu | mount | 7297 | 36414 | 1000 | Possible regression |
FluentProviderWithTheme | mount | 133 | 148 | 10 | Possible regression |
All results
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 749 | 750 | 5000 | |
BaseButton | mount | 806 | 766 | 5000 | |
Breadcrumb | mount | 2344 | 2287 | 1000 | |
ButtonNext | mount | 423 | 418 | 5000 | |
Checkbox | mount | 1318 | 1342 | 5000 | |
CheckboxBase | mount | 1151 | 1107 | 5000 | |
ChoiceGroup | mount | 4149 | 4139 | 5000 | |
ComboBox | mount | 855 | 859 | 1000 | |
CommandBar | mount | 8874 | 8898 | 1000 | |
ContextualMenu | mount | 7297 | 36414 | 1000 | Possible regression |
DefaultButton | mount | 1497 | 989 | 5000 | |
DetailsRow | mount | 3190 | 3198 | 5000 | |
DetailsRowFast | mount | 4194 | 3203 | 5000 | |
DetailsRowNoStyles | mount | 3110 | 3036 | 5000 | |
Dialog | mount | 1902 | 1925 | 1000 | |
DocumentCardTitle | mount | 151 | 165 | 1000 | |
Dropdown | mount | 2773 | 3542 | 5000 | |
FluentProviderNext | mount | 1649 | 1605 | 5000 | |
FluentProviderWithTheme | mount | 133 | 148 | 10 | Possible regression |
FluentProviderWithTheme | virtual-rerender | 102 | 104 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 169 | 168 | 10 | |
FocusTrapZone | mount | 1616 | 1509 | 5000 | |
FocusZone | mount | 1537 | 1560 | 5000 | |
IconButton | mount | 1501 | 1499 | 5000 | |
Label | mount | 318 | 317 | 5000 | |
Layer | mount | 2590 | 2580 | 5000 | |
Link | mount | 438 | 428 | 5000 | |
MakeStyles | mount | 1492 | 1492 | 50000 | |
MenuButton | mount | 1266 | 1272 | 5000 | |
MessageBar | mount | 1769 | 1750 | 5000 | |
Nav | mount | 2848 | 2829 | 1000 | |
OverflowSet | mount | 968 | 977 | 5000 | |
Panel | mount | 1847 | 1887 | 1000 | |
Persona | mount | 761 | 767 | 1000 | |
Pivot | mount | 1278 | 1258 | 1000 | |
PrimaryButton | mount | 1090 | 1124 | 5000 | |
Rating | mount | 6625 | 6668 | 5000 | |
SearchBox | mount | 1184 | 1159 | 5000 | |
Shimmer | mount | 2201 | 2191 | 5000 | |
Slider | mount | 1694 | 1712 | 5000 | |
SpinButton | mount | 4282 | 4328 | 5000 | |
Spinner | mount | 379 | 397 | 5000 | |
SplitButton | mount | 2744 | 2715 | 5000 | |
Stack | mount | 475 | 469 | 5000 | |
StackWithIntrinsicChildren | mount | 1990 | 2003 | 5000 | |
StackWithTextChildren | mount | 4505 | 4510 | 5000 | |
SwatchColorPicker | mount | 11901 | 9974 | 5000 | |
TagPicker | mount | 2200 | 2235 | 5000 | |
TeachingBubble | mount | 10874 | 11291 | 5000 | |
Text | mount | 386 | 337 | 5000 | |
TextField | mount | 1214 | 1245 | 5000 | |
ThemeProvider | mount | 1020 | 1047 | 5000 | |
ThemeProvider | virtual-rerender | 555 | 549 | 5000 | |
ThemeProvider | virtual-rerender-with-unmount | 1607 | 1622 | 5000 | |
Toggle | mount | 719 | 706 | 5000 | |
buttonNative | mount | 141 | 125 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
}); | ||
|
||
it('target MouseEvents does not throw exception', () => { | ||
it('does not throw with target MouseEvent', () => { | ||
const mouseEvent = document.createEvent('MouseEvent'); |
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.
Nit: I know you're just migrating from one testing library to another but should this be changed from document.createEvent....
to new MouseEvent()
?
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.
Sure, maybe it was due to the API not being available in the past? But I'm changing various other things like that.
isFocused = options.containsFocus; | ||
restoreCalled = true; | ||
}; | ||
const onRestoreFocus = jest.fn(); | ||
// In order to have eventlisteners that have been added to the window to be called the JSX needs |
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.
Is this comment still relevant?
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.
Nope, will remove
b855377
to
6187fc3
Compare
Current Behavior
Callout, Dialog, and Modal's tests use a mix of enzyme, react-test-renderer, and react-dom, along with assorted odd patterns such as unnecessary snapshot testing.
New Behavior
Convert the tests to all use testing-library. In some cases I also tweaked what the tests were doing to be simpler or to test things more directly/effectively: for example, replacing some of the snapshot tests with direct tests of the relevant DOM.
Please review snapshot updates with whitespace changes hidden!
I noticed that some of the tests were having issues due to conformance tests not fully cleaning up. As a temporary workaround in react-conformance (until it's also converted to use testing-library), I added an
afterEach
which doesdocument.body.innerHTML = ''
.Related Issue(s)
Part of #21663