Components: upgrade Ariakit to latest #62947
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
DaniGuardiola
added
the
[Type] Code Quality
DaniGuardiola
requested review from
mirka,
jsnajdr,
ciampo and
tyxla
and removed request for
ajitbohra
|
Size Change: +2.82 kB (+0.16%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Since there are no evident breaking changes that affect us on changelog inspection, but we have many failing tests, I'm going to take another strategy. I will go version by version and addressing any errors step by step. |
|
Also ping @WordPress/gutenberg-components if you can take a look at failing tests in CI and see if anything rings a bell or if you have any ideas. Since you're more familiar with the code, you might be able to save me some research in some cases. |
|
Flaky tests detected in 49a959f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10109063935
|
Mamaduka
added
[Feature] UI Components
|
Happy to pair on this one whenever you've got time to work on it |
For some of the tests, it might be as easy as replacing the to: Note that historically when tabbing, we needed to do |
That has definitely proven true for other ariakt-based components like |
|
I have some unpushed fixes like that, let me push soon and I'll let y'all know so we can figure out any remaining cases. |
|
See last update (update 8) - 0.4.0 version makes a bunch of tests fail. |
At a first glance, it seems like something changed with the ariakit test helpers like For example, I've pushed 49a959f, which should resolve the |
This reverts commit 49a959f.
|
See update 9, I found the root causes to all test failures on v0.4.0 🎉 Edit: fixed now and all tests pass, see update 10. |
Unfortunately a large portion of the Gutenberg app is not type checked 😅 ( |
|
@mirka good point. Fortunately, I think most of these features were not exposed by components in the first place (child render function, That said, I will do another exhaustive pass to see if there's something missing in regards to this. |
ciampo
reviewed
Jul 29, 2024
There was a problem hiding this comment.
Great work so far, @DaniGuardiola ! I left some comments, hopefully I didn't miss some context.
That custom render function looks scary, hopefully we'll manage to move that code either into @ariakit/test or @testing-library 🤞
Comment on lines
-68
to
-97
| it( 'should associate the tooltip text with its anchor via the accessible description when visible', async () => { | ||
| render( <Tooltip { ...props } /> ); | ||
|
|
||
| // The anchor can not be found by querying for its description, | ||
| // since that is present only when the tooltip is visible | ||
| expect( | ||
| screen.queryByRole( 'button', { description: 'tooltip text' } ) | ||
| ).not.toBeInTheDocument(); | ||
|
|
||
| // Hover the anchor. The tooltip shows and its text is used to describe | ||
| // the tooltip anchor | ||
| await hover( | ||
| screen.getByRole( 'button', { | ||
| name: 'Tooltip anchor', | ||
| } ) | ||
| ); | ||
| expect( | ||
| await screen.findByRole( 'button', { | ||
| description: 'tooltip text', | ||
| } ) | ||
| ).toBeInTheDocument(); | ||
|
|
||
| // Hover outside of the anchor, tooltip should hide | ||
| await hoverOutside(); | ||
| await waitExpectTooltipToHide(); | ||
| expect( | ||
| screen.queryByRole( 'button', { description: 'tooltip text' } ) | ||
| ).not.toBeInTheDocument(); | ||
| } ); | ||
|
|
There was a problem hiding this comment.
Is this test going to be restored before merging this PR?
There was a problem hiding this comment.
No, unless we want to re-implement this functionality (I don't think we should), see https://github.com/WordPress/gutenberg/pull/62947/files/4c022ed8d80fdce62795e3c0895fd3dfcbc08421..114badda5f68770118090baaf032c1a19b4cf84a#diff-eb9fcb45579ca92e96f172d835623c0682387b4ec0ff0e7ed5009b958b7c2c5c
Comment on lines
-496
to
-500
| expect( | ||
| screen.getByRole( 'button', { | ||
| description: 'Outer tooltip', | ||
| } ) | ||
| ).toBeVisible(); |
There was a problem hiding this comment.
Why was this assertion deleted?
There was a problem hiding this comment.
| @@ -47,7 +47,7 @@ async function renderAndValidate( ...args: Parameters< typeof render > ) { | |||
| const activeButton = queryByAttribute( | |||
| 'data-active-item', | |||
| view.baseElement, | |||
| '' | |||
| 'true' | |||
There was a problem hiding this comment.
With the new render function, could we actually remove the renderAndValidate utility in this file?
|
Thanks @ciampo! I'm moving to #64066 because it was better to start clean due to the annoying npm issue. @WordPress/gutenberg-components this PR can still be useful to reference old information I left here, but I tried to make the new PR "complete enough" in terms of context needed to understand what's going on. I also ensured that CI unit tests ran completely for the versions with failures, so that we can check those logs if necessary and for the record. Plus the version-by-version commit history should help in future See you there! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Outdated PR, see #64066
Moved over from personal repo, see original PR: #60992
Went through all breaking changes in all relevant versions. Only 0.4.0 had breaking changes (it's a "pseudo-major", so it makes sense). Here's a summary of those changes, TL;DR it doesn't seem like we need to do anything about them, and we just need to test enough to be confident in the upgrade.
packages/dataviews/src/search-widget.tsx, unaffected.There is another change not considered "breaking" by Ariakit, but it deserves consideration:
Previously, Ariakit added
aria-describedByto tooltip anchors. Now, it doesn't, and users are responsible for making anchors accessible, see: https://ariakit.org/components/tooltip#tooltip-anchors-must-have-accessible-namesRemaining work:
renderin problematic tests with@ariakit/testversion.@ariakit/testto latest version.renderchanges to upstream@ariakit/test(returning the output of the wrappedReactTestingLibrary.rendercall, e.g.rerender, etc).@ariakit/test.renderin appropriate place.@ariakit/test'srenderin all components package tests? (e.g.noRestrictedImports)Update 1 - Initial state of tests as of v0.3.13 (79fa288):
After turning a couple of
getByRoleintoawait findByRole(179f374):(sometimes it fails on 16 tests, not sure which one is the flaky one yet)
Update 2 - Still on v0.3.13. After merging trunk in, there are new failing tests (aebd132):
Additionally, there's a new type error after unifying the version in
dataviews:This is due to this unification being essentially a downgrade, since the prop value is from a recent version:
For now, I'm doing a quick fix in order to keep upgrading version by version:
Update 3 - Still on v0.3.13. Using
renderfrom Ariakit incustom-select-control/test/index.tsxpreventsactwarnings, fixing one test (0c64d63).Before:
After:
The remaining test is related to this re-render issue, for which we're waiting on Ariakit to signal if a fix can be provided on their side: ariakit/ariakit#3939 (comment)
Current global state of tests:
(no change probably due to the flaky one)
Update 4 - Still on v0.3.13. Using
renderfrom Ariakit intabs/test/index.tsxpreventsactwarnings, fixing seven tests (3be0bc2).Unfortunately, this also causes a few new errors due to the rerender issue, but those should be fixed once that's addressed at a fundamental level.
There are also some remaining tests that weren't failing due to act warnings and still fail after this change. They will need to be reviewed individually to figure out what's going on with them.Apparently, not true. It was all fixed by proper rerendering. See next update.Before:
After:
Current global state of tests:
Higher amount (+2 including flaky, which didn't fail this time around) due to reasons stated above.
Update 5 - Implemented
rerendermethod (bac4026), and it works!! All tests in Tabs are fixed by this after updating to this API (11a2492).To achieve this, I temporarily copied the Ariakit
rendermethod into the codebase (68afa3c), and modified to provide a way to re-render with the same safeguards as the originalrender(e.g. preventingactwarnings). It is used like this:After updating the Tabs tests, all of them now pass! 🎉
Current global state of tests:
As you can see, with Tabs tests fixed, the number is WAY down.
Update 6 - Reverted manual fixes from update 1 (affecting) two files, and switch to the Ariakit
rendermethod instead (f8ada30 and 375c5ca).As there's now a fundamental fix, it's better to consistently use it instead of relying in manual fixes.
Update 7 - After updating remaining tests with the Ariakit
renderutility, all tests are now passing! (f4f5cb9 and 38383d1)Update 8 - The rest of the 0.3.x minors pass all tests, but tests start failing again after upgrading to 0.4.0 (7e7cb32).
Oh well! Got more work cut out for me.
Full output: https://pastebin.com/raw/rDPQ1XFs
Note:
@ariakit/testwas upgraded to latest version, didn't break anything.Update 9 - I found the root causes of all test failures and left TODO comments on each relevant place, see this diff: https://github.com/WordPress/gutenberg/pull/62947/files/4c022ed8d80fdce62795e3c0895fd3dfcbc08421..114badda5f68770118090baaf032c1a19b4cf84a
Here's a full list of causes, see linked diff above for details:
''value in a data attribute, but the right value is now'true'.These findings cover ALL failing tests cases.
Update 10 - On 0.4.0 with all tests passing! (6679cc6)
Update 11 - Upgraded to 0.4.6 version by version, and all tests pass in each of them (6ed775d)
However, when upgrading to 0.4.7 there are some tests that now fail, see full output: https://pastebin.com/raw/0BAqt1th
Separately, the amount of deletions in the
package.jsonseems extremely sus, so I'm gonna look into that as I might have messed something up along the way (I needed to do some dirty hacks to get npm to actually upgrade in some cases).Update 12 - The package-lock file was indeed messed up in some way. After cleaning up and testing a variety of versions of Ariakit, it seems like those failures happen even down to the 0.4.0 versions. Will need to investigate more, but at least the mess has been cleaned up now and we're on the latest versionof all ariakit packages (c32b9a0).
Still 5 test failures to go (2 of them seem flaky). Updated test output: https://pastebin.com/raw/fDZjTc4k