-
Notifications
You must be signed in to change notification settings - Fork 842
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
Install React Testing Library and export several RTL test helpers #6091
Conversation
@@ -0,0 +1,105 @@ | |||
/* |
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.
RTL tests should just be .test.tsx
but since I'm not migrating 100% of EuiPopover's tests currently I just did rtl.test.tsx
. End goal everything should be just .test.tsx
once we're out of Enzyme.
@chandlerprall Does this technically close #5597? |
The good news is that RTL works great with base EuiPopover The bad news is that I then have no idea why Kibana is throwing so many pointer events errors :/ |
// NOTE: Using baseElement instead of container is required for components that use portals | ||
expect(baseElement).toMatchSnapshot(); |
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.
Just want everyone to share the pain I had of figuring this out in a 3 year old github thread when google wasn't returning any relevant results 💀
Preview documentation changes for this PR: https://eui.elastic.co/pr_6091/ |
it does! 🎉 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6091/ |
By commenting out blocks, I found the mocks fail when rtl_custom_render.ts imports from ../components. Fix scripts/jest/setup/config.json "setupFiles": [
"<rootDir>/scripts/jest/setup/enzyme.js",
"<rootDir>/scripts/jest/setup/throw_on_console_error.js",
+ "<rootDir>/scripts/jest/setup/mocks.js"
],
"setupFilesAfterEnv": [
- "<rootDir>/scripts/jest/setup/mocks.js",
"<rootDir>/scripts/jest/setup/polyfills.js",
"<rootDir>/scripts/jest/setup/unmount_enzyme.js"
], Why Jest loads the snapshot serializers before setupFilesAfterEnv, and scripts/jest/setup/emotion.js imports from src/test kicking off the import chain: src/test -> rtl_custom_render -> ../components thus pre-requiring the entirety of EUI's components before the mock code loads. During execution, the pre-warmed file cache is apparently preferred to going to the defined mocks. We should probably also move unmount_enzyme.js to |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6091/ |
Whoa, thank you for the debugging!! I thought I tried that but Jest took so long when I moved that file to Out of curiosity, would it also work to import EDIT: Looks like it doesn't, womp womp. |
Unfortunately if I move
Moving |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6091/ |
…test/` - Since RTL is a devDependency not a peerDependency, we can't assume consumers will have the library installed, and need to make reaching for it optional
Preview documentation changes for this PR: https://eui.elastic.co/pr_6091/ |
1 similar comment
Preview documentation changes for this PR: https://eui.elastic.co/pr_6091/ |
+ helps with quick type checking as well
a0764e8
to
d22eeeb
Compare
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.
I'm definitely not in love with the manual .d.ts
files workaround, but I guess it works 🙈 I think this is ready for Kibana prime time @thompsongl @chandlerprall!
Preview documentation changes for this PR: https://eui.elastic.co/pr_6091/ |
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! Thanks, as always, for all the tests
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.
🦑
) * Add RTL dependencies (at the same versions that Kibana uses) * Add basic EuiPopover RTL tests * consolidate react types yarn.lock entries * Add custom RTL queries and render/screen * consolidate more yarn.lock entries * Fix custom RTL render ignoring testenv mocks * Add RTL helpers for waiting for an EuiPopover to open/close * [Misc] Tweak typing by using correct get (vs query/find) API * Move RTL helpers to `src/test/rtl` instead of being exported by `src/test/` - Since RTL is a devDependency not a peerDependency, we can't assume consumers will have the library installed, and need to make reaching for it optional * Add Typescript definitions for helpers * Add unit tests for new generic test helpers + helps with quick type checking as well * Changelog/consuming documentation Co-authored-by: Greg Thompson <thompson.glowe@gmail.com> Co-authored-by: Chandler Prall <chandler.prall@gmail.com>
Summary
Meta issue: #5596
Our first baby steps into the eventual conversion of Enzyme tests into RTL. This PR was born more out of the need to figure out why the heck Kibana's RTL tests are flipping their lids over some of our recent changes to EuiPopover/EuiPortal.
This PR:
ByDataTestSubj
utilities for targeting EUI'sdata-test-subj
attributes. These can be accessed by importing custom RTLrender
andscreen
utils from@elastic/eui/lib/test/rtl
.waitForEuiPopoverOpen
andwaitForEuiPopoverClose
Checklist
@testing-library/react
installed@testing-library/react
installed