FocusTrapZone: port most tests to cypress #21741
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 9ff5d82:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: d4c33467e224fee30434372238eedf5e7f7d3866 (build) |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 765 | 758 | 5000 | |
| BaseButton | mount | 812 | 816 | 5000 | |
| Breadcrumb | mount | 2306 | 2323 | 1000 | |
| ButtonNext | mount | 422 | 399 | 5000 | |
| Checkbox | mount | 1348 | 1347 | 5000 | |
| CheckboxBase | mount | 1126 | 1123 | 5000 | |
| ChoiceGroup | mount | 4156 | 4117 | 5000 | |
| ComboBox | mount | 863 | 853 | 1000 | |
| CommandBar | mount | 8785 | 8918 | 1000 | |
| ContextualMenu | mount | 7315 | 7411 | 1000 | |
| DefaultButton | mount | 977 | 985 | 5000 | |
| DetailsRow | mount | 3236 | 3227 | 5000 | |
| DetailsRowFast | mount | 3299 | 3251 | 5000 | |
| DetailsRowNoStyles | mount | 3072 | 3069 | 5000 | |
| Dialog | mount | 1925 | 1949 | 1000 | |
| DocumentCardTitle | mount | 149 | 161 | 1000 | |
| Dropdown | mount | 2762 | 2769 | 5000 | |
| FluentProviderNext | mount | 1731 | 1764 | 5000 | |
| FluentProviderWithTheme | mount | 155 | 120 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 106 | 89 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 185 | 166 | 10 | |
| FocusTrapZone | mount | 1638 | 1619 | 5000 | |
| FocusZone | mount | 1579 | 1587 | 5000 | |
| IconButton | mount | 1514 | 1516 | 5000 | |
| Label | mount | 325 | 328 | 5000 | |
| Layer | mount | 2599 | 2590 | 5000 | |
| Link | mount | 423 | 428 | 5000 | |
| MakeStyles | mount | 1455 | 1476 | 50000 | |
| MenuButton | mount | 1294 | 1276 | 5000 | |
| MessageBar | mount | 1745 | 1764 | 5000 | |
| Nav | mount | 2816 | 2831 | 1000 | |
| OverflowSet | mount | 979 | 972 | 5000 | |
| Panel | mount | 1885 | 1848 | 1000 | |
| Persona | mount | 740 | 745 | 1000 | |
| Pivot | mount | 1261 | 1264 | 1000 | |
| PrimaryButton | mount | 1125 | 1126 | 5000 | |
| Rating | mount | 6597 | 6670 | 5000 | |
| SearchBox | mount | 1181 | 1151 | 5000 | |
| Shimmer | mount | 2247 | 2213 | 5000 | |
| Slider | mount | 1727 | 1724 | 5000 | |
| SpinButton | mount | 4317 | 4294 | 5000 | |
| Spinner | mount | 394 | 387 | 5000 | |
| SplitButton | mount | 2705 | 2739 | 5000 | |
| Stack | mount | 480 | 490 | 5000 | |
| StackWithIntrinsicChildren | mount | 2015 | 2052 | 5000 | |
| StackWithTextChildren | mount | 4561 | 4581 | 5000 | |
| SwatchColorPicker | mount | 9939 | 9997 | 5000 | |
| TagPicker | mount | 2238 | 2294 | 5000 | |
| TeachingBubble | mount | 11393 | 11406 | 5000 | |
| Text | mount | 396 | 399 | 5000 | |
| TextField | mount | 1222 | 1226 | 5000 | |
| ThemeProvider | mount | 1024 | 1048 | 5000 | |
| ThemeProvider | virtual-rerender | 552 | 542 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1645 | 1628 | 5000 | |
| Toggle | mount | 726 | 741 | 5000 | |
| buttonNative | mount | 140 | 134 | 5000 |
| const [props, setProps] = React.useState<IFocusTrapZoneProps | undefined>(); | ||
|
|
||
| React.useEffect(() => { | ||
| (window as FTZTestWindow).setProps = setProps; |
There was a problem hiding this comment.
I honestly don't think this is bad, I guess a more 'proper' way would be to use storybook to manage the globals
There was a problem hiding this comment.
Do you know how to manage globals with storybook? I tried setting up the story to accept parameters, which can be set in the query, but IIRC the issue with that was cy.loadStory didn't have an API to set query params.
There was a problem hiding this comment.
@Hotell and wrote some code to handle globals for the theme picker in the v9 storybook docs, you can take a look in react-storybook-addon, but you don't need to do query params to manage storybook globals.
That being said, SB globals can be ugly to manage, that's why I said I honestly don't think it's that bad
There was a problem hiding this comment.
FYI: All my local --mode open runs with the previous approach (setting a global but not cleaning it up) worked fine, but a lot of the tests failed in headless mode in the PR due to a timing issue. I guess because runs in a browser UI are slower, each run always got the current test's version of setProps, but in headless runs, a lot of times the test would start running and call the previous test's setProps before the story had time to finish rendering and set a new one.
Fix is:
- define the global inside a one-time effect WITH cleanup that deletes the global
- in the test, use
window.shouldnotwindow.thento wrap thesetPropscall, so that it retries if the global isn't ready yet
I wrote a helper that handles this and updated all the tests to use it. If we end up porting more tests that need global functions, the helper could be made a bit more generic and moved to a shared location.
packages/react-examples/src/react/FocusTrapZone/e2e/FocusTrapZone.e2e.ts
Outdated
Show resolved
Hide resolved
packages/react-examples/src/react/FocusTrapZone/e2e/FocusTrapZone.e2e.ts
Outdated
Show resolved
Hide resolved
packages/react-examples/src/react/FocusTrapZone/e2e/FocusTrapZone.e2e.ts
Outdated
Show resolved
Hide resolved
packages/react-examples/src/react/FocusTrapZone/e2e/FocusTrapZone.e2e.ts
Outdated
Show resolved
Hide resolved
packages/react-examples/src/react/FocusTrapZone/e2e/FocusTrapZone.e2e.ts
Outdated
Show resolved
Hide resolved
| setProps({ disableFirstFocus: true }); | ||
|
|
||
| // For some reason this doesn't work: | ||
| // cy.focused().should('match', 'body'); |
There was a problem hiding this comment.
what's the actual focused item in that case ?
There was a problem hiding this comment.
The body. document.activeElement is body if you inspect it, but that assertion doesn't match for some reason.
There was a problem hiding this comment.
yeah but if you run the test in the cypres ui, it should tell you what the result of cy.focused() is, just curious, maybe we're matching the wrong thing.... or even what we're expecting to be focused isn't that
There was a problem hiding this comment.
IIRC it says focused() is body but it doesn't match that selector. Maybe I'm misunderstanding what match does but it seemed like it should match any CSS selector.
There was a problem hiding this comment.
perhaps this ?
cy.get('body').should('have.focus')
There was a problem hiding this comment.
There was a problem hiding this comment.
nope (╯°□°）╯︵ ┻━┻
(to be clear that's directed at cypress not you)
There was a problem hiding this comment.
Looking at the error messages for various attempted assertions and thinking about it some more, I think the issue is that it's checking focus with :focus rather than looking at document.activeElement. Apparently even if body is document.activeElement it doesn't get :focus. https://codepen.io/ecraig12345/pen/jOavBRw
Since cy.focused() returns a jquery object with the matches, cy.focused().should('have.length', 0) works.
There was a problem hiding this comment.
Oh a bit better: cy.focused().should('not.exist')
packages/react-examples/src/react/FocusTrapZone/e2e/FocusTrapZone.e2e.ts
Outdated
Show resolved
Hide resolved
packages/react-examples/src/react/FocusTrapZone/e2e/FocusTrapZone.e2e.ts
Outdated
Show resolved
Hide resolved
| @@ -105,8 +105,12 @@ jobs: | |||
|
|
|||
| # only run e2e tests when the appropriate storybook is published by scoping to relevant packages | |||
| - script: | | |||
| yarn e2e $(sinceArg) --scope @fluentui/react-components --scope @fluentui/react | |||
There was a problem hiding this comment.
I think having two packages as scopes caused this PR's first build to try and run tests for deps of react-components even though react-components hadn't been built or deployed. Hopefully splitting into two steps will fix it. (Problem is it won't be possible to tell in this PR, because changing a pipeline file makes the PR build all packages.)
ecraig12345
force-pushed
the
cypress-ftz
branch
from
ef03530
to
e2f5677
Compare
packages/react-examples/src/react/FocusTrapZone/e2e/FocusTrapZone.e2e.ts
Show resolved
Hide resolved
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| ButtonMinimalPerf.default | 168 | 125 | 1.34:1 |
| AttachmentMinimalPerf.default | 160 | 133 | 1.2:1 |
| BoxMinimalPerf.default | 306 | 287 | 1.07:1 |
| DropdownMinimalPerf.default | 2609 | 2445 | 1.07:1 |
| AccordionMinimalPerf.default | 148 | 139 | 1.06:1 |
| AlertMinimalPerf.default | 226 | 214 | 1.06:1 |
| CardMinimalPerf.default | 485 | 461 | 1.05:1 |
| DatepickerMinimalPerf.default | 4833 | 4590 | 1.05:1 |
| PortalMinimalPerf.default | 161 | 153 | 1.05:1 |
| IconMinimalPerf.default | 538 | 511 | 1.05:1 |
| TreeWith60ListItems.default | 162 | 155 | 1.05:1 |
| ButtonOverridesMissPerf.default | 1395 | 1336 | 1.04:1 |
| DividerMinimalPerf.default | 310 | 299 | 1.04:1 |
| FormMinimalPerf.default | 345 | 333 | 1.04:1 |
| ImageMinimalPerf.default | 325 | 312 | 1.04:1 |
| ListWith60ListItems.default | 563 | 542 | 1.04:1 |
| RefMinimalPerf.default | 210 | 202 | 1.04:1 |
| DropdownManyItemsPerf.default | 601 | 581 | 1.03:1 |
| FlexMinimalPerf.default | 257 | 249 | 1.03:1 |
| ListNestedPerf.default | 479 | 464 | 1.03:1 |
| LoaderMinimalPerf.default | 597 | 581 | 1.03:1 |
| ReactionMinimalPerf.default | 332 | 321 | 1.03:1 |
| ToolbarMinimalPerf.default | 803 | 783 | 1.03:1 |
| AttachmentSlotsPerf.default | 911 | 896 | 1.02:1 |
| EmbedMinimalPerf.default | 3476 | 3412 | 1.02:1 |
| HeaderMinimalPerf.default | 314 | 309 | 1.02:1 |
| InputMinimalPerf.default | 1119 | 1096 | 1.02:1 |
| ProviderMinimalPerf.default | 977 | 960 | 1.02:1 |
| TextMinimalPerf.default | 306 | 299 | 1.02:1 |
| CustomToolbarPrototype.default | 3569 | 3491 | 1.02:1 |
| VideoMinimalPerf.default | 541 | 528 | 1.02:1 |
| CarouselMinimalPerf.default | 406 | 403 | 1.01:1 |
| ListMinimalPerf.default | 447 | 441 | 1.01:1 |
| MenuMinimalPerf.default | 722 | 712 | 1.01:1 |
| SplitButtonMinimalPerf.default | 3654 | 3626 | 1.01:1 |
| TableManyItemsPerf.default | 1633 | 1614 | 1.01:1 |
| TableMinimalPerf.default | 352 | 350 | 1.01:1 |
| TextAreaMinimalPerf.default | 423 | 420 | 1.01:1 |
| TooltipMinimalPerf.default | 886 | 875 | 1.01:1 |
| ChatDuplicateMessagesPerf.default | 256 | 255 | 1:1 |
| LayoutMinimalPerf.default | 306 | 306 | 1:1 |
| MenuButtonMinimalPerf.default | 1400 | 1398 | 1:1 |
| SegmentMinimalPerf.default | 301 | 301 | 1:1 |
| SliderMinimalPerf.default | 1424 | 1422 | 1:1 |
| CheckboxMinimalPerf.default | 2224 | 2246 | 0.99:1 |
| GridMinimalPerf.default | 291 | 293 | 0.99:1 |
| LabelMinimalPerf.default | 331 | 335 | 0.99:1 |
| ListCommonPerf.default | 536 | 544 | 0.99:1 |
| PopupMinimalPerf.default | 517 | 524 | 0.99:1 |
| ProviderMergeThemesPerf.default | 1484 | 1505 | 0.99:1 |
| TreeMinimalPerf.default | 677 | 685 | 0.99:1 |
| ItemLayoutMinimalPerf.default | 992 | 1017 | 0.98:1 |
| RadioGroupMinimalPerf.default | 378 | 384 | 0.98:1 |
| SkeletonMinimalPerf.default | 305 | 311 | 0.98:1 |
| DialogMinimalPerf.default | 623 | 644 | 0.97:1 |
| RosterPerf.default | 999 | 1033 | 0.97:1 |
| StatusMinimalPerf.default | 574 | 592 | 0.97:1 |
| ButtonSlotsPerf.default | 428 | 447 | 0.96:1 |
| ChatMinimalPerf.default | 609 | 633 | 0.96:1 |
| ChatWithPopoverPerf.default | 315 | 335 | 0.94:1 |
| HeaderSlotsPerf.default | 614 | 651 | 0.94:1 |
| AnimationMinimalPerf.default | 411 | 458 | 0.9:1 |
| AvatarMinimalPerf.default | 152 | 170 | 0.89:1 |
| cy.focused().should('have.text', 'last'); | ||
| }); | ||
|
|
||
| // TODO: investigate why this intermittently fails and re-enable. |
There was a problem hiding this comment.
After more investigation, I think the test is flaky because of bugs or timing issues in the function component conversion. That will be fixed and the test enabled in an upcoming PR (the change is ready to go, but it will be hard to review until this PR goes in).
|
|
||
| // wait for first focus to finish to avoid timing issue | ||
| cy.focused().should('have.text', 'first'); | ||
|
|
There was a problem hiding this comment.
suggestion: You could maybe click mid or last before clicking outside so that you have a change from mid or last to first instead of having first twice in a row.
There was a problem hiding this comment.
For some reason that makes the rest of the test stop working, which may be due to function component conversion bugs. I'll leave it as-is for now but try that suggestion in the version with bug fixes.
|
|
||
| /** | ||
| * Tab and shift-tab wrap at extreme ends of the FTZ: | ||
| * | ||
| * can tab across FocusZones with different button structures | ||
| * can between multiple FocusZones with different button structures |
There was a problem hiding this comment.
nit:
| * can between multiple FocusZones with different button structures | |
| * can tab between multiple FocusZones with different button structures |
| // shift+tab focuses the first bumper (since the buttons inside aren't keyboard-focusable) | ||
| // => sends focus to first element | ||
| cy.realPress(['Shift', 'Tab']); | ||
| cy.focused().should('have.text', 'first'); |
There was a problem hiding this comment.
suggestion: Maybe it'd be more informative to go to last instead of mid, because doing a Shift + Tab on mid with focusable buttons would also lead to first, whereas it would go to mid when doing a Shift + Tab on last with focusable buttons.
Current Behavior
FocusTrapZone's tests do so much mocking that it's not clear whether they're actually validating the things they claim to be validating. Also, they use enzyme (which we're trying to get rid of) and some of the mocking stops working when upgrading to jest 27.
New Behavior
Move most of the tests to use Cypress. In all cases I'm testing the same things as in the removed jest tests with the same descriptions, but I changed the element IDs to be more semantic to try and make it easier to follow what the tests are doing, and removed some
data-*props that don't seem to be needed in the actual browser. The layouts are also basically the same as they were with the manually-setclientRectmocks, just sometimes achieved with different styling (and larger button sizes to accommodate the text).One pattern that I'm currently using in this PR but don't really like is creating global functions on
windowfor a few cases. If anyone has better suggestions for how to approach any of these, I'd appreciate it.@cypress/reactwhich allows direct rendering of React components within the Cypress test, but that would probably be done in a separate PRfocus()methodFocusTrapZone.focusStackis correct (I'm not sure if there's an alternative way to test this based on the rendered DOM)Another thing to consider for the future is whether we want to add
@testing-library/cypressto facilitate testing that's more like actual user interaction patterns, similar to@testing-library/react. That would probably be done in a separate PR.Related Issue(s)
Part of #21663