Refactoring BorderControl's unit tests
#54155
Conversation
This patch migrates uses of `fireEvent` in `BorderControl` to `testing-library/user-event`, where possible; there is one remaining usage that isn't currently supported by `userEvent`.
Was running the wrong tests locally
…fireevent-userevent
|
|
||
| expect( props.onChange ).toHaveBeenNthCalledWith( 3, { | ||
| expect( props.onChange ).toHaveBeenNthCalledWith( 4, { |
There was a problem hiding this comment.
What is causing the onChange prop to be called one more time? Could it be related to the fact that user.type also clicks on the element? If that's the case, we could try passing the skipClick option, or use user.key instead (since the input already has focus).
Same applies to other instances of this PR where the number of times that the onChange prop was called changed.
There was a problem hiding this comment.
It's because of the additional user.clear call, which generates a change event of its own. Without it, user.type appends to the current value, rather than replacing it (which makes sense).
Did a bit of digging though, and user.type also accepts a selection range, which means the value of the field is totally replaced. This is closer to the original intent of directly firing a change event, so while user.clear => user.type is probably closer to most user interactions, user.type with a selection is probably preferable.
await user.type( getWidthInput(), '0', {
initialSelectionStart: 0,
initialSelectionEnd: 1,
} );There was a problem hiding this comment.
Thank you for the digging, your proposed solution looks good :)
Calling `user.clear` was necessary to prevent `user.type` from adding to an existing input value. However, by providing a selection range to `user.type` we can avoid that call entirely.
…fireevent-userevent
|
|
||
| expect( props.onChange ).toHaveBeenNthCalledWith( 3, { | ||
| expect( props.onChange ).toHaveBeenNthCalledWith( 4, { |
There was a problem hiding this comment.
Thank you for the digging, your proposed solution looks good :)
mikachan
added
[Type] Enhancement
What?
This patch migrates uses of
fireEventinBorderControltotesting-library/user-event, where possible; there is one remaining usage around sliders/range inputs that isn't currently supported byuserEvent.Why?
We should, where feasible, be using
userEventoverfireEvent, as it better replicates how users interact with the DOM.How?
The
BorderControltest file has been refactored to (mostly) replacefireEventcalls with an equivalent use ofuserEvent.Testing Instructions
Nothing user-facing is changed, only automated tests. This should be covered by the CI running against this PR.