-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[Lens] Prepare Lens for jest-environment-jsdom migration #95327
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.test.tsx
Show resolved
Hide resolved
Not sure why the type checker told me to remove the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM except for one typing nit
const component = mountWithIntl(<LayerPanels {...defaultProps} />); | ||
const component = mountWithIntl( | ||
<LayerPanels {...defaultProps} />, | ||
// @ts-expect-error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why expecting an error here? It seems like we can just extend the options type in packages/kbn-test/src/jest/utils/enzyme_helpers.tsx
to take attachTo
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! If we can get rid of //@ts-expect-error with Joe's suggestion, let's do it, but otherwise approved!
@elasticmachine merge upstream |
The changes in the helper triggered another review from |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
* 🐛 Fix activeElement issue with Jest * 🏷️ Fix type issue * 👌 Removed expect-errors directives Co-authored-by: Kibana Machine <[email protected]>
…5530) * 🐛 Fix activeElement issue with Jest * 🏷️ Fix type issue * 👌 Removed expect-errors directives Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Marco Liberati <[email protected]>
Summary
Fixes #95202
The message reported on the issue is really rewriting the original messages, which are referring to
document.activeElement
.To reproduce the issue:
jest-environment-jsdom
environment in thepackages/kbn-test/jest-preset.js
To see the original issue bubble up add
skip
to the following tests but one (others will interfere):https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx#L616
https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.test.tsx#L107
https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.test.tsx#L122
https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.test.tsx#L143
https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/config_panel.test.tsx#L164
And something like this will show up:
These specific tests were using the
document.activeElement
which in jsdom has a specific behaviour, as reported also on this Enzyme issue.This PR implements the solution proposed here. Using directly
document.body
as suggested on the jsdom issue prints some very verbose performance warnings, so the container solution is the safest option.Checklist