Port react-conformance tests to testing-library #22009
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
requested review from
a team,
khmakoto,
Jahnp and
Vitalius1
as code owners
|
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 50cfdc4:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 3e9421d1cf1545a8ab9fce425f3695e62ac99d73 (build) |
ecraig12345
commented
Mar 8, 2022
| @@ -66,22 +75,20 @@ export function isConformant( | |||
| } = options; | |||
|
|
|||
| const defaultConfig: IsConformantOptions = { | |||
| customMount: mountWithProvider, | |||
| renderOptions: { wrapper: EmptyThemeProvider }, | |||
There was a problem hiding this comment.
| @@ -8,5 +8,7 @@ describe('FormButton', () => { | |||
| constructorName: 'FormButton', | |||
| forwardsRefTo: `Button`, | |||
| targetComponent: Button, | |||
| getTargetElement: (result, attr) => | |||
| attr === 'className' ? result.container.querySelector(`.${formButtonClassName}`) : result.getByRole('button'), | |||
There was a problem hiding this comment.
Since react-conformance no longer uses enzyme's find method to look for an instance of a particular component, we have to look for where attributes are applied in the DOM instead. Open to suggestions from the northstar team if there's a more appropriate way to do this.
| @@ -53,13 +55,13 @@ describe('Radio', () => { | |||
| }); | |||
|
|
|||
| it('respects checked', () => { | |||
| const { getByRole } = render(<Radio checked />); | |||
| const { getByRole } = render(<Radio checked onChange={noOp} />); | |||
There was a problem hiding this comment.
This was just to get rid of a React warning creating noise while testing
ecraig12345
force-pushed
the
conformance-testing-lib
branch
from
21fbebb to
ac936ab
Compare
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AttachmentMinimalPerf.default | 140 | 130 | 1.08:1 |
| AvatarMinimalPerf.default | 179 | 166 | 1.08:1 |
| ChatWithPopoverPerf.default | 354 | 327 | 1.08:1 |
| DividerMinimalPerf.default | 317 | 299 | 1.06:1 |
| FlexMinimalPerf.default | 255 | 244 | 1.05:1 |
| TreeWith60ListItems.default | 156 | 148 | 1.05:1 |
| AttachmentSlotsPerf.default | 892 | 854 | 1.04:1 |
| HeaderSlotsPerf.default | 658 | 633 | 1.04:1 |
| ButtonMinimalPerf.default | 152 | 148 | 1.03:1 |
| CardMinimalPerf.default | 479 | 467 | 1.03:1 |
| GridMinimalPerf.default | 295 | 286 | 1.03:1 |
| HeaderMinimalPerf.default | 310 | 301 | 1.03:1 |
| LabelMinimalPerf.default | 328 | 317 | 1.03:1 |
| ListCommonPerf.default | 532 | 519 | 1.03:1 |
| ListNestedPerf.default | 473 | 459 | 1.03:1 |
| RosterPerf.default | 956 | 930 | 1.03:1 |
| PortalMinimalPerf.default | 154 | 150 | 1.03:1 |
| RadioGroupMinimalPerf.default | 382 | 371 | 1.03:1 |
| StatusMinimalPerf.default | 581 | 565 | 1.03:1 |
| TableMinimalPerf.default | 353 | 343 | 1.03:1 |
| TextAreaMinimalPerf.default | 420 | 406 | 1.03:1 |
| VideoMinimalPerf.default | 541 | 525 | 1.03:1 |
| BoxMinimalPerf.default | 294 | 287 | 1.02:1 |
| ChatMinimalPerf.default | 629 | 618 | 1.02:1 |
| DropdownMinimalPerf.default | 2610 | 2568 | 1.02:1 |
| LoaderMinimalPerf.default | 584 | 571 | 1.02:1 |
| MenuMinimalPerf.default | 734 | 722 | 1.02:1 |
| PopupMinimalPerf.default | 544 | 535 | 1.02:1 |
| RefMinimalPerf.default | 211 | 207 | 1.02:1 |
| TooltipMinimalPerf.default | 885 | 866 | 1.02:1 |
| AnimationMinimalPerf.default | 467 | 463 | 1.01:1 |
| CheckboxMinimalPerf.default | 2259 | 2246 | 1.01:1 |
| DialogMinimalPerf.default | 658 | 653 | 1.01:1 |
| EmbedMinimalPerf.default | 3517 | 3470 | 1.01:1 |
| InputMinimalPerf.default | 1113 | 1106 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1000 | 987 | 1.01:1 |
| LayoutMinimalPerf.default | 312 | 309 | 1.01:1 |
| ListMinimalPerf.default | 436 | 430 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1432 | 1417 | 1.01:1 |
| ReactionMinimalPerf.default | 321 | 317 | 1.01:1 |
| SkeletonMinimalPerf.default | 298 | 294 | 1.01:1 |
| SplitButtonMinimalPerf.default | 3690 | 3648 | 1.01:1 |
| TableManyItemsPerf.default | 1626 | 1603 | 1.01:1 |
| TextMinimalPerf.default | 292 | 289 | 1.01:1 |
| ButtonSlotsPerf.default | 466 | 466 | 1:1 |
| CarouselMinimalPerf.default | 397 | 396 | 1:1 |
| FormMinimalPerf.default | 342 | 342 | 1:1 |
| ProviderMergeThemesPerf.default | 1467 | 1468 | 1:1 |
| ProviderMinimalPerf.default | 974 | 972 | 1:1 |
| CustomToolbarPrototype.default | 3527 | 3513 | 1:1 |
| ToolbarMinimalPerf.default | 793 | 794 | 1:1 |
| TreeMinimalPerf.default | 685 | 686 | 1:1 |
| AccordionMinimalPerf.default | 127 | 128 | 0.99:1 |
| DatepickerMinimalPerf.default | 4746 | 4775 | 0.99:1 |
| ImageMinimalPerf.default | 311 | 315 | 0.99:1 |
| ListWith60ListItems.default | 545 | 549 | 0.99:1 |
| SegmentMinimalPerf.default | 288 | 292 | 0.99:1 |
| SliderMinimalPerf.default | 1426 | 1439 | 0.99:1 |
| ButtonOverridesMissPerf.default | 1427 | 1451 | 0.98:1 |
| ChatDuplicateMessagesPerf.default | 257 | 263 | 0.98:1 |
| AlertMinimalPerf.default | 234 | 242 | 0.97:1 |
| DropdownManyItemsPerf.default | 567 | 582 | 0.97:1 |
| IconMinimalPerf.default | 524 | 556 | 0.94:1 |
ecraig12345
commented
Mar 8, 2022
| @@ -5,7 +5,7 @@ | |||
| "lib": ["DOM", "ES2019"], | |||
| "outDir": "dist", | |||
| "declaration": true, | |||
| "types": ["static-assets", "environment", "jest"], | |||
| "types": ["static-assets", "environment", "jest", "node"], | |||
There was a problem hiding this comment.
I had to add this to make the build pass after enzyme types and their indirect node type leakage were removed
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 924 | 949 | 5000 | |
| Breadcrumb | mount | 2747 | 2704 | 1000 | |
| Checkbox | mount | 1514 | 1517 | 5000 | |
| CheckboxBase | mount | 1310 | 1332 | 5000 | |
| ChoiceGroup | mount | 4841 | 4927 | 5000 | |
| ComboBox | mount | 1015 | 991 | 1000 | |
| CommandBar | mount | 10703 | 10733 | 1000 | |
| ContextualMenu | mount | 12088 | 12718 | 1000 | |
| DefaultButton | mount | 1183 | 1174 | 5000 | |
| DetailsRow | mount | 3892 | 3853 | 5000 | |
| DetailsRowFast | mount | 3891 | 3972 | 5000 | |
| DetailsRowNoStyles | mount | 3709 | 3743 | 5000 | |
| Dialog | mount | 2265 | 2286 | 1000 | |
| DocumentCardTitle | mount | 163 | 174 | 1000 | |
| Dropdown | mount | 3344 | 3418 | 5000 | |
| FocusTrapZone | mount | 1843 | 1875 | 5000 | |
| FocusZone | mount | 1955 | 1842 | 5000 | |
| IconButton | mount | 1842 | 1837 | 5000 | |
| Label | mount | 352 | 368 | 5000 | |
| Layer | mount | 3073 | 3032 | 5000 | |
| Link | mount | 479 | 474 | 5000 | |
| MenuButton | mount | 1553 | 1514 | 5000 | |
| MessageBar | mount | 2228 | 2201 | 5000 | |
| Nav | mount | 3429 | 3368 | 1000 | |
| OverflowSet | mount | 1127 | 1129 | 5000 | |
| Panel | mount | 2183 | 2228 | 1000 | |
| Persona | mount | 883 | 862 | 1000 | |
| Pivot | mount | 1447 | 1468 | 1000 | |
| PrimaryButton | mount | 1350 | 1334 | 5000 | |
| Rating | mount | 7996 | 7802 | 5000 | |
| SearchBox | mount | 1375 | 1339 | 5000 | |
| Shimmer | mount | 2617 | 2567 | 5000 | |
| Slider | mount | 2049 | 2010 | 5000 | |
| SpinButton | mount | 5135 | 5164 | 5000 | |
| Spinner | mount | 440 | 439 | 5000 | |
| SplitButton | mount | 3267 | 3247 | 5000 | |
| Stack | mount | 532 | 557 | 5000 | |
| StackWithIntrinsicChildren | mount | 2320 | 2290 | 5000 | |
| StackWithTextChildren | mount | 5294 | 5399 | 5000 | |
| SwatchColorPicker | mount | 12001 | 11772 | 5000 | |
| TagPicker | mount | 2741 | 2757 | 5000 | |
| TeachingBubble | mount | 98610 | 99836 | 5000 | |
| Text | mount | 455 | 445 | 5000 | |
| TextField | mount | 1419 | 1477 | 5000 | |
| ThemeProvider | mount | 1219 | 1220 | 5000 | |
| ThemeProvider | virtual-rerender | 662 | 651 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1875 | 1888 | 5000 | |
| Toggle | mount | 827 | 797 | 5000 | |
| buttonNative | mount | 143 | 131 | 5000 |
ecraig12345
force-pushed
the
conformance-testing-lib
branch
from
ac936ab to
28c5f0a
Compare
ecraig12345
force-pushed
the
conformance-testing-lib
branch
2 times, most recently
from
c8b0113 to
517e027
Compare
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1044 | 1107 | 5000 | |
| Button | mount | 662 | 687 | 5000 | |
| FluentProvider | mount | 2158 | 2167 | 5000 | |
| FluentProviderWithTheme | mount | 340 | 300 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 293 | 264 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 351 | 381 | 10 | |
| MakeStyles | mount | 1865 | 1870 | 50000 |
This was referenced Mar 15, 2022
ecraig12345
commented
Mar 15, 2022
| if (container) { | ||
| const ReactDOM = await import('react-dom'); | ||
| ReactDOM.unmountComponentAtNode(container); |
There was a problem hiding this comment.
After removing the "nuclear" document.body.innerHTML = '' cleanup in react-conformance, the incomplete cleanup here was causing subsequent tests to fail if the component rendered anything outside of the container node (such as PopoverSurface rendering a Portal).
ecraig12345
force-pushed
the
conformance-testing-lib
branch
from
3371802 to
a5dcccc
Compare
layershifter
approved these changes
Mar 15, 2022
|
Sorry for the brief web-components change...that wasn't supposed to be committed and has been reverted.
Good point, made an issue #22118 |
ecraig12345
force-pushed
the
conformance-testing-lib
branch
2 times, most recently
from
b1e4885 to
9b70f07
Compare
spmonahan
approved these changes
Mar 16, 2022
ecraig12345
force-pushed
the
conformance-testing-lib
branch
from
9b70f07 to
f5307e6
Compare
ecraig12345
force-pushed
the
conformance-testing-lib
branch
from
01aa11a to
50cfdc4
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
45 tasks
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
@fluentui/react-conformanceuses enzyme, which we're trying to move away from. It also exposes enzyme APIs as part of its public API, leading to pollution of the global namespace with node types (see #17101).New Behavior
Port all the tests in react-conformance that used enzyme to use testing-library instead. Note that I did NOT port the northstar-specific
asprop tests (which were moved back into northstar in #21700) to enzyme due to uncertainty about some of the intended behavior.This required some API updates in react-conformance (and corresponding updates in some component tests):
customMount(wrapper of enzyme'smount) =>renderOptions(options for testing-library'srender)wrapperComponents,helperComponents,targetComponent=>getTargetElement(renderResult, attr)Note that northstar's
isConformantstill supportswrapperComponents,helperComponents, andtargetComponentoptions for use with the legacyasprop tests.Related Issue(s)
part of #21663, #21665, #17101