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.

Sign up for GitHub

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

Jump to bottom

Block Editor: Wait for popover positioning in MediaReplaceFlow tests #45863

Merged
merged 10 commits into from
Nov 21, 2022
Merged

Block Editor: Wait for popover positioning in MediaReplaceFlow tests #45863

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
001d6b6
Block Editor: Wait for popover positioning in MediaReplaceFlow tests
tyxla Nov 17, 2022
e035d52
Remove an extra backtick
tyxla Nov 18, 2022
1f7ee28
Use Element.closest() to locate closest popover
tyxla Nov 18, 2022
99069f9
Fix comment
tyxla Nov 18, 2022
1e65537
Remove an excess assertion
tyxla Nov 18, 2022
8032ab0
Reorder assertions
tyxla Nov 18, 2022
e59733d
Introduce a less-specific way to assert positioning
tyxla Nov 18, 2022
85d318d
Use the right element when locating the popover
tyxla Nov 18, 2022
6c63bc5
Clarify element.style usage
tyxla Nov 18, 2022
56eef5d
Wait for the menu to get visible
tyxla Nov 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 47 additions & 9 deletions packages/block-editor/src/components/media-replace-flow/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { render, screen } from '@testing-library/react';
import { render, screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

/**
Expand Down Expand Up @@ -32,6 +32,34 @@ function TestWrapper() {
);
}

/**
* Returns the first found popover element up the DOM tree.
*
* @param {HTMLElement} element Element to start with.
* @return {HTMLElement|null} Popover element, or `null` if not found.
*/
function getWrappingPopoverElement( element ) {
return element.closest( '.components-popover' );
Copy link
Member Author

Choose a reason for hiding this comment

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

@jsnajdr a note on this approach: when we have direct access to the Dropdown props, we can pass a data-testid prop through popoverProps and it will be a more optimal way to access the popover than using .closest(). I've used this technique in #45911 if you're interested to see an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ?

}

/**
* Asserts that the specified popover has already been positioned.
* Necessary because it will be positioned a bit later after it's displayed.
*
* We're intentionally not using `.toHaveStyle()` because we want to be
* less specific and avoid specific values for better test flexibility.
*
* @async
*
* @param {HTMLElement} popover Popover element.
*/
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( '' ) );
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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. 🤷

/* eslint-enable jest-dom/prefer-to-have-style */
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


describe( 'General media replace flow', () => {
it( 'renders successfully', () => {
render( <TestWrapper /> );
Expand All @@ -57,11 +85,11 @@ describe( 'General media replace flow', () => {
name: 'Replace',
} )
);

const uploadMenu = screen.getByRole( 'menu' );

expect( uploadMenu ).toBeInTheDocument();
expect( uploadMenu ).not.toBeVisible();
await popoverIsPositioned( getWrappingPopoverElement( uploadMenu ) );

await waitFor( () => expect( uploadMenu ).toBeVisible() );
} );

it( 'displays media URL', async () => {
Expand All @@ -78,11 +106,13 @@ describe( 'General media replace flow', () => {
} )
);

expect(
screen.getByRole( 'link', {
name: 'example.media (opens in a new tab)',
} )
).toHaveAttribute( 'href', 'https://example.media' );
const link = screen.getByRole( 'link', {
name: 'example.media (opens in a new tab)',
} );

await popoverIsPositioned( getWrappingPopoverElement( link ) );

expect( link ).toHaveAttribute( 'href', 'https://example.media' );
} );

it( 'edits media URL', async () => {
Expand All @@ -99,6 +129,14 @@ describe( 'General media replace flow', () => {
} )
);

await popoverIsPositioned(
getWrappingPopoverElement(
screen.getByRole( 'link', {
name: 'example.media (opens in a new tab)',
} )
)
);

await user.click(
screen.getByRole( 'button', {
name: 'Edit',
Expand Down