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.

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.

The comment is not 100% correct because the popover will be displayed immediately, only the positioning is delayed.

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.

Thanks for the hints! I've improved the comment in 99069f9 and reordered assertions in 8032ab0.

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();

Sure. Not a huge deal I guess, but happy to make things more clear. Done in 85d318d

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.

I agree with you and have also been pestered by the specificity here. So I went for alternatively checking that we have any top and left in e59733d, and further improved inline docs in 6c63bc5. LMK what you think.

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

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 ?

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.

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?

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.

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.

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.

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 ?

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.

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