NavigationToggle: wait for tooltip positioning in unit test #45587
Conversation
|
|
|
Size Change: +5.51 kB (0%) Total Size: 1.29 MB
ℹ️ View Unchanged
|
| const siteIcon = screen.getByAltText( 'Site Icon' ); | ||
|
|
||
| expect( siteIcon ).toBeVisible(); |
There was a problem hiding this comment.
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();There was a problem hiding this comment.
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:
- 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: noneelement will be successfully found and returned byfindByText. - 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
findByTextdoes 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.
There was a problem hiding this comment.
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
awaitis redundant.getByTextdoesn'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 😅 .
There was a problem hiding this comment.
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 thesiteIconis 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,
getByTextreturns 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
cleanuphack 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Because Jest's
afterEachwould queue another microtask (again, not sure), thecleanupfunction 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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
|
Hey, @jsnajdr thanks for this work! Really nice catch that it takes more time for the popovers to get positioned and we've been storing obsolete snapshots as a consequence. I've just pushed a commit that does a few things:
Let me know what you think. |
I think it's more ergonomic if the matcher finds the expect( screen.getByText( 'toggle' ) ).toBePositioned()what I mean by it is "find a popover that contains the 'toggle' text and check if it's positioned". If that popover is like <Tooltip><b>toggle</b> navigation</Tooltip>then
Oh no, this is just another random coincidence. There is no waiting or timeouts involved, just promise chains, where all promises resolve immediately. Like while ( ! finished ) {
advanceTimers(); // synchronously advance fake timers by 50ms and run their handlers
checkCallback(); // check the `waitFor` callback after running the timers
await Promise.resolve(); // do a microtask tick so that promise handlers can run
}With the default 1000 timeout this loop runs 20 times before giving up, with 2000 it runs 40 times. Advancing timers by 50ms in each iteration. Because there are no timers to advance, it does nothing else but run 20/40 microtask ticks. In the Floating UI's for ( let i = 0; i < middleware.length; i++) {
await middleware[i]();
}In our case, this await loop takes exactly 39 microtask ticks to finish. That's why the That's how it works with React v17 and |
I kinda disagree, this adds some confusion and reduces the control. In my mind, selecting an element and then performing an assertion on it are two separate things, and I find it counterintuitive that the matcher will attempt to find an element, although I've already provided one. If you feel strongly about this, I can reconsider, but it would be a better idea if we make sure to include that fact somehow in the matcher name. |
I agree, and won't mind if they are two separate functions, like: expect( screen.findPopoverByText( 'toggle' ) ).toBePositioned();
I'd just prefer to have a convenient |
I think we have an agreement 👍 I'm happy to have 2 separate functions with 2 separate responsibilities - one for retrieving the popover element and one for the matcher. You're right about the |
Tracked down the difference to this: as I wrote above, the pseudocode for the while ( ! finished ) {
advanceTimers(); // synchronously advance fake timers by 50ms and run their handlers
checkCallback(); // check the `waitFor` callback after running the timers
await Promise.resolve(); // do a microtask tick so that promise handlers can run
}that's the same both in v12 and v13 of Testing Library. However, in both version the await advanceTimersWrapper( () => Promise.resolve() );and it's the In v12, the entire So, in v13 we have: await act( () => Promise.resolve() );Without async function act( fn ) {
await fn();
while ( hasPendingUpdates() ) {
flushPendingUpdates();
await new Promise( r => setImmediate( r ) );
}
}It runs the Here for ( let i = 0; i < 1000; i++ ) {
await Promise.resolve();
}will schedule 1000 microtasks, and all these microtasks have higher priority than the Conclusion: this PR doesn't have a chance to run reliably with React 17. Only React 18 and |
| expect( | ||
| screen.getByText( 'Toggle navigation' ).parentElement | ||
| ).toBePositioned(), | ||
| { timeout: 2000 } // It might take more than a second to position the popover. |
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Implemented, including a comment update. 👍
|
I think #45726 is an excellent alternative to this PR! I'd suggest going with that, and if we ever need the positioned matcher or the popover finder, we can always reuse that code. |
I think we'll need the popover finder for any test that |
|
Closing as the alternative solution in #45726 got merged instead. |
Fixes the unit test for
NavigationTogglewhich triggers the "update not wrapped in act()" warning after React 18 upgrade, as in #45235.NavigationToggleis this site icon button in the top-left corner that opens the nav sidebar:It has a not-entirely-desired behavior (which is going to stay for a while though, see #37265) where it grabs focus on mount, and focus leads to displaying the "Toggle navigation" tooltip. The tooltip gets positioned asynchronously, and a correct unit test needs to wait for it to finish, otherwise the positioning update will trigger the act() warning.
This PR implement the Right Solution™ to that problem, one which I outlined in #45235 (comment): wait for the positioning DOM changes with
await waitFor()after render, and proceed with the test only after the positioning is done. See theisPopoverhelper. And it works! The tests start passing in the React 18 migration branch, and also please notice the snapshot changes: thecomponent-popoverelements now have the final positioning styles. We're indeed testing the final state.TODO for follow-up PRs: the
isPopoverhelper should be improved, so thatexpectcalls that use it are as ergonomic as possible (suggestions welcome) and extracted into a common helper shared by other tests.