[EuiToolTip] Convert tests to RTL and export tooltip RTL helpers #6106

cee-chen · 2022-08-03T23:47:43Z

Summary

This PR:

Improves our EuiToolTip tests to actually snapshot the rendered tooltip content
Converts our tooltip tests to RTL while still maintaining a single Enzyme test for legacy/portal regression testing
Exports new RTL waitForEuiToolTipVisible and waitForEuiToolTipHidden test helpers
Closes Testing popover visibility and contents with react testing library. #6065

Checklist

Added or updated jest and cypress tests
A changelog entry exists and is marked appropriately

cee-chen · 2022-08-03T23:50:41Z

Quick note: CI on this PR will fail until #6105 is merged (on the Enzyme regression test)

cee-chen · 2022-08-03T23:57:10Z

src/components/tool_tip/tool_tip_popover.tsx

@@ -40,6 +40,7 @@ export class EuiToolTipPopover extends Component<Props> {
  componentDidMount() {
    document.body.classList.add('euiBody-hasPortalContent');
    window.addEventListener('resize', this.updateDimensions);
+    this.updateDimensions();


I'm not 100% sure this is the best way to solve #6065 but it definitely is the quickest!

The issue here is that EuiToolTipPopover.updateDimensions/EuiToolTip.positionToolTip simply isn't getting called in tests.

I believe position styles are initially set/work in browser environments because of the EuiResizeObserver here:

eui/src/components/tool_tip/tool_tip.tsx

Lines 329 to 331 in f73360c

<EuiResizeObserver onResize={this.positionToolTip}>

{(resizeRef) => <div ref={resizeRef}>{content}</div>}

</EuiResizeObserver>

But because this is jsdom, and jsdom doesn't support MutationObservers on the version we're on (v11), and presumably also does not support ResizeObservers, positionToolTip never gets called and the visibility styles never get set.

Calling updateDimensions on mount should fix the issue and have minimal perf impact, but I could be wrong on that - 2nd opinions appreciated (cc @thompsongl).

If we absolutely had to we could attempt to either 1. polyfill window.ResizeObserver or 2. mock EuiResizeObserver, but one downside to those approaches is that consuming applications will have to import those mocks as opposed to simply getting a fix (this approach).

We could use the IS_JEST_ENVIRONMENT util and only run this during tests. Just confirmed locally that it'd work.

if (IS_JEST_ENVIRONMENT) this.updateDimensions();

That's definitely an approach we can use - but I guess like, in a vacuum, doesn't it feel like positionTooltip should be called on tooltip mount in any case? If you told me that it gets passed to a window resize listener and a resize observer, I wouldn't assume positionTooltip gets called except on resize.

(Separately I also wonder if we should examine our EuiResizeObserver to make sure it isn't incorrectly calling the onResize callback on mount when it shouldn't be)

doesn't it feel like positionTooltip should be called on tooltip mount in any case? If you told me that it gets passed to a window resize listened and a resize observer, I wouldn't assume positionTooltip gets called except on resize. I do wonder if we should examine our EuiResizeObserver to make sure it isn't incorrectly calling the onResize callback on mount when it shouldn't be.

So ResizeObserver does call the callback immediately upon instantiation, per spec: WICG/resize-observer#38 (comment)
https://codepen.io/thompsongl/pen/MWVVZjZ?editors=1111
At one point before browser support was there, EUI had it's own polyfill to mimic the callback-on-init behavior:

eui/src/components/observer/resize_observer/resize_observer.js

Line 35 in 895a07c

requestAnimationFrame(this.onResize); // Mimic ResizeObserver behavior of triggering a resize event on init

Not that it's intuitive at all, but calling the callback on mount explicitly would mean that the callback is called twice on mount in normal browsers situations.

Ha 🙃 That spec is super unintuitive. Thanks for the source!

To be honest, I'm not a huge fan of our various usages of IS_JEST_ENVIRONMENT and I'd rather move away from them than add another instance. I think jest.mock()s are a safer way of handling test-only logic without adding unreachable branches to source/production code.

If we don't want the extra call on mount, I'd personally rather add a testenv mock to EuiResizeObserver that simply calls on the onResize callback on mount. Thoughts?

add a testenv mock to EuiResizeObserver that simply calls on the onResize callback on mount

Probably worth looking into. I wonder how/if that would impact consumers who are use the polyfill. Kibana doesn't use it as far as I know.

Consumers who use the polyfill wouldn't be importing/automatically using our testenv mock, so it shouldn't affect them 🤷

Alrighty, added a testenv mock: acb564c

kibanamachine · 2022-08-04T00:15:59Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6106/

+ prefer runAllTimers over static timeout + restore real timers after

- keep single Enzyme test for legacy/portal regression catches

kibanamachine · 2022-08-04T15:51:52Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6106/

kibanamachine · 2022-08-04T22:37:34Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6106/

src/components/observer/resize_observer/resize_observer.testenv.tsx

thompsongl · 2022-08-08T15:52:57Z

src/components/observer/resize_observer/resize_observer.testenv.tsx

+// Re-export other exports
+export { hasResizeObserver, useResizeObserver } from './resize_observer';


This won't work because resize_observer.testenv.tsx completely replaces resize_observer.tsx in the testenv build.

For hasResizeObserver you could probably copy-paste. For useResizeObserver I'm not exactly sure. Maybe it's similar to EuiResizeObserver and it just does an on mount useEffect?

ahhh gotcha gotcha, sorry, I totally brainfarted on how our test-env export works. looking into this now

ok this time for real 5f15f23

kibanamachine · 2022-08-08T16:05:12Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6106/

kibanamachine · 2022-08-08T16:59:40Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6106/

thompsongl

Code here looks good but when I run tests in Kibana I get a bunch of errors like this:

Error: Uncaught [TypeError: Cannot read properties of undefined (reading '_context')]

I think it's unrelated and my local env is just borked but want to confirm that, say, src/plugins/discover Jest tests all pass for you.

cee-chen · 2022-08-08T21:12:39Z

No failures with the following commands:

yarn kbn reset // since my local Kibana has upgrade work
yarn kbn bootstrap --no-validate
yarn test:jest src/plugins/discover

cee-chen requested a review from chandlerprall August 3, 2022 23:47

cee-chen commented Aug 3, 2022

View reviewed changes

cee-chen added 4 commits August 4, 2022 08:24

Tweak test to show actual rendered tooltip HTML

706a39b

+ prefer runAllTimers over static timeout + restore real timers after

Convert tooltip tests to RTL and add RTL test helpers

85bb6fc

- keep single Enzyme test for legacy/portal regression catches

Fix tooltips not registering as visible in tests

e54a046

Changelog

211dab8

cee-chen force-pushed the tooltip-testing branch from 1f853ff to 211dab8 Compare August 4, 2022 15:24

cee-chen marked this pull request as ready for review August 4, 2022 15:24

[PR feedback] Prefer EuiResizeObserver test mock over affecting prod

acb564c

cee-chen requested a review from thompsongl August 4, 2022 22:14

cee-chen mentioned this pull request Aug 4, 2022

Testing popover visibility and contents with react testing library. #6065

Closed

thompsongl reviewed Aug 8, 2022

View reviewed changes

src/components/observer/resize_observer/resize_observer.testenv.tsx Show resolved Hide resolved

thompsongl reviewed Aug 8, 2022

View reviewed changes

[PR feedback] fix mock testenv exports

5f15f23

cee-chen force-pushed the tooltip-testing branch from fa75a72 to 5f15f23 Compare August 8, 2022 16:33

cee-chen requested a review from thompsongl August 8, 2022 19:38

thompsongl reviewed Aug 8, 2022

View reviewed changes

thompsongl approved these changes Aug 9, 2022

View reviewed changes

cee-chen merged commit b060306 into elastic:main Aug 9, 2022

cee-chen deleted the tooltip-testing branch August 9, 2022 15:13

thompsongl mentioned this pull request Aug 15, 2022

Upgrade EUI to v62.2.3 elastic/kibana#138837

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiToolTip] Convert tests to RTL and export tooltip RTL helpers #6106

[EuiToolTip] Convert tests to RTL and export tooltip RTL helpers #6106

cee-chen commented Aug 3, 2022 •

edited

Loading

cee-chen commented Aug 3, 2022

cee-chen Aug 3, 2022 •

edited

Loading

thompsongl Aug 4, 2022 •

edited

Loading

cee-chen Aug 4, 2022 •

edited

Loading

thompsongl Aug 4, 2022 •

edited

Loading

thompsongl Aug 4, 2022

cee-chen Aug 4, 2022

thompsongl Aug 4, 2022

cee-chen Aug 4, 2022

thompsongl Aug 4, 2022

cee-chen Aug 4, 2022

kibanamachine commented Aug 4, 2022

kibanamachine commented Aug 4, 2022

kibanamachine commented Aug 4, 2022

thompsongl Aug 8, 2022

cee-chen Aug 8, 2022

cee-chen Aug 8, 2022

kibanamachine commented Aug 8, 2022

kibanamachine commented Aug 8, 2022

thompsongl left a comment

cee-chen commented Aug 8, 2022

	<EuiResizeObserver onResize={this.positionToolTip}>
	{(resizeRef) => <div ref={resizeRef}>{content}</div>}
	</EuiResizeObserver>

		// Re-export other exports
		export { hasResizeObserver, useResizeObserver } from './resize_observer';

[EuiToolTip] Convert tests to RTL and export tooltip RTL helpers #6106

[EuiToolTip] Convert tests to RTL and export tooltip RTL helpers #6106

Conversation

cee-chen commented Aug 3, 2022 • edited Loading

Summary

Checklist

cee-chen commented Aug 3, 2022

cee-chen Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

thompsongl Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

cee-chen Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

thompsongl Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 4, 2022

kibanamachine commented Aug 4, 2022

kibanamachine commented Aug 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 8, 2022

kibanamachine commented Aug 8, 2022

thompsongl left a comment

Choose a reason for hiding this comment

cee-chen commented Aug 8, 2022

cee-chen commented Aug 3, 2022 •

edited

Loading

cee-chen Aug 3, 2022 •

edited

Loading

thompsongl Aug 4, 2022 •

edited

Loading

cee-chen Aug 4, 2022 •

edited

Loading

thompsongl Aug 4, 2022 •

edited

Loading