-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
LinkControl unit tests: use user.type to type into search field #45802
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
Size Change: 0 B Total Size: 1.3 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM – I don't consider my comment a blocker.
There's one occurrence of searchInput.focus()
left on line 754, shall we use the opportunity to wrap it in act
?
|
||
// Step down into the search results, highlighting the first result item. | ||
triggerArrowDown( searchInput ); | ||
|
||
createButton.focus(); | ||
await user.keyboard( '[Enter]' ); | ||
await user.click( createButton ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description for this test mentions creating entities via keyboard but here we're using a mouse event. As per your PR description it's clear to me why but it may be worth a comment so devs coming here in the future easily understand, too. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, this should be fixed. But when I tried, it becomes a complex task. Let's do it in another PR: this one was supposed to be a search-and-replace easy fix.
There's one occurrence of searchInput.focus() left on line 754, shall we use the opportunity to wrap it in act?
Yes, but later, for the same reasons.
Simpifies the
LinkControl
unit tests by convertingto
because that's exactly what the
user.type
helper is for. It first doesuser.click( searchInput )
, which focuses the element and then dispatches theclick
event, and then it doesuser.keyboard( searchTerm )
on the focused element.This eliminates
searchInput.focus()
calls which would have to be wrapped inact()
during the React 18 migration (#45235). Using the higher-level abstraction saves us some work.We also don't need to focus before
user.clear
becauseuser.clear
internally focuses the element, too.At one place I had to convert
to
We're doing the click with a mouse instead of the keyboard, but it's an irrelevant detail for that particular test.