InputControl: Add tests and update to use @testing-library/user-event
#41421
Conversation
This file contains 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
|
Size Change: 0 B Total Size: 1.24 MB ℹ️ View Unchanged
|
stokesman
changed the title
InputControlInputControl: Add tests and update unit tests to use `@testing-library/user-event
stokesman
changed the title
InputControl: Add tests and update unit tests to use `@testing-library/user-eventInputControl: Add tests and update to use `@testing-library/user-event
stokesman
changed the title
InputControl: Add tests and update to use `@testing-library/user-eventInputControl: Add tests and update to use @testing-library/user-event
stokesman
force-pushed
the
update/unit-test-input-control
branch
2 times, most recently
from
d17c28b to
7c6912e
Compare
mirka
reviewed
Jun 1, 2022
|
|
||
| // Ensuring <InputControl /> is controlled. | ||
| fireEvent.blur( input ); | ||
| await user.click( document.body ); |
There was a problem hiding this comment.
I mildly agree with testing-library/user-event#189 (comment) to begin with, and it's made even more unclear because of the incidental comment above it on L90 😆 Maybe we should adjust the comment placement there, and/or add an inline comment or something to note that the intent is to blur?
There was a problem hiding this comment.
I'd meant to revisit this after #40518 landed because I'd hoped the blur would no longer be necessary. It doesn't feel like it should be because the component no longer blocks updates from props strictly while focused. Still the test won't pass when it's removed 😕. For now, I've removed the comment about ensuring it’s controlled because that was specifically meant to reference the old behavior (introduced in #25609).
I wonder if you think it'd be a change for the better to test it in a more explicitly controlled fashion:
it( 'should work as a controlled component', async () => {
const user = setupUser();
const spy = jest.fn();
const Example = () => {
const [ state, setState ] = useState( 'one' );
const onChange = ( value ) => {
setState( value );
spy( value );
};
const onKeyDown = ( { key } ) => {
if ( key === 'Escape' ) setState( 'three' );
};
return (
<InputControl
value={ state }
onChange={ onChange }
onKeyDown={ onKeyDown }
/>
);
};
render( <Example /> );
const input = getInput();
await user.type( input, '2' );
// Make a controlled update.
await user.keyboard( '{Escape}' );
expect( input ).toHaveValue( 'three' );
/*
* onChange called only once. onChange is not called when a
* parent component explicitly passed a (new value) change down to
* the <InputControl />.
*/
expect( spy ).toHaveBeenCalledTimes( 1 );
} );That way eliminates the need to blur the input.
Also on the general idea of what is the most clear way to blur elements, I don't really have an opinion. I'd be glad to use user.tab() instead but so far it seems like our other tests have been going with user.click( document.body ).
There was a problem hiding this comment.
Also on the general idea of what is the most clear way to blur elements, I don't really have an opinion. I'd be glad to use user.tab() instead but so far it seems like our other tests have been going with user.click( document.body ).
I've been using the same technique on UnitControl (example), and just added a comment to clarify my intentions.
We could also look into adding some "blur" utility function to our testing utils, in case that makes the code more clear to read
There was a problem hiding this comment.
I wonder if you think it'd be a change for the better to test it in a more explicitly controlled fashion
I really like your code snippet! It paints a very realistic scenario, so it's easy to envision what it's testing and why. I'll defer to you on it, but if you also think it's better than the current test, I'm all for it 👍
We could also look into adding some "blur" utility function to our testing utils, in case that makes the code more clear to read
Yeah, this is an interestingly hard problem! On one hand, "blur" is such an explicit implementation detail-y thing to do, which is kind of counter to the whole idea of user-event and how we want to simply mimic real user interactions. So in that sense, maybe it is better to express those not as "blur"s but as clicking or tabbing away.
I guess the potential confusion as a code reader is less about "can I understand that this is a blur", and more about "can I understand why this blur is necessary for the test". It'll always be better to have a comment if the latter is unclear.
There was a problem hiding this comment.
Agreed with all points above:
- up for switching the unit test to the one proposed by @stokesman in
InputControl: Add tests and update to use @testing-library/user-event #41421 (comment) - I say, let's keep writing explicit clicks on
document.bodyfor now, and make sure that they are well commented out. We can keep an eye on this pattern and decide to refactor to a utility later on if we feel it's necessary
There was a problem hiding this comment.
Swapped out the test in 1cb8978. Also noticed one last place the document.body click wasn't commented and added one f448bc6.
stokesman
force-pushed
the
update/unit-test-input-control
branch
2 times, most recently
from
16636cd to
7b41c82
Compare
ciampo
added
[Type] Automated Testing
ciampo
reviewed
Jun 9, 2022
There was a problem hiding this comment.
I'd still like to wait for @mirka 's approval for this PR, especially regarding the conversation in https://github.com/WordPress/gutenberg/pull/41421/files#r886950299
Otherwise, the rest of the changes LGTM
mirka
approved these changes
Jun 9, 2022
Co-authored-by: Marco Ciampini <[email protected]>
stokesman
force-pushed
the
update/unit-test-input-control
branch
from
53422c3 to
f448bc6
Compare
|
Thanks Lena and Marco for the reviews and suggestions! CI has to finish but this should be ready 🚢 |
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.
What?
Updates the
InputControlunit tests to use@testing-library/user-eventfor interacting with the component and adds a couple new tests.Why?
How?
https://testing-library.com/docs/ecosystem-user-event/
Testing Instructions
npm run test-unit input-control