Fix a few more issues blocking React 17 in tests #22265
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ecraig12345
added this to In progress
in V-Build - @microsoft/fluentui-react-build
via automation
|
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 a13f9a8:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: fe7dbfd37146a6927bf8bda91c91780423af4599 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1077 | 1043 | 5000 | |
| Button | mount | 640 | 648 | 5000 | |
| FluentProvider | mount | 2092 | 2159 | 5000 | |
| FluentProviderWithTheme | mount | 324 | 329 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 268 | 285 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 352 | 362 | 10 | |
| MakeStyles | mount | 1843 | 1832 | 50000 |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AccordionMinimalPerf.default | 169 | 147 | 1.15:1 |
| TreeWith60ListItems.default | 200 | 178 | 1.12:1 |
| AvatarMinimalPerf.default | 219 | 199 | 1.1:1 |
| SkeletonMinimalPerf.default | 380 | 344 | 1.1:1 |
| PortalMinimalPerf.default | 196 | 179 | 1.09:1 |
| TextAreaMinimalPerf.default | 539 | 496 | 1.09:1 |
| ButtonSlotsPerf.default | 591 | 550 | 1.07:1 |
| ReactionMinimalPerf.default | 417 | 390 | 1.07:1 |
| AttachmentSlotsPerf.default | 1179 | 1113 | 1.06:1 |
| FlexMinimalPerf.default | 315 | 300 | 1.05:1 |
| ListCommonPerf.default | 692 | 661 | 1.05:1 |
| HeaderSlotsPerf.default | 831 | 800 | 1.04:1 |
| MenuButtonMinimalPerf.default | 1870 | 1796 | 1.04:1 |
| RefMinimalPerf.default | 263 | 253 | 1.04:1 |
| SegmentMinimalPerf.default | 363 | 348 | 1.04:1 |
| ButtonMinimalPerf.default | 174 | 169 | 1.03:1 |
| DropdownMinimalPerf.default | 3414 | 3322 | 1.03:1 |
| GridMinimalPerf.default | 362 | 351 | 1.03:1 |
| RosterPerf.default | 1233 | 1194 | 1.03:1 |
| RadioGroupMinimalPerf.default | 488 | 475 | 1.03:1 |
| IconMinimalPerf.default | 664 | 646 | 1.03:1 |
| ButtonOverridesMissPerf.default | 1659 | 1634 | 1.02:1 |
| CardMinimalPerf.default | 601 | 590 | 1.02:1 |
| CarouselMinimalPerf.default | 501 | 492 | 1.02:1 |
| DividerMinimalPerf.default | 386 | 378 | 1.02:1 |
| ProviderMinimalPerf.default | 450 | 442 | 1.02:1 |
| CustomToolbarPrototype.default | 3043 | 2974 | 1.02:1 |
| ChatMinimalPerf.default | 791 | 784 | 1.01:1 |
| DialogMinimalPerf.default | 837 | 826 | 1.01:1 |
| EmbedMinimalPerf.default | 4454 | 4407 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1257 | 1245 | 1.01:1 |
| PopupMinimalPerf.default | 682 | 677 | 1.01:1 |
| ProviderMergeThemesPerf.default | 1390 | 1370 | 1.01:1 |
| SplitButtonMinimalPerf.default | 4751 | 4696 | 1.01:1 |
| TreeMinimalPerf.default | 899 | 886 | 1.01:1 |
| ChatWithPopoverPerf.default | 413 | 415 | 1:1 |
| DatepickerMinimalPerf.default | 6644 | 6627 | 1:1 |
| DropdownManyItemsPerf.default | 733 | 730 | 1:1 |
| InputMinimalPerf.default | 1375 | 1370 | 1:1 |
| LabelMinimalPerf.default | 405 | 405 | 1:1 |
| MenuMinimalPerf.default | 911 | 913 | 1:1 |
| TextMinimalPerf.default | 378 | 378 | 1:1 |
| ToolbarMinimalPerf.default | 1039 | 1034 | 1:1 |
| TooltipMinimalPerf.default | 1171 | 1168 | 1:1 |
| AnimationMinimalPerf.default | 587 | 591 | 0.99:1 |
| ImageMinimalPerf.default | 379 | 383 | 0.99:1 |
| ListMinimalPerf.default | 559 | 565 | 0.99:1 |
| ListNestedPerf.default | 594 | 599 | 0.99:1 |
| ListWith60ListItems.default | 729 | 734 | 0.99:1 |
| TableManyItemsPerf.default | 2091 | 2107 | 0.99:1 |
| TableMinimalPerf.default | 416 | 422 | 0.99:1 |
| VideoMinimalPerf.default | 691 | 701 | 0.99:1 |
| AlertMinimalPerf.default | 284 | 290 | 0.98:1 |
| AttachmentMinimalPerf.default | 157 | 160 | 0.98:1 |
| CheckboxMinimalPerf.default | 2880 | 2926 | 0.98:1 |
| BoxMinimalPerf.default | 353 | 363 | 0.97:1 |
| SliderMinimalPerf.default | 1775 | 1825 | 0.97:1 |
| ChatDuplicateMessagesPerf.default | 299 | 312 | 0.96:1 |
| FormMinimalPerf.default | 417 | 436 | 0.96:1 |
| HeaderMinimalPerf.default | 362 | 376 | 0.96:1 |
| LoaderMinimalPerf.default | 711 | 742 | 0.96:1 |
| LayoutMinimalPerf.default | 357 | 381 | 0.94:1 |
| StatusMinimalPerf.default | 692 | 735 | 0.94:1 |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 893 | 908 | 5000 | |
| Breadcrumb | mount | 2743 | 2733 | 1000 | |
| Checkbox | mount | 1482 | 1449 | 5000 | |
| CheckboxBase | mount | 1248 | 1263 | 5000 | |
| ChoiceGroup | mount | 4703 | 4601 | 5000 | |
| ComboBox | mount | 954 | 992 | 1000 | |
| CommandBar | mount | 10308 | 10498 | 1000 | |
| ContextualMenu | mount | 11410 | 11426 | 1000 | |
| DefaultButton | mount | 1142 | 1138 | 5000 | |
| DetailsRow | mount | 3834 | 3893 | 5000 | |
| DetailsRowFast | mount | 3771 | 3762 | 5000 | |
| DetailsRowNoStyles | mount | 3607 | 3643 | 5000 | |
| Dialog | mount | 2195 | 2228 | 1000 | |
| DocumentCardTitle | mount | 156 | 179 | 1000 | |
| Dropdown | mount | 3319 | 3288 | 5000 | |
| FocusTrapZone | mount | 1793 | 1807 | 5000 | |
| FocusZone | mount | 1759 | 1816 | 5000 | |
| IconButton | mount | 1746 | 1758 | 5000 | |
| Label | mount | 351 | 354 | 5000 | |
| Layer | mount | 2888 | 2988 | 5000 | |
| Link | mount | 468 | 480 | 5000 | |
| MenuButton | mount | 1473 | 1482 | 5000 | |
| MessageBar | mount | 2121 | 2156 | 5000 | |
| Nav | mount | 3282 | 3265 | 1000 | |
| OverflowSet | mount | 1133 | 1092 | 5000 | |
| Panel | mount | 2139 | 2171 | 1000 | |
| Persona | mount | 1017 | 978 | 1000 | |
| Pivot | mount | 1447 | 1444 | 1000 | |
| PrimaryButton | mount | 1299 | 1290 | 5000 | |
| Rating | mount | 7743 | 7680 | 5000 | |
| SearchBox | mount | 1298 | 1289 | 5000 | |
| Shimmer | mount | 2502 | 2496 | 5000 | |
| Slider | mount | 1957 | 1953 | 5000 | |
| SpinButton | mount | 4956 | 5021 | 5000 | |
| Spinner | mount | 425 | 423 | 5000 | |
| SplitButton | mount | 3152 | 3121 | 5000 | |
| Stack | mount | 521 | 508 | 5000 | |
| StackWithIntrinsicChildren | mount | 2243 | 2235 | 5000 | |
| StackWithTextChildren | mount | 5211 | 5137 | 5000 | |
| SwatchColorPicker | mount | 11545 | 11527 | 5000 | |
| TagPicker | mount | 2695 | 2876 | 5000 | |
| TeachingBubble | mount | 90781 | 90329 | 5000 | |
| Text | mount | 440 | 420 | 5000 | |
| TextField | mount | 1391 | 1364 | 5000 | |
| ThemeProvider | mount | 1187 | 1173 | 5000 | |
| ThemeProvider | virtual-rerender | 647 | 662 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1883 | 1904 | 5000 | |
| Toggle | mount | 789 | 792 | 5000 | |
| buttonNative | mount | 126 | 135 | 5000 |
spmonahan
approved these changes
Mar 31, 2022
|
@ecraig12345 I tried to check out this PR and test it out. I used a yarn resolution to pin the repo to react 17. However, I'm getting errors when running the test. is there any other steps that are required to test this out ? I get this error as soon as mount is called in the |
Did you update the enzyme adapter (and references) to |
ecraig12345
force-pushed
the
react-17-pre
branch
2 times, most recently
from
5bbd414
to
cf09816
Compare
V-Build - @microsoft/fluentui-react-build
automation
moved this from In progress
to Done
Apr 5, 2022
V-Build - @microsoft/fluentui-react-build
automation
moved this from Done
to In progress
Apr 5, 2022
ecraig12345
force-pushed
the
react-17-pre
branch
from
cf09816
to
ac8a7f1
Compare
layershifter
approved these changes
Apr 6, 2022
packages/fluentui/react-northstar-fela-renderer/test/felaRenderer-test.tsx
Outdated
Show resolved
Hide resolved
| @@ -1,20 +1,42 @@ | |||
| import { createFelaRenderer } from '@fluentui/react-northstar-fela-renderer'; | |||
| import { ICSSInJSStyle } from '@fluentui/styles'; | |||
| // @ts-ignore | |||
| import { createSnapshot } from 'jest-react-fela'; | |||
| import { renderToString } from 'fela-tools'; | |||
There was a problem hiding this comment.
Wondering why nothing complains, but probably fela-tools should be added to devDependencies in packages/fluentui/react-northstar-fela-renderer/package.json or to a root package.json.
+"fela-tools": "^10.6.1",There was a problem hiding this comment.
It's in react-northstar devDependencies, plus it's a dep of various other things, so maybe it's working implicitly for that reason.
There was a problem hiding this comment.
I added it to react-northstar-fela-renderer for now since we don't currently have any fela stuff at the root. But at some point those dev deps should probably all go to the root for consistency.
V-Build - @microsoft/fluentui-react-build
automation
moved this from In progress
to Reviewer approved
Apr 6, 2022
ecraig12345
force-pushed
the
react-17-pre
branch
from
ac8a7f1
to
a13f9a8
Compare
|
@ling1726 I'm going to merge this to keep moving but let me know if you have more feedback later |
V-Build - @microsoft/fluentui-react-build
automation
moved this from Reviewer approved
to Done
Apr 6, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current Behavior
Assorted things in tests fail with React 17. I fixed some of them already (#21032) but this PR fixes a few more.
New Behavior
After this PR, at least as far as the things a local
yarn testcovers, we should be unblocked from upgrading to React 17 (with the community Enzyme adapter@wojtekmaj/enzyme-adapter-react-17).northstar changes
Add missing
act()wrappers in theEventListenertest.In
@fluentui/react-northstar-fela-renderer, replace the snapshot serializer fromjest-react-felawith a small custom one. The old serializer relied onhtmltojsx, which is extremely outdated (depends on React 15) and tries to import something fromreact-domwhich was removed in 17. (This fix could theoretically be contributed upstream later if someone wants, I just won't have time.)v8 changes
(When I previously attempted this, there wereThere also some really difficult issues with the FocusTrapZone tests in React 17, but we got rid of that problem by moving the tests to Cypress. Same with a few other components and testing-library.)
Related Issue(s)
Part of #20145