Tooltip: improve tests #57345

ciampo · 2023-12-22T15:31:13Z

Related to #57206 and #57204

What?

Improve Tooltip tests:

make less flaky / hacky
test for tooltip to hide after hover / focus / click events as expected
tidy up code

Why?

The conversation in #57206 brought up some added nuance to Tooltip's behavior that we should add to our unit tests

How?

Switched to @ariakit/test
Went test by test, improving assertions, adding comments, and generally proposing a better order/organisation

Testing Instructions

Note: I recommend reviewing code changes commit by commit

Tooltip: No runtime changes, make sure unit tests pass
Modal: added an extra check on detecting the Escape key => test on Storybook and in the editor that the Modal component still closes as expected when pressing the Escape key

ciampo · 2023-12-22T15:33:25Z

packages/components/src/modal/index.tsx

@@ -209,7 +209,7 @@ function UnforwardedModal(

 		if (
 			shouldCloseOnEsc &&
-			event.code === 'Escape' &&
+			( event.code === 'Escape' || event.key === 'Escape' ) &&


@ariakit/test keyboard events use event.key, so I had to add this check in order for the Modal component to react to those simulated user events

We can implement code in @ariakit/test. I'm just not sure how to transform keys into code, and I'd prefer to sidestep the need to maintain a full map of values.

I guess we can keep this change to the Modal component for now (cc @mirka )

ciampo · 2023-12-22T15:35:57Z

packages/components/src/tooltip/test/index.tsx


-		render(
-			<>
+		it( 'should not show tooltip if the mouse leaves the tooltip anchor before set delay', async () => {


@diegohaz This test keeps failing when the whole suite runs, but passes when run in isolation.

It looks like there is still some "leak" across tests even after using @ariakit/test, but I can't understand what it is — do you have any ideas?

Tooltips appear instantly if another tooltip has just been hidden. This is controlled by a global state, which is probably shared among multiple tests within the same file. These tests don't exactly run in total isolation.

If this is the issue, you might need to introduce a delay at the start to ensure tooltips from other tests aren't interfering with this one.

Tooltips appear instantly if another tooltip has just been hidden. This is controlled by a global state, which is probably shared among multiple tests within the same file. These tests don't exactly run in total isolation.

That seems to be the issue, since the test passes if I set the skipTimeout: 0 (although I'd prefer to leave the skipTimeout delay around if possible).

If this is the issue, you might need to introduce a delay at the start to ensure tooltips from other tests aren't interfering with this one.

I tried adding a delay specifically for that test, but I keep getting failures. The only way to reliably have all tests pass seems to be to add

afterEach( async () => { await sleep( 300 ); } );

although I'd prefer to avoid slowing down the whole suite if possible. Do you have any suggestions?

Another option is to run those tests fully in parallel. I know Vitest offers this feature, but I'm not sure about Jest.

For now, I'll have to resort to waiting those extra 300ms at the end of each test to make sure that the "skip timeout" behaviour is not triggered.

FYI, this await sleep(300) might not be required anymore with @ariakit/react@0.4.2.

PR to remove it: #60897

packages/components/src/tooltip/test/index.tsx

github-actions · 2023-12-22T15:49:41Z

Flaky tests detected in ac4d28f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7301468581
📝 Reported issues:

[Flaky Test] should be disabled when post is being saved #35641 in /test/e2e/specs/editor/various/publish-button.spec.js

packages/components/src/tooltip/test/index.tsx

ciampo · 2024-01-03T22:43:15Z

packages/components/src/tooltip/index.tsx

@@ -66,7 +66,7 @@ function Tooltip( props: TooltipProps ) {

 	const tooltipStore = Ariakit.useTooltipStore( {
 		placement: computedPlacement,
-		timeout: delay,
+		showTimeout: delay,


Looking at the delay prop, it looks like it was always intended to only be applied to the "show" timeout, and not affect the "hide" timeout.

ciampo · 2024-01-03T22:44:51Z

@diegohaz @WordPress/gutenberg-components this PR is ready for another round of review

mirka

Nice improvements! And thanks for the cleanly separated commits, it was easy to go through 👍

packages/components/src/tooltip/test/index.tsx

ciampo requested a review from ajitbohra as a code owner December 22, 2023 15:31

ciampo mentioned this pull request Dec 22, 2023

Fix/tooltip hide on blur #57204

Closed

ciampo self-assigned this Dec 22, 2023

ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Dec 22, 2023

ciampo commented Dec 22, 2023

View reviewed changes

packages/components/src/tooltip/test/index.tsx Show resolved Hide resolved

ciampo requested review from mirka and a team December 22, 2023 15:37

ciampo requested a review from diegohaz December 22, 2023 16:47

diegohaz reviewed Dec 23, 2023

View reviewed changes

packages/components/src/tooltip/test/index.tsx Outdated Show resolved Hide resolved

t-hamano mentioned this pull request Dec 23, 2023

Gallery Block: Remove unused attributes property in shortcode transformation #57356

Merged

ciampo force-pushed the refactor/tooltip-tests branch from ac4d28f to 613bd0d Compare January 3, 2024 15:26

ciampo added 14 commits January 3, 2024 22:51

Remove unnecessary RegExp

e826e66

Use @ariakit/test instead of testing library

6ca3f5d

Use sleep utility instead of custom promise

6b5334f

Remove unnecessary import

c97a986

Add utility functions

8292091

Make multiple children test actually test what it's supposed to test

32b5703

Test for tooltip to hide when another element gets focus

8cd4033

Test for tooltip to hide when hovering outside the anchor

3894dca

Improve tooltip hiding on click test

a6ca61f

More utils refactor

7674cb0

Improve test with disabled anchor

0c47cab

Wait for tooltip to be hidden in the hideOnClick={false} test

18a228c

Refactor custom delay test

5e590e3

Refactor "mouse leave early" test (skip as it's still not passing)

4fe91bc

ciampo added 10 commits January 3, 2024 22:51

Improve shortcut-related tests

be278f8

Improve the Modal Escape test

090e22b

Refactor description text test

b5aaf5a

Add second-level describes

cf1ea1a

Improve TS expecte error message

2d4692a

Add missing async/await in test utils

536ddeb

Await for tooltip to appear in shortcut tests

636678f

Apply delay prop only when showing the tooltip

0e8c5e6

Add skip timeout waiting time after each test

57b5335

CHANGELOG

21d2c0e

ciampo force-pushed the refactor/tooltip-tests branch from 1036668 to 21d2c0e Compare January 3, 2024 22:40

ciampo commented Jan 3, 2024

View reviewed changes

ciampo requested a review from diegohaz January 3, 2024 22:44

ciampo mentioned this pull request Jan 4, 2024

Tooltip: no-op when nested inside another Tooltip component #57202

Merged

mirka approved these changes Jan 4, 2024

View reviewed changes

packages/components/src/tooltip/test/index.tsx Outdated Show resolved Hide resolved

packages/components/src/tooltip/test/index.tsx Outdated Show resolved Hide resolved

packages/components/src/tooltip/test/index.tsx Outdated Show resolved Hide resolved

Apply review feedback

89372ed

ciampo enabled auto-merge (squash) January 4, 2024 23:28

ciampo merged commit bb2a39f into trunk Jan 4, 2024
56 checks passed

ciampo deleted the refactor/tooltip-tests branch January 4, 2024 23:57

github-actions bot added this to the Gutenberg 17.5 milestone Jan 4, 2024

DaniGuardiola mentioned this pull request Apr 19, 2024

Tooltip: remove unecessary delay in tests except where needed #60897

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tooltip: improve tests #57345

Tooltip: improve tests #57345

ciampo commented Dec 22, 2023 •

edited

Loading

ciampo Dec 22, 2023

diegohaz Dec 23, 2023

ciampo Jan 3, 2024

ciampo Dec 22, 2023 •

edited

Loading

diegohaz Dec 23, 2023

ciampo Jan 3, 2024

diegohaz Jan 3, 2024

ciampo Jan 3, 2024

diegohaz Feb 29, 2024 •

edited

Loading

DaniGuardiola Apr 19, 2024

github-actions bot commented Dec 22, 2023

ciampo Jan 3, 2024

ciampo commented Jan 3, 2024

mirka left a comment

Tooltip: improve tests #57345

Tooltip: improve tests #57345

Conversation

ciampo commented Dec 22, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Dec 22, 2023 • 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

diegohaz Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 22, 2023

Choose a reason for hiding this comment

ciampo commented Jan 3, 2024

mirka left a comment

Choose a reason for hiding this comment

ciampo commented Dec 22, 2023 •

edited

Loading

ciampo Dec 22, 2023 •

edited

Loading

diegohaz Feb 29, 2024 •

edited

Loading