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

NavigationToggle: wait for tooltip positioning in unit test #45587

Closed
wants to merge 3 commits into from
Closed

NavigationToggle: wait for tooltip positioning in unit test #45587

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd5c517
NavigationToggle: wait for tooltip positioning in unit test
jsnajdr Nov 7, 2022
ee016bb
Try with a custom matcher
tyxla Nov 8, 2022
b81f2ff
Update waitFor timeout option to interval
jsnajdr Nov 9, 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
3 changes: 2 additions & 1 deletion ...site/src/components/navigation-sidebar/navigation-toggle/test/__snapshots__/index.js.snap
View file
Open in desktop
Expand Up @@ -26,11 +26,12 @@ exports[`NavigationToggle when in full screen mode should display a default site
<div
aria-hidden="true"
class="components-popover components-tooltip"
style="position: absolute;"
style="position: absolute; left: 0px; top: 1px;"
tabindex="-1"
>
<div
class="components-popover__content"
style="max-height: -4px; overflow: auto;"
>
Toggle navigation
</div>
Expand Down
76 changes: 73 additions & 3 deletions packages/edit-site/src/components/navigation-sidebar/navigation-toggle/test/index.js
View file
Open in desktop
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { render, screen } from '@testing-library/react';
import { render, screen, waitFor } from '@testing-library/react';

/**
* WordPress dependencies
Expand All @@ -24,9 +24,59 @@ jest.mock( '@wordpress/data/src/components/use-dispatch', () => ( {

jest.mock( '@wordpress/core-data' );

/**
* Whether the element has been positioned.
* True if `top` and `left` have been set, false otherwise.
*
* @param {Element} element Element to check.
* @return {boolean} True if positioned, false otherwise.
*/
function isElementPositioned( element ) {
const { getComputedStyle } = element.ownerDocument.defaultView;

const { top, left } = getComputedStyle( element );
return top !== '' && left !== '';
}

/**
* Custom jest matcher.
* Determines whether an element has been positioned or not.
*
* @param {Element} element Element to check.
* @return {Object} Matcher result
*/
function toBePositioned( element ) {
const isInDocument =
element.ownerDocument === element.getRootNode( { composed: true } );
const isPositioned = isInDocument && isElementPositioned( element );
return {
pass: isPositioned,
message: () => {
const is = isPositioned ? 'is' : 'is not';
return [
this.utils.matcherHint(
`${ this.isNot ? '.not' : '' }.toBePositioned`,
'element',
''
),
'',
`Received element ${ is } positioned${
isInDocument ? '' : ' (element is not in the document)'
}:`,
` ${ this.utils.printReceived( element.cloneNode( false ) ) }`,
].join( '\n' );
},
};
}

// Register the custom matcher
expect.extend( {
toBePositioned,
} );

describe( 'NavigationToggle', () => {
describe( 'when in full screen mode', () => {
it( 'should display a user uploaded site icon if it exists', () => {
it( 'should display a user uploaded site icon if it exists', async () => {
useSelect.mockImplementation( ( cb ) => {
return cb( () => ( {
getCurrentTemplateNavigationPanelSubMenu: () => 'root',
Expand All @@ -40,12 +90,23 @@ describe( 'NavigationToggle', () => {

render( <NavigationToggle /> );

// The `NavigationToggle` component auto-focuses on mount, and that
// causes the button tooltip to appear and to be positioned relative
// to the button. It happens async and we need to wait for it.
await waitFor(
() =>
expect(
screen.getByText( 'Toggle navigation' ).parentElement
).toBePositioned(),
{ timeout: 2000 } // It might take more than a second to position the 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.

One little idea: maybe an { interval: 10 } option is better. It better expresses the intent: we don't really need to wait longer, we only need to check more often 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Go for it 👍 Maybe the comment needs some improvements as well, to reflect that it's not about time but it's rather about the waitFor() loops ran under the hood.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented, including a comment update. 👍

);

const siteIcon = screen.getByAltText( 'Site Icon' );

expect( siteIcon ).toBeVisible();
Comment on lines 104 to 106
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if, instead of waiting for implementation details like isPopover which is harder to understand to end users, we could just auto-wait the results directly in the assertions?

await expect( screen.findByText( 'Toggle navigation' ) ).resolves.toBeVisible();

I've not tested this with React 18 though so not sure if it fixes the act() issue. This technique is used by Playwright and I think it makes a lot of sense in unit testing as well.

For reference, this is how it would like in Playwright.

await expect( page.getByText( 'Toggle navigation' ) ).toBeVisible();

Copy link
Member Author

Choose a reason for hiding this comment

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

we could just auto-wait the results directly in the assertions?

The example assertion you proposed is equivalent to:

expect( await screen.findText( 'Toggle navigation' ) ).toBeVisible();

and what it does is:

  1. wait for the "Toggle navigation" element to appear in the DOM. It doesn't have to appear immediately, we'll wait. And it doesn't need to be visible, even a display: none element will be successfully found and returned by findByText.
  2. check if the returned element is visible, and perform the check right now, synchronously. We have only one attempt: if the element is not visible, the check will fail. We'll not wait and try again, like findByText does when it doesn't find the element.

That means that if the tooltip gets initially rendered with display: none or opacity: 0, and is made visible only a bit later, the assertion will fail. We're not waiting for the element to get visible, we're waiting only for it to appear in the DOM.

The test author needs to be aware how exactly the UI evolves and write the right checks.

In our case, we don't want to wait for the tooltip to become visible, but to become positioned. We could write a custom matcher for that, and then write:

const tooltip = screen.getByText( 'Toggle navigation' );
await waitFor( () => expect( tooltip ).toBePositionedPopover() );

This is a combination of sync and async checks. The tooltip element is in the DOM right after render, we don't need to wait. But the positioning happens async a bit later, and we need to wait.

await expect( page.getByText( 'Toggle navigation' ) ).toBeVisible();

In the Testing Library language, this is a fully synchronous check. The await is redundant. getByText doesn't return a promise: it returns either the element it found or throws an error.

Copy link
Member

Choose a reason for hiding this comment

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

I see! Thanks for the explanation! I'm a bit confused though. Isn't the toBeVisible() assertion the only and last assertion in the test? Do we care if the tooltip is positioned correctly? I think we only care if the siteIcon is visible in this test case. Not to mention that the tooltip is sort of an unexpected side effect, or can even be considered a bug.

I guess a better solution, even though not perfect, would be to manually call cleanup() at the end of the test as shown in the doc. I'm aware of your original comment and that you mentioned that it's more like a "hack". However, compared to other solutions (manual act() and unrelated waitFor), I think the cleanup hack might be the best one that doesn't involve many implementation details. WDYT? I'm not sure if this hack even works in this case though 😅 .

In the Testing Library language, this is a fully synchronous check. The await is redundant. getByText doesn't return a promise: it returns either the element it found or throws an error.

The example I shared is not Testing Library's code, but Playwright's code. In Playwright, getByText returns a locator, which lazily locates the element. toBeVisible() also auto-waits and auto-retries until the assertion passes. Playwright also checks for actionability before performing the assertion. But I know that it's not the same as the Jest framework and is not really related to this issue though 😅 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't the toBeVisible() assertion the only and last assertion in the test? Do we care if the tooltip is positioned correctly? I think we only care if the siteIcon is visible in this test case. Not to mention that the tooltip is sort of an unexpected side effect, or can even be considered a bug.

Yes, the test actually doesn't care about the tooltip at all. But the tooltip is there anyway and it spawns an async "thread" that does the positioning. We need to have these threads under control, because if we don't, there are rogue threads from tests that have finished a long time ago which are still fetching or writing to the DOM or whatever. And that causes trouble.

It would help if the computePosition().then() thread was cancelled when the tooltip is unmounted, but that doesn't happen. I verified that it's still running long after the test finished.

This particular tooltip is indeed bug-like: we don't really want to show it on mount, only after closing the navigation menu. After #37314 is finished, the "focus on mount" code can be removed and the tooltip will never be rendered in these tests.

In Playwright, getByText returns a locator, which lazily locates the element.

Thanks for the example. I'm mostly unfamiliar with Playwright API. The API is almost identical to Testing Library, but Playwright is fully async, right? In Testing Library, there are both sync (getBySomething) and async (findBySomething) queries.

I think the cleanup hack might be the best one that doesn't involve many implementation details. WDYT?

I spent some time now carefully debugging the cleanup hack and figuring out how and why it works, and I don't like it at all 🙂 Seems to me that when it works, it works by pure accident.

First, the Testing Library itself registers an afterEach hook that runs cleanup. So it always runs anyway. If I call cleanup manually in my test, the entire difference is that it runs one or two microtasks earlier. That doesn't look like a reliable technique.

The computePosition async thread still runs even after cleanup. So, one way it might effectively work is that cleanup causes computePosition to throw, because, for example, it can't find a DOM element? Then the .then handler doesn't run and doesn't do the state update that triggers the "not wrapped in act" warning.

Another way the test suite avoids triggering the warning is by ending before computePosition resolves. I saw this during a debugging session: computePosition runs and returns results, but neither the .then nor the .catch handler is ever called. The test suite is finished, so Node.js process apparently decided to exit, probably by calling exit(0), without bother to finish executing the rogue threads.

Copy link
Member

Choose a reason for hiding this comment

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

It would help if the computePosition().then() thread was cancelled when the tooltip is unmounted, but that doesn't happen. I verified that it's still running long after the test finished.

Do you have a reference for this? I tried it with React 18 and it worked. It seems like it's also handled in floating-ui judging from their source code.

The API is almost identical to Testing Library, but Playwright is fully async, right? In Testing Library, there are both sync (getBySomething) and async (findBySomething) queries.

Yep, that's why I said that it's not really relevant here. I just wanted to throw it here for reference of how Playwright handles this kind of issue. 😅

If I call cleanup manually in my test, the entire difference is that it runs one or two microtasks earlier. That doesn't look like a reliable technique.

I think you're right, but I don't think this is an accident. I think it might be related to how Jest schedules the test too. I'm not sure if this is correct but it seems like the act warning would only happen on a "properly cleaned up test" if it's scheduling a microtask (promise). Because Jest's afterEach would queue another microtask (again, not sure), the cleanup function would happen after the state has already been updated.

However, for tests that actually care about the position, like the other test below (more on that later), we still need some way to properly wait for it to finish. await act(async () => {}) works, your toBePositioned() works too, either way is fine to me. I would recommend a more explicit way to wait and assert the end result though. Something like:

await waitFor(() => {
  expect( tooltip ).toHaveStyle( `
    left: 0px;
    top: 1px;
  ` );
});

It's very similar to toBePositioned, I think both are fine.

A little bit off-topic: I feel like the snapshot test at the end of the second test is out of place. The test's title doesn't imply anything about the tooltip's position and I don't think it's something we want to test either. I think we can just remove the snapshot test and replace it with explicit assertions if needed. If we do need to check for something that the snapshot is covering, then we should create another dedicated test for that and write explicit and atomic assertions for them.

In this case, given that the tooltip thing is a side-effect, I think we don't need to test that and we can just delete the snapshot. This means we can just use the cleanup trick for both tests in this file without having to introduce toBePositioned or other tricks as suggested above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because Jest's afterEach would queue another microtask (again, not sure), the cleanup function would happen after the state has already been updated.

Yes, I've been debugging why exactly the cleanup trick would work and you're exactly right. Jest runs the test in an async loop:

await runTest();
for ( const hook of hooks.afterEach ) {
  await runHook( hook );
}

and if there are enough registered afterEach hooks, it will take a few ticks to run. The jest-console package that verifies that the test didn't log any rubbish to console, it registers four.

At the same time, computePosition() runs an async loop, too:

for ( let i = 0; i < middlewares.length; i++ ) {
  await middleware[ i ]();
}

It's the same microtask race as the one in waitFor that we discussed with @tyxla. If computePosition() wins the race, there is the act() warning.

So, if the test is synchronous, i.e., it doesn't do any awaiting, we can solve the issue by calling the cleanup synchronously. That guarantees that computePosition() always loses the race even before it started.

For sync test we can use the cleanup solution. For other, more complex tests, we'll need to await the popover positioning.

I implemented the alternative solution in #45726, let's review and merge it instead. The work done in this PR, namely the custom matchers, will be reused elsewhere.

I feel like the snapshot test at the end of the second test is out of place.

That's true, we don't need to check the snapshot to achieve the test's goal, i.e., verifying that the button was rendered and displays the right site icon. I removed it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it has anything to do with "racing" though. cleanup is a synchronous action, it internally unmounts the component and triggers any cleanup effect the component might register. The promise callback will still be fired (unless it uses AbortController which floating-ui doesn't), but setState won't be called to trigger the act warning. That is, if the component properly cleans up the effect after unmount, as every component should, calling cleanup or unmount synchronously should solve the act warning.

The act warning might still happen in between assertions and other updates though, which will be a different issue to tackle, and manual waiting might be required as you suggested. That will depend on whether we care about the updated position, and we can just mock out the library (if we don't) or write explicit assertions about the updated position (if we care). Either way, I think introducing this toBePostioned() helper might be a little early. We can wait and see until there're more such cases. :)

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 don't think it has anything to do with "racing" though.

There is a race, not in the cleanup() function itself, but in the async loop that executes the afterEach hooks. Look at the two async loops I described in the previous comment. One of them runs the afterEach hooks, the other executes the computePosition middlewares and finally calls setState.

It can happen that the setState calls runs on a still-mounted component, because the afterEach loop didn't get to call cleanup() yet. That's why we need to call cleanup() synchronously inside the test body if we want it to run reliably.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, what I mean is that we don't have to care about the race as test authors if we use the cleanup trick. Maybe there're some edge cases that it still matters though!

} );

it( 'should display a default site icon if no user uploaded site icon exists', () => {
it( 'should display a default site icon if no user uploaded site icon exists', async () => {
useSelect.mockImplementation( ( cb ) => {
return cb( () => ( {
getCurrentTemplateNavigationPanelSubMenu: () => 'root',
Expand All @@ -59,6 +120,15 @@ describe( 'NavigationToggle', () => {

const { container } = render( <NavigationToggle /> );

// wait for the button tooltip to appear and to be positioned
await waitFor(
() =>
expect(
screen.getByText( 'Toggle navigation' ).parentElement
).toBePositioned(),
{ timeout: 2000 } // It might take more than a second to position the popover.
);

expect(
screen.queryByAltText( 'Site Icon' )
).not.toBeInTheDocument();
Expand Down