Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix test issues discovered while trying to test React 17 #17050

Closed
wants to merge 1 commit into from
Closed

Fix test issues discovered while trying to test React 17 #17050

wants to merge 1 commit into from

Conversation

ecraig12345
Copy link
Member

@ecraig12345 ecraig12345 commented Feb 18, 2021 •
edited
Loading

Pull request checklist

Description of changes

While trying to run tests with React 17 (see #16886), I found a few instances where either:

  • Tests were doing things incorrectly in ways that (for whatever reason) worked in master, but doesn't work after upgrading React/Jest
  • Tests were doing things in a less than ideal way which outright stops working after upgrading React/Jest

This PR fixes those issues where it makes sense to go ahead and do so. Some common ones:

  • React 17-related test utilities have a nice warning when you use the wrong act helper, which several tests were.
    • When rendering with react-dom, should use act from react-dom/test-utils
    • When rendering with Enzyme, should use act from enzyme
  • React 17 and/or Jest 25 (and its jsdom version) make focus-related tests trickier. There are a few issues I've yet to resolve, but in a lot of cases the resolution is simple: attach the root element to the document when testing.
  • In a few cases, React 17 is pickier about wrapping things such as unmount() with act. Go ahead and add this since it's harmless.

To help with attaching to the document, I added a new attach parameter to @fluentui/test-utilities safeMount. I also added a new safeRender utility which wraps ReactDOM.render with cleanup and an attach parameter.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 18, 2021 •
edited
Loading

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 64cbeaf:

Sandbox Source
Fluent UI Button Configuration
codesandbox-react-template Configuration
codesandbox-react-northstar-template Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Feb 18, 2021 •
edited
Loading

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 870 866 5000
BaseButton mount 855 859 5000
Breadcrumb mount 41560 41521 5000
ButtonNext mount 571 551 5000
Checkbox mount 1468 1455 5000
CheckboxBase mount 1239 1266 5000
ChoiceGroup mount 4522 4512 5000
ComboBox mount 934 916 1000
CommandBar mount 9935 9753 1000
ContextualMenu mount 6234 6201 1000
DefaultButton mount 1108 1156 5000
DetailsRow mount 3597 3714 5000
DetailsRowFast mount 3629 3673 5000
DetailsRowNoStyles mount 3451 3463 5000
Dialog mount 1437 1467 1000
DocumentCardTitle mount 1833 1822 1000
Dropdown mount 3224 3241 5000
FocusTrapZone mount 1798 1801 5000
FocusZone mount 1778 1797 5000
IconButton mount 1714 1715 5000
Label mount 327 348 5000
Layer mount 1772 1755 5000
Link mount 449 470 5000
MakeStyles mount 1795 1782 50000
MenuButton mount 1444 1453 5000
MessageBar mount 1998 2029 5000
Nav mount 3260 3255 1000
OverflowSet mount 1042 1048 5000
Panel mount 1358 1451 1000
Persona mount 806 802 1000
Pivot mount 1411 1425 1000
PrimaryButton mount 1295 1271 5000
Rating mount 7507 7616 5000
SearchBox mount 1293 1292 5000
Shimmer mount 2496 2506 5000
Slider mount 1969 1964 5000
SpinButton mount 4932 5289 5000
Spinner mount 423 444 5000
SplitButton mount 3111 3121 5000
Stack mount 513 494 5000
StackWithIntrinsicChildren mount 1552 1559 5000
StackWithTextChildren mount 4541 4552 5000
SwatchColorPicker mount 10263 10164 5000
Tabs mount 1385 1395 1000
TagPicker mount 2809 2841 5000
TeachingBubble mount 11715 11814 5000
Text mount 435 416 5000
TextField mount 1370 1341 5000
ThemeProvider mount 1177 1195 5000
ThemeProvider virtual-rerender 611 602 5000
ThemeProviderNext mount 15867 15992 5000
Toggle mount 792 806 5000
buttonNative mount 116 113 5000

Perf Analysis (Fluent)

⚠️ 1 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
ButtonMinimalPerf.default 176 166 1.06:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🦄 Avatar.Fluent 0.16 0.45 0.36:1 2000 329
🦄 Button.Fluent 0.11 0.19 0.58:1 5000 537
🔧 Checkbox.Fluent 0.61 0.35 1.74:1 1000 609
🎯 Dialog.Fluent 0.14 0.2 0.7:1 5000 712
🔧 Dropdown.Fluent 2.98 0.37 8.05:1 1000 2975
🔧 Icon.Fluent 0.12 0.06 2:1 5000 613
🦄 Image.Fluent 0.07 0.12 0.58:1 5000 365
🔧 Slider.Fluent 1.51 0.44 3.43:1 1000 1514
🔧 Text.Fluent 0.07 0.03 2.33:1 5000 354
🦄 Tooltip.Fluent 0.14 0.85 0.16:1 5000 678

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
RefMinimalPerf.default 243 219 1.11:1
ImageMinimalPerf.default 385 353 1.09:1
AttachmentMinimalPerf.default 168 156 1.08:1
CarouselMinimalPerf.default 465 434 1.07:1
DividerMinimalPerf.default 379 355 1.07:1
GridMinimalPerf.default 351 330 1.06:1
LabelMinimalPerf.default 389 368 1.06:1
AvatarMinimalPerf.default 215 205 1.05:1
HeaderMinimalPerf.default 366 348 1.05:1
ReactionMinimalPerf.default 377 359 1.05:1
VideoMinimalPerf.default 616 586 1.05:1
Avatar.Fluent 329 314 1.05:1
ChatWithPopoverPerf.default 380 365 1.04:1
HeaderSlotsPerf.default 741 715 1.04:1
TextAreaMinimalPerf.default 490 470 1.04:1
AnimationMinimalPerf.default 408 395 1.03:1
CardMinimalPerf.default 538 524 1.03:1
RadioGroupMinimalPerf.default 418 404 1.03:1
SegmentMinimalPerf.default 354 344 1.03:1
SkeletonMinimalPerf.default 356 344 1.03:1
SliderMinimalPerf.default 1560 1508 1.03:1
TooltipMinimalPerf.default 939 914 1.03:1
AlertMinimalPerf.default 273 268 1.02:1
ItemLayoutMinimalPerf.default 1217 1196 1.02:1
LayoutMinimalPerf.default 363 355 1.02:1
PortalMinimalPerf.default 167 163 1.02:1
IconMinimalPerf.default 581 571 1.02:1
TextMinimalPerf.default 351 345 1.02:1
ToolbarMinimalPerf.default 916 896 1.02:1
Button.Fluent 537 524 1.02:1
Checkbox.Fluent 609 599 1.02:1
Icon.Fluent 613 599 1.02:1
Text.Fluent 354 348 1.02:1
ButtonOverridesMissPerf.default 1648 1639 1.01:1
ChatDuplicateMessagesPerf.default 286 284 1.01:1
DropdownMinimalPerf.default 3000 2983 1.01:1
ListWith60ListItems.default 610 605 1.01:1
MenuButtonMinimalPerf.default 1507 1487 1.01:1
PopupMinimalPerf.default 685 678 1.01:1
SplitButtonMinimalPerf.default 3611 3568 1.01:1
TableManyItemsPerf.default 1828 1817 1.01:1
TreeMinimalPerf.default 758 749 1.01:1
Dialog.Fluent 712 703 1.01:1
ChatMinimalPerf.default 596 596 1:1
DatepickerMinimalPerf.default 42592 42533 1:1
DropdownManyItemsPerf.default 650 651 1:1
EmbedMinimalPerf.default 3981 3992 1:1
ListMinimalPerf.default 484 484 1:1
ProviderMergeThemesPerf.default 1633 1635 1:1
ProviderMinimalPerf.default 973 976 1:1
StatusMinimalPerf.default 659 656 1:1
Dropdown.Fluent 2975 2966 1:1
Tooltip.Fluent 678 681 1:1
AttachmentSlotsPerf.default 1109 1121 0.99:1
CheckboxMinimalPerf.default 2631 2657 0.99:1
FormMinimalPerf.default 395 399 0.99:1
LoaderMinimalPerf.default 679 683 0.99:1
MenuMinimalPerf.default 850 855 0.99:1
TableMinimalPerf.default 398 403 0.99:1
CustomToolbarPrototype.default 3676 3721 0.99:1
Slider.Fluent 1514 1531 0.99:1
ButtonSlotsPerf.default 528 537 0.98:1
ButtonUseCssNestingPerf.default 1027 1050 0.98:1
DialogMinimalPerf.default 701 713 0.98:1
FlexMinimalPerf.default 303 308 0.98:1
InputMinimalPerf.default 1209 1233 0.98:1
ListNestedPerf.default 534 544 0.98:1
ButtonUseCssPerf.default 750 771 0.97:1
ListCommonPerf.default 576 596 0.97:1
RosterPerf.default 1126 1163 0.97:1
Image.Fluent 365 378 0.97:1
AccordionMinimalPerf.default 145 153 0.95:1
BoxMinimalPerf.default 344 364 0.95:1
TreeWith60ListItems.default 167 177 0.94:1

@size-auditor
Copy link

size-auditor bot commented Feb 18, 2021 •
edited
Loading

Asset size changes

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

Baseline commit: e67afa33850a94a97f97aa7b67dc89c2c15f49c3 (build)

@ecraig12345 ecraig12345 force-pushed the react-17-test-fixes branch 3 times, most recently from 4dd60ea to 5e28f09 Compare February 18, 2021 21:11
khmakoto
khmakoto approved these changes Feb 19, 2021
View reviewed changes
Copy link
Member

@khmakoto khmakoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

khmakoto
khmakoto requested changes Feb 19, 2021
View reviewed changes
Copy link
Member

@khmakoto khmakoto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it seems like some FocusTrapZone tests are failing so it'd be great if we could have them working as expected again.

@msft-fluent-ui-bot
Copy link
Collaborator

This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI!

@ecraig12345 ecraig12345 mentioned this pull request Mar 1, 2021
Closed
5 tasks
@ecraig12345 ecraig12345 force-pushed the react-17-test-fixes branch from 2cfb310 to 968bd46 Compare April 10, 2021 03:04
@ecraig12345 ecraig12345 requested review from ThomasMichon and a team as code owners April 10, 2021 03:04
@ecraig12345 ecraig12345 force-pushed the react-17-test-fixes branch from 968bd46 to 64cbeaf Compare April 10, 2021 03:09
varholak-peter
varholak-peter approved these changes Sep 22, 2021
View reviewed changes
Copy link
Contributor

@varholak-peter varholak-peter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🇮🇸

Please resolve conflicts and this can be merged. ✅

@TristanWatanabe TristanWatanabe mentioned this pull request Nov 4, 2021
Closed
@ecraig12345 ecraig12345 mentioned this pull request Dec 14, 2021
Merged
2 tasks
@ecraig12345 ecraig12345 deleted the react-17-test-fixes branch December 15, 2021 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khmakoto khmakoto khmakoto requested changes

@varholak-peter varholak-peter varholak-peter approved these changes

@behowell behowell Awaiting requested review from behowell behowell is a code owner

@dzearing dzearing Awaiting requested review from dzearing dzearing is a code owner

@joschect joschect Awaiting requested review from joschect

@xugao xugao Awaiting requested review from xugao

@ThomasMichon ThomasMichon Awaiting requested review from ThomasMichon ThomasMichon is a code owner

Assignees

@joschect joschect

Labels
Area: Testing React 17
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ecraig12345 @fabricteam @msft-fluent-ui-bot @khmakoto @varholak-peter @joschect