Skip to content
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

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

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

DaniGuardiola
Copy link
Contributor

@DaniGuardiola DaniGuardiola commented Apr 19, 2024

What?

There was a need to add a delay after every Tooltip test. After an Ariakit update, it's not necessary anymore for every test. See discussion: #57345 (comment)

Why?

Faster tests.

How?

Removed delay after every test and only added where necessary (after one specific test).

Testing Instructions

Run Tooltip unit tests.

Testing Instructions for Keyboard

N/A

Screenshots or screencast

N/A

Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@mirka mirka requested a review from a team April 19, 2024 11:42
@mirka mirka added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Package] Components /packages/components labels Apr 19, 2024
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks 👍

Let's keep an eye out for flakiness 👁️

- `Text`: Add `text-wrap: pretty;` to improve wrapping. ([#60164](https://github.com/WordPress/gutenberg/pull/60164)).
- `Navigator`: Navigation to the active path doesn't create a new location history. ([#60561](https://github.com/WordPress/gutenberg/pull/60561))
- `FormToggle`: Forwards ref to input. ([#60234](https://github.com/WordPress/gutenberg/pull/60234)).
- `ToggleControl`: Forwards ref to FormToggle. ([#60234](https://github.com/WordPress/gutenberg/pull/60234)).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning those up 🙌

// Prevent this test from interfering with the next one.
// "Tooltips appear instantly if another tooltip has just been hidden."
// See: https://github.com/WordPress/gutenberg/pull/57345#discussion_r1435495655
await sleep( 3000 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, there would be a better way, and something more specific to wait for, but this is not related to the PR, so 🤷

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this won't be needed with @ariakit/react@^0.4.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I didn't realize we're still on 0.3.12, is there any discussion on upgrading? Any relevant blockers? @tyxla

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there were some v0.4.0 breaking changes we need to address, like:

  • We may have to manually set role="gridcell" to grid cells within Composite when using role="grid"
  • Data attributes can no longer be empty strings
  • Type changes

Nothing too critical though - I'd go through all the breaking changes and if there are any that apply to the package, verify that we update properly without breaking something.

Also I haven't been directly involved with updating Ariakit before, so perhaps @mirka might know more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the protocol, I'd love for us to update to the latest version. There are so many improvements and new features we could benefit from!

@tyxla tyxla merged commit 7d9e390 into WordPress:trunk Apr 19, 2024
63 of 65 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.2 milestone Apr 19, 2024
@ciampo ciampo mentioned this pull request Jul 17, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants