Tests: Unmount to clear pending popover effects #46303
Conversation
tyxla
added
[Type] Enhancement
tyxla
requested review from
Mamaduka,
youknowriad,
mirka,
jsnajdr,
flootr,
chad1008,
ntsekouras and
brookewp
|
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for the PR @tyxla!
It feels though that it might be difficult to pinpoint if another component just uses the Popover or something else that uses Popover internally, like the Tooltip. How someone could know that they should unmount? The message from the failing CI in the other PR is ["Warning: An update to %s inside a test was not wrapped in act(...).
Can we do something in the Popover to stop side effects or have something in our tests to unmount everything after each test, or something? Or even maybe if we can fix the above warning to display the component it would be enough. What do you think?
I understand the confusion and would definitely love to fix this in a better way, unfortunately, the late state update happens by design in
I personally believe What do you think @ntsekouras? |
There was a problem hiding this comment.
Most of the tests changed by this PR are async -- there is some await userEvent.click() or similar. In that case, manual unmount() is not 100% reliable. It makes it more likely that unmount() wins the async promise race with floating-ui, but doesn't guarantee it.
It was justifiable to do unmount() in these cases in the React 18 migration PR, to unblock the migration faster, but now we should use only the reliable methods, i.e., waiting for the popover to be positioned.
Waiting for the positioning is also a user-visible action, easy to understand from user perspective.
Also, some of the tests modified here do both: waitFor .toBePositionedPopover, and unmount. That shouldn't be necessary.
|
Hey @jsnajdr, From what I've seen, though, it's not enough to wait for the popover to be positioned. I tinkered with it a bit more today and waiting for it to have WDYT - would that be an acceptable solution, given that part of the problem is outside of our control? |
I would like to see and debug the specific test that is failing in an unfixable way. This particular PR doesn't remove any actual "wrapped in
I know only about the But an undetectable |
|
FWIW I removed A good example is this one: #46187 - feel free to try it out and see if there's a rational way to get the tests to pass without the hacks suggested in the floating-ui docs. |
I'll have a look. Checking repeated positioning while scrolling -- yes, that surely looks like a case where things can get interesting. 🙂 |
|
@tyxla Looking at #46187, I see it modifies the styles how a popover is positioned. Instead of using |
That makes sense, but there should likely be more to that. I'm noticing that the tooltip tests will keep failing regardless with this one, and my quick tests indicate that it's coming from here: This seems to be similar to what floating-ui/react-popper#368 describes. Do you have any further suggestions? |
I think they're failing because the I prepared a testing branch (https://github.com/WordPress/gutenberg/tree/try/popover-positioning-classname) where I'm adding a new |
Sounds great, @jsnajdr - do you need anything to make the branch an actual PR? |
|
Brilliant, thanks @jsnajdr. I've updated that PR to close this one when we merge it and will review it shortly. |
What?
This PR updates the tests that open popover to unmount the components, in order to clean up any running or pending popover effects.
Why?
The tests that include the opening of popovers are unreliable and flaky. They depend on us waiting for the popover to be fully finished, visible, and positioned, but that's not enough, since there are effects running afterward, and this may cause
act()warnings in unexpected PRs (like #46187).How?
We're unmounting after such tests to specifically make sure that pending and running popover effects will be canceled on time and before the next test.
Testing Instructions
Verify all tests still pass.