Block Editor: Wait for popover positioning in MediaReplaceFlow tests
#45863
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
tyxla
added
[Type] Code Quality
tyxla
requested review from
Mamaduka,
youknowriad,
mirka,
jsnajdr,
ciampo,
flootr,
chad1008 and
brookewp
|
|
|
Size Change: 0 B Total Size: 1.3 MB ℹ️ View Unchanged
|
tyxla
force-pushed
the
improve/media-replace-flow-tests
branch
from
001d6b6
to
d041185
Compare
jsnajdr
approved these changes
Nov 18, 2022
There was a problem hiding this comment.
Yes this works, although I'm suggesting some improvements. I'd change the getWrappingPopoverElement implementation to use .closest at least if nothing else.
When I cherry-pick the commit into #45235, it makes the unit test suite green again 🎉, after adding popover waiting also to ToggleGroupControl in eb321d0
| return element; | ||
| } | ||
| return getWrappingPopoverElement( element.parentElement ); | ||
| } |
There was a problem hiding this comment.
This could be implemented as just return element.closest( '.components-popover' ). Your version will also crash when the popover element is not found, because it will reach the top of the DOM tree where .parentElement is null and then element.classList will fail.
There was a problem hiding this comment.
Sounds good. I've used .closest() in 1f7ee28. Wanted to point out that I kept the helper function here because it also helps satisfy the no-node-access ESLint rule.
| const uploadMenu = screen.getByRole( 'menu' ); | ||
|
|
||
| expect( uploadMenu ).toBeInTheDocument(); | ||
| expect( uploadMenu ).not.toBeVisible(); | ||
|
|
||
| /** | ||
| * The popover will be displayed and positioned with a slight delay, |
There was a problem hiding this comment.
The comment is not 100% correct because the popover will be displayed immediately, only the positioning is delayed.
There was a problem hiding this comment.
And I would reorder the assertions: first check that the popover is positioned, and only then start asserting on the uploadMenu element.
The test will succeed anyway, but the reordered test tells a better story to a human reader: 1) open the popover 2) wait for it to be displayed and positioned and whatever other dust needs to settle 3) check stuff in the popover.
| expect( | ||
| getWrappingPopoverElement( screen.getByRole( 'menu' ) ) | ||
| ).toHaveStyle( { top: '13px', left: '0' } ) | ||
| ); |
There was a problem hiding this comment.
Again, to tell a better story, I wouldn't look for the .getByRole( 'menu' ) element. The test is not interested in it. Use the link element instead:
const link = screen.getByRole( 'link' ); // or await findByRole if it doesn't appear sync
await waitFor( () => expect( getWrappingPopover( link ) ).toHaveStyle() );
expect( link ).toHaveAttribute();There was a problem hiding this comment.
Sure. Not a huge deal I guess, but happy to make things more clear. Done in 85d318d
| await waitFor( () => | ||
| expect( getWrappingPopoverElement( uploadMenu ) ).toHaveStyle( { | ||
| top: '13px', | ||
| left: '0', |
There was a problem hiding this comment.
The styles are unfortunately overspecified, we'd rather be checking for "top and left styles are not empty", but .toHaveStyle only supports checking for exact values.
All the tests will need to be updated whenever the positioning algorithm changes, although they never care about what the position exactly is.
tyxla
force-pushed
the
improve/media-replace-flow-tests
branch
from
d041185
to
6c63bc5
Compare
|
@jsnajdr thanks for your thorough feedback 🙌 I've pushed a bunch of updates, would you mind taking one last look? |
tyxla
commented
Nov 18, 2022
| * @return {HTMLElement|null} Popover element, or `null` if not found. | ||
| */ | ||
| function getWrappingPopoverElement( element ) { | ||
| return element.closest( '.components-popover' ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Agreed, using data-test-id sounds like a better option when possible.
Do we ever add data-testid to a component's source, or does that only happen inside the tests ?
There was a problem hiding this comment.
I'd never recommend cluttering the component source, but for test fixtures, I've found it to be nice and clean as a last resort.
There was a problem hiding this comment.
I'm afraid I don't get the point of data-testid -- on what scale is it a more optimal way to access the popover? That it doesn't trigger the no-node-access rule?
There was a problem hiding this comment.
That, too, but there is more. It also allows us to avoid testing implementation details, and having to peek into the implementation of other components. That makes our tests more resilient since they don't rely on the implementation details of other components we're not currently testing.
There was a problem hiding this comment.
FWIW I just wanted to point it out as a potential alternative; we can't achieve it here without altering the component source because popoverProps don't accept additional props.
There was a problem hiding this comment.
I think it'll be hard to avoid testing implementation details: the way how the popover is positioned, which element and with what CSS styles, that it's done async -- these are all implementation details. The best we can do is to wrap them inside helpers like custom queries or matchers.
Even the fact that the positioned div and the div that gets the data-testid prop are the same element is an implementation detail. It doesn't always need to be so. The rest props are internally called contentProps, so maybe they were applied to components-popover_content element in the past?
Also, as you write, it's not always possible to pass popoverProps, especially when testing a larger unit, like a full block edit UI, where there are multiple tooltips and menus.
There was a problem hiding this comment.
That's fair, and there's not much we can do to make things easier for composable components that offer larger flexibility through a plethora of props.
Do you have any reservations about the approach proposed in the PR after the changes I made @jsnajdr ?
jsnajdr
approved these changes
Nov 21, 2022
| async function popoverIsPositioned( popover ) { | ||
| /* eslint-disable jest-dom/prefer-to-have-style */ | ||
| await waitFor( () => expect( popover.style.top ).not.toBe( '' ) ); | ||
| await waitFor( () => expect( popover.style.left ).not.toBe( '' ) ); |
There was a problem hiding this comment.
There should be ideally just one waitFor loop, but I know, then ESLint will complain about two expect calls inside waitFor 🙂 Sometimes the rules are a bit too dogmatic IMO.
There was a problem hiding this comment.
Ha yes. I felt the same way about this TBH. That's probably the one rule I haven't seen that much value from. I guess the problem is that awaiting multiple things at the same time can create more paths toward the execution of a test, while when we specifically wait for each assertion one by one, it's more linear and predictable. 🤷
|
Thanks for the stellar feedback here as always, @jsnajdr 🚀 |
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.
What?
This PR updates the
MediaReplaceFlowtests to wait for popover positioning before using the popover contents.An alternative solution to #45736. Closes #45736.
Why?
Those tests are failing in React 18, see #45235. With waiting for positioning to be finished, they'll pass in both React 17 and React 18.
How?
We're simply
await-ing for the popover to be positioned, in other words, to havetopandleftspecified. We're introducing a small inline helper to help locate the first popover up the DOM tree.In #45736 (comment) @jsnajdr suggested that we use the
.toBePositioned()Jest custom matcher, but I found that to be a bit of an overkill, considering that we can just go with a simpletoHaveStyle()assertion instead. So I opted for simplicity.Testing Instructions
npm run test:unit packages/block-editor/src/components/media-replace-flow/test/index.js