[Resolver] `SideEffectContext` changes, remove `@testing-library` uses #81077

oatkiller · 2020-10-19T22:04:55Z

Summary

getBoundingClientRect is now accessed through SideEffectContext.
writeText from the Clipboard API is now accessed through the
SideEffectContext
No longer using @testing-library/react and @testing-library/react-hooks
No longer using jest.spyOn (mostly) or jest.clearAllMocks

The motivation for this PR:

We already have SideEffectContext, which is meant to be an alternative to using jest.spyOn. This PR uses the SideEffectContext for getBoundingClientRect and navigator.clipboard.writeText.
We have been using enzyme lately. This removes uses of @testing-library/react and @testing-library/react-hooks in favor of enzyme.

For maintainers

This was checked for breaking API changes and was labeled appropriately

oatkiller · 2020-10-20T19:32:55Z

.../resolver/data_access_layer/mocks/no_ancestors_two_children_with_related_events_on_origin.ts

Removed unused param

@bkimmel I think we added this so we could mock the pagination functionality for the /events api. Are we still hoping to implement that?

https://github.com/elastic/kibana/blob/0f70bdd41f610a40eb8f0b53d5e30cb8283f3bc1/x-pack/plugins/security_solution/public/resolver/data_access_layer/mocks/no_ancestors_two_children_with_related_events_and_cursor_on_origin.ts I think that was this guy

oatkiller · 2020-10-20T19:33:34Z

x-pack/plugins/security_solution/public/resolver/store/actions.ts

noticed this used alongside another side effect. I'm scheduled to work on removing this.

oatkiller · 2020-10-20T19:33:47Z

x-pack/plugins/security_solution/public/resolver/test_utilities/extend_jest.ts

This was copy-pasted incorrectly (sorry.)

oatkiller · 2020-10-20T19:34:03Z

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

This function can now take a promise factory as well.

oatkiller · 2020-10-20T19:34:13Z

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

Access the (mock) clipboard text.

oatkiller · 2020-10-20T19:49:51Z

x-pack/plugins/security_solution/public/resolver/view/panels/node_list.tsx

This was imported from the greater security solution app. Also it required a theme provider that we don't use (at least during tests and in the resolver simulator.) The function returned an emdash that had a color taken from the EUI theme. I'll try and add back the color styling using our EUI theme.

In the future we should discuss EUI theme handling.

oatkiller · 2020-10-20T19:53:10Z

x-pack/plugins/security_solution/public/resolver/view/side_effect_context.ts

provide the production implementations of these side effectors.

oatkiller · 2020-10-20T19:53:52Z

x-pack/plugins/security_solution/public/resolver/view/use_camera.test.tsx

No longer use @testing-library. We have moved to enzyme now.

oatkiller · 2020-10-20T19:54:44Z

x-pack/plugins/security_solution/public/resolver/view/use_camera.test.tsx

This functionality is essentially copied from the resolver simulator. You can also find similar code in jest's github issues. It is used to continually assert a value until it is correct. See toYieldEqualTo.

x-pack/plugins/security_solution/public/resolver/view/use_camera.test.tsx

oatkiller · 2020-10-20T20:33:09Z

x-pack/plugins/security_solution/public/resolver/view/panels/use_formatted_date.test.tsx

These look like this now:

myasonik · 2020-10-21T14:38:03Z

x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx

Not sure if this is something you want to resolve in this PR, but all this hover interaction (and whatever is happening on onMouseLeave above) can't really be made accessibly.

I'm not super familiar with the security app so would need some help getting started to offer possible solutions but feel free to reach out with any questions.

This gif shows the feature in use. If the user hovers over the panel that contains the interactive divs, gray backgrounds are added to divs. On mouseenter, a tooltip shows up near the div. The tooltip contains a button with an image in it. You might not know it is a button. If the user hovers with the mouse over the button, a tooltip shows up on top of the original tooltip. This tooltip describes the button. Clicking this will attempt to copy text to the users clipboard.

Actually, I think there's a pretty straight forward solution to all this! 🤞

If StyledCopyableField becomes a button instead of a div, you can trigger the same popover on click (which works for keyboard "clicks" as well) and leave the same hover trigger you have now. This will also give you some basic focus states for free though you may want to style your own if you don't like the defaults.

You can also mimic the mouseenter effect on the whole panel by adding a StyledPanel:focus-within rule in addition to the :hover rule which will trigger when any children of the panel are focused.

Lastly, we can improve the accessibility of the popover by adding the ownFocus (or ownFocus={true}) attribute. (Honestly, this prop can/should be applied to every EuiPopover but isn't right now due to some historical reasons in EUI...)

Stepping out of strictly accessibility for a sec, there seems to be enough room in the screenshot you posted to add the copy button inline to the right of the timestamps. That would remove a lot of the a11y concerns we're dealing with now if it's a viable option...

We discussed doing that, but were looking to maintain parity with our current consuming application (security_solution) in how they have copy/paste functionality implemented. I know there's work planned to make all of this generally more accessible. Thoughts @monina-n @lindseypoli?

🔴 I already put my thoughts in on this on a previous PR, but just to reiterate , I think the problems with putting something in that keyboard users can't use overrides the style/ux consistency concerns.

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx

bkimmel · 2020-10-21T21:23:53Z

Couple things to clear up, then I can thumb

* `getBoundingClientRect` is now access through `SideEffectContext`. * `writeText` from the `Clipboard` API is now accessed through the `SideEffectContext` * No longer using `@testing-library/react` and `@testing-library/react-hooks` * No longer using `jest.spyOn` or `jest.clearAllMocks`

oatkiller · 2020-10-26T20:29:37Z

@elasticmachine merge upstream

kibanamachine · 2020-10-26T22:10:04Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c45484e

Metrics [docs]

async chunks size

id	before	after	diff
`securitySolution`	8.1MB	8.1MB	+1.2KB

History

💚 Build #83393 succeeded 7522b68
💚 Build #82933 succeeded 0f70bdd41f610a40eb8f0b53d5e30cb8283f3bc1
💔 Build #82928 failed d2f775d9f37c49996d4b108434fc243febfa0e83
💚 Build #82914 succeeded 29e8ee637aaed2ac14cc9ad47deed82fd8d20b90
💚 Build #82892 succeeded cd1ec0d968ce820fe326c107b8b3c883354380a1

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

elastic#81077) * `getBoundingClientRect` is now accessed through `SideEffectContext`. * `writeText` from the `Clipboard` API is now accessed through the `SideEffectContext` * No longer using `@testing-library/react` and `@testing-library/react-hooks` * No longer using `jest.spyOn` (mostly) or `jest.clearAllMocks` The motivation for this PR: * We already have `SideEffectContext`, which is meant to be an alternative to using `jest.spyOn`. This PR uses the `SideEffectContext` for `getBoundingClientRect` and `navigator.clipboard.writeText`. * We have been using `enzyme` lately. This removes uses of `@testing-library/react` and `@testing-library/react-hooks` in favor of `enzyme`.

* master: (87 commits) [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81778) [i18n] add get_kibana_translation_paths tests (elastic#81624) [UX] Fix search term reset from url (elastic#81654) [Lens] Improved range formatter (elastic#80132) [Resolver] `SideEffectContext` changes, remove `@testing-library` uses (elastic#81077) [Time to Visualize] Update Library Text with Call to Action (elastic#81667) [docs] Resolving failed Kibana upgrade migrations (elastic#80999) [ftr/menuToggle] provide helper for enhanced menu toggle handling (elastic#81709) [Task Manager] adds basic observability into Task Manager's runtime operations (elastic#77868) Doc changes for stack management and grouped feature privileges (elastic#80486) Added functional test for alerts list filters by status, alert type and action type. Did a code refactoring and splitting for alerts tests. (elastic#81422) [Security Solution][Endpoint][Admin] Malware Protections Notify User Version (elastic#81415) Revert "[Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)" [Maps] Use default format when proxying EMS-files (elastic#79760) [Discover] Deangularize context.html (elastic#81442) Use the displayName property in default editor (elastic#73311) adds bug label to Bug report for Security Solution template (elastic#81643) [ML] Transforms: Remove index field limitation for custom query. (elastic#81467) [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390) [Task Manager] Mark task as failed if maxAttempts has been met. (elastic#80681) ...

#81077) (#81811) * `getBoundingClientRect` is now accessed through `SideEffectContext`. * `writeText` from the `Clipboard` API is now accessed through the `SideEffectContext` * No longer using `@testing-library/react` and `@testing-library/react-hooks` * No longer using `jest.spyOn` (mostly) or `jest.clearAllMocks` The motivation for this PR: * We already have `SideEffectContext`, which is meant to be an alternative to using `jest.spyOn`. This PR uses the `SideEffectContext` for `getBoundingClientRect` and `navigator.clipboard.writeText`. * We have been using `enzyme` lately. This removes uses of `@testing-library/react` and `@testing-library/react-hooks` in favor of `enzyme`.

oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 19, 2020

oatkiller requested a review from michaelolo24 October 19, 2020 22:05

oatkiller force-pushed the remove-react-testing-library branch from ba41788 to cd1ec0d Compare October 20, 2020 19:26

oatkiller commented Oct 20, 2020

View reviewed changes

oatkiller force-pushed the remove-react-testing-library branch 2 times, most recently from 29e8ee6 to d2f775d Compare October 20, 2020 21:24

oatkiller marked this pull request as ready for review October 21, 2020 14:02

oatkiller requested review from a team as code owners October 21, 2020 14:02

oatkiller changed the title ~~[Resolver] SideEffectContext changes~~ [Resolver] SideEffectContext changes, remove @testing-library uses Oct 21, 2020

myasonik reviewed Oct 21, 2020

View reviewed changes

bkimmel reviewed Oct 21, 2020

View reviewed changes

x-pack/plugins/security_solution/public/resolver/test_utilities/simulator/index.tsx Outdated Show resolved Hide resolved

michaelolo24 approved these changes Oct 21, 2020

View reviewed changes

oatkiller added 12 commits October 22, 2020 11:53

wip

3d9211e

stuff

ab34a0c

fixup

b7dac74

comment

a6d6e38

use enzyme for useFormattedDate tests

cdcfcbc

oops

707b0f5

cleanup tests

b670cd3

cleanup

9c19a9d

cleanup

d946f54

remove panel content utilities module

b0c1f22

support multiple dts that share the same dd

7522b68

oatkiller force-pushed the remove-react-testing-library branch from 0f70bdd to 7522b68 Compare October 22, 2020 15:56

bkimmel approved these changes Oct 22, 2020

View reviewed changes

Merge branch 'master' into remove-react-testing-library

c45484e

oatkiller merged commit 161972e into elastic:master Oct 27, 2020

oatkiller deleted the remove-react-testing-library branch October 27, 2020 16:41

oatkiller mentioned this pull request Oct 27, 2020

[7.x] [Resolver] SideEffectContext changes, remove @testing-library uses (#81077) #81811

Merged

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Oct 27, 2020

[Resolver] SideEffectContext changes, remove @testing-library uses #81077

[Resolver] SideEffectContext changes, remove @testing-library uses #81077

Uh oh!

Conversation

oatkiller commented Oct 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oatkiller Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkimmel commented Oct 21, 2020

Uh oh!

oatkiller commented Oct 26, 2020

Uh oh!

kibanamachine commented Oct 26, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[Resolver] `SideEffectContext` changes, remove `@testing-library` uses #81077

[Resolver] `SideEffectContext` changes, remove `@testing-library` uses #81077

oatkiller commented Oct 19, 2020 •

edited

Loading

oatkiller Oct 21, 2020 •

edited

Loading