BorderControl test: await floating-ui state changes when rendering/opening popover #45241
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
ciampo
added
[Type] Bug
ciampo
approved these changes
Oct 24, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Thank you @jsnajdr for working on this!
Maybe we should add some inline comments in the tests explaining why those unusual act calls are necessary?
I'm also improving the test suite by removing the global props object which is i) mutable and ii) shared by all tests. And replacing it with a createProps() function that creates a props instance for each test separately.
Those are good changes as well. Cc'ing @tyxla who has recently opened a PR refactoring BorderBoxControl tests (https://github.com/WordPress/gutenberg/pull/45208/files).
In general I'm not a fan of having the props/createProps and renderBorderControl / rerenderBorderControl utilities, as they make the tests more abstract and harder to read, but it's definitely not something related to this PR.
Contributor
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for addressing this one @jsnajdr 👍
✅ After checking out #45235 I could replicate the original error
✅ Applying the changes from this PR resolves that issue
Maybe we should add some inline comments in the tests explaining why those unusual act calls are necessary?
This sounds like a good idea. Maybe include the link to the floating-ui docs in those comments as well?
tyxla
approved these changes
Oct 25, 2022
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Thanks for this @jsnajdr 🙌
I generally agree with what @ciampo already offered as feedback and don't have much more to add. The helpers are pre-existing, so I don't feel it's blocking to inline them, it would just be a nice improvement. Also, inline comments would definitely help, because act() around element.focus() seems pretty redundant.
|
|
||
| const renderBorderControl = ( customProps ) => { | ||
| return render( <BorderControl { ...{ ...props, ...customProps } } /> ); | ||
| const renderBorderControl = async ( props ) => { |
Member
There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment.
OK I added explanatory comment so that any reader will understand the connection between the mysterious await act and floating-ui's useFloating hook.
I don't want to spend time on inlining the renderBorderControl calls now. Some wrappers around the plain render and rerender calls still make sense, because there is the additional act() call. We could refactor them to:
renderAndWait( <BorderControl { ...props } /> );making the wrapper a bit shorter, but let's do that in another PR. We could probably avoid all the rerender calls, too, by putting the value prop into local useState of a wrapper component that is actually tested. But let's do that independently, as it's not necessary for the React 18 upgrade.
inline comments would definitely help, because
act()aroundelement.focus()seems pretty redundant.
element.focus() is an issue unrelated to floating-ui and I'd say the need to wrap it in act() is not a mystery at all: we're directly calling a method of the DOM element object, which will internaly fire a focus event, completely circumventing any Testing Library wrappers like fireEvent or userEvent. I did a similar fix for another component in #45243.
jsnajdr
force-pushed
the
fix/border-control-await-floating-ui
branch
from
e94ef94 to
bc2fbe8
Compare
Member
|
The failing PHPUnit tests are fixed now. See #45265. |
jsnajdr
force-pushed
the
fix/border-control-await-floating-ui
branch
from
bc2fbe8 to
6c4349a
Compare
Member
Author
Thank you! I rebased the branch and will merge once the tests come back green. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes an issue where the
BorderControlunit test fails with React 18 (see #45235) with anerror. It happens right after the
BorderControlis rendered or rerendered, or after anopenPopovercall. Why would that happen given thatrenderandfireEventcalls are automatically wrapped withact()? Because the component uses the@floating-ui/react-domlibrary, and that library doessetStatein a promise handler, like:Because the
act()wrappers used byrenderandfireEventare "synchronous", i.e, they areact( () => {} )calls that don't wait for promises to resolve, thesetDatacall escapes outside theact().We need to add an "asynchronous"
act():to get the
setDataflushed before the promise returned byact()resolves.This PR fixes the issue by making the
renderBorderControlandopenPopoverfunctions asynchronous and by adding anawait actcall inside them.The same fix is recommended by the Floating UI docs: https://floating-ui.com/docs/react-dom#testing
I'm also improving the test suite by removing the global
propsobject which is i) mutable and ii) shared by all tests. And replacing it with acreateProps()function that creates apropsinstance for each test separately.This issue was already somewhat discussed back in April in https://github.com/WordPress/gutenberg/pull/32765/files#r845193273.