ComboboxControl: add unit tests #42403
Conversation
|
Size Change: 0 B Total Size: 1.24 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thank you @chad1008 for working on this! Adding unit tests to this component will be of great value.
Regarding what other tests we could add, here's what comes to mind:
- we should test both the controlled and uncontrolled version of the component
- we should add tests in which the user interacts with the component exclusively using the keyboard
- We should add a test for a more complete and typical user interaction, involving search and filtering. For example:
- user clicks on the combobox
- (assert that all options are being rendered)
- user moves focus on the search input
- user types a string that should not generate any matches
- (assert that there are no options being rendered)
- user clears text search input
- (assert that all options are being rendered)
- user types a new search string, this time one that should match a few options
- (assert that the options that are being rendered are the expected ones matching the search string)
- user clicks on one of the available options
- (assert that input value and onChange spy have the new correct value selected)
- etc...
A few important reflections that I noticed while reviewing this component:
- The last test has revealed a potential discrepancy: it looks like the
<input />'svalueand the argument passed to theonChangecallback are not the same value 😱 Shouldn't the expectation be that theinput'svalueis theabbreviation, instead of thename?between the value passed toonChangeand the value of the `input - Not sure if this is an accessibility issue, but while reviewing the tests, I noticed that the
optionsare not rendered to the DOM until the combobox is focused/clicked. This differs from otherComboboxexamples where those option items are rendered in the DOM, but just not visible (e.g.ariakit) userEventhas some nice utility functions, including one to select or deselectoptions — unfortunately I tried it for these tests, but it doesn't seem to work because we're using custom aria roles, instead of nativeselectandoptionelements.
cc @mirka , especially regarding the last 3 points.
We should also keep in mind that this component is potentially going to be rewritten using ariakit component primitives.
@stokesman , if I remember correctly you had worked a few weeks ago on adding some unit tests for a component in which you were testing for both the controlled and uncontrolled version of such component? |
Yes, it was |
|
Wow, thank you for the thorough review at @ciampo... really informative and super helpful. I've pushed some updates addressing most of your points. Still to do:
The second item in that list will be tricky. Thus far I haven't had any luck getting I'm not sure why yet, but I have seen reports of issues with arrow keys on other elements, which could be related (or possibly not?) |
Nice, thank you @stokesman! |
There was a problem hiding this comment.
2. Not sure if this is an accessibility issue, but while reviewing the tests, I noticed that the
optionsare not rendered to the DOM until the combobox is focused/clicked. This differs from otherComboboxexamples where those option items are rendered in the DOM, but just not visible (e.g.ariakit)
I think it should be fine, given that options can often be lazy loaded (e.g. virtualized or paginated).
|
Interesting development I'd love some input on. I was writing a test for invalid search strings and found the component currently behaves differently than I'd expect. When the input gets focus, it starts by highlighting the zero indexed element of the list, which makes sense. As the user types it filters that list. If a string with no matches is entered, I expected it would set the value to an empty string (selecting nothing). It actually selects the original zero-indexed element (in the case of these tests, Greenwich Mean Time). So you can type something that doesn't exist, hit That feels like a bug to address in a separate PR to me... thoughts @ciampo / @mirka ? Edit/correction: On invalid strings, it actually selects whatever the last highlighted item was (not just the zero index), either because of a hover or a partially correct string. For example, in Storybook, typing |
|
I dug a little more and this appears to be intentional. The list is filtered as the user types and invalid matches choose the zero index of the remaining option(s). The goal of this PR appears to be to address confusion when an existing option was already selected a new string was entered. This still feels like a confusing experience to me though, because when I type an invalid string, the suggestion I'm ultimately selecting isn't shown until after I select it. I can think of three options:
|
This definitely looks like an aspect of the component that we can polish — but I would do it in a separate issue. Let's keep this issue focused on adding the essential unit tests to
Just flagging this point in case it got overlooked — this may be an even higher priority task to follow-up with after this PR (and the related exhaustive-deps changes) get merged. |
chad1008
force-pushed
the
add/ComboboxControl-unit-tests
branch
from
1a140a2
to
1db34a1
Compare
|
I've just pushed some updates to address the feedback shared above! I responded to one of the comments directly, but in addition to that I've added additional tests for
Important note: some of the uncontrolled tests will fail until #42752 is merged, as the component's current implementation doesn't support an uncontrolled mode. Without a value prop passed in, the component never actually inserts a selection into the input element. The linked PR addresses that (hat tip to @ciampo for teaching me how to go about implementing that hook!) |
|
Sounds good! Let's merge #42752 first, rebase this PR, and then give it a final round of review. |
chad1008
force-pushed
the
add/ComboboxControl-unit-tests
branch
from
c3a8fa7
to
3a1f950
Compare
|
#42752 merge and a rebase of this PR are complete. Marking as ready for review! |
| const getInput = ( name ) => screen.getByRole( 'combobox', { name } ); | ||
| const getOption = ( name ) => screen.getByRole( 'option', { name } ); | ||
| const getAllOptions = () => screen.getAllByRole( 'option' ); | ||
| const setTargetOption = ( index ) => { |
There was a problem hiding this comment.
This comment is a mix of nitpicking and general advice:
- I believe a more appropriate name for this function would be
getTargetOption, as you're passing an index and getting the option in return - Since we're setting the
optionsprop in each test, it feels more correct that this function takes the array of options as a parameter too, instead of assuming that to always betimezones - Overall, I'm not sure about how much value does this abstraction holds. It basically exists in order to append the
searchStringproperty. At most, if we wanted to abstract that piece of functionality away, we could at most create a utility function likegetSearchString( option ) => option.label.substring( 0, 11 )— or even have no "utility" function at all
There was a problem hiding this comment.
Agreed on the function name and the options parameter. Made both of those changes! 👍
Re: the abstraction itself... in addition to appending the searchString property, I also felt like it could be beneficial from a maintenance perspective to have the desired index declared once per test.
The target property gets referenced as much as three times per test. Without the abstraction, if we ever update the target of a given test we need to make sure to hit all three instances of it. With the abstraction it can just be updated once for the test in question, because the abstraction keeps it nice and DRY.
I'm happy to remove it or replace it with a getSearchString() if you think that would be best though.
There was a problem hiding this comment.
Re: the abstraction itself... in addition to appending the searchString property, I also felt like it could be beneficial from a maintenance perspective to have the desired index declared once per test.
The target property gets referenced as much as three times per test. Without the abstraction, if we ever update the target of a given test we need to make sure to hit all three instances of it. With the abstraction it can just be updated once for the test in question, because the abstraction keeps it nice and DRY.
For completeness sake, here's what I had in mind when writing the previous message:
click to expand
diff --git a/packages/components/src/combobox-control/test/index.js b/packages/components/src/combobox-control/test/index.js
index 2bcd980982..72033f679f 100644
--- a/packages/components/src/combobox-control/test/index.js
+++ b/packages/components/src/combobox-control/test/index.js
@@ -53,10 +53,7 @@ const getLabel = ( labelText ) => screen.getByText( labelText );
const getInput = ( name ) => screen.getByRole( 'combobox', { name } );
const getOption = ( name ) => screen.getByRole( 'option', { name } );
const getAllOptions = () => screen.getAllByRole( 'option' );
-const getTargetOption = ( options, index ) => {
- const searchString = options[ index ].label.substring( 0, 11 );
- return { searchString, ...options[ index ] };
-};
+const getOptionSearchString = ( option ) => option.label.substring( 0, 11 );
const setupUser = () =>
userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
@@ -138,7 +135,7 @@ describe.each( [
it( 'should select the correct option via click events', async () => {
const user = setupUser();
- const targetOption = getTargetOption( timezones, 2 );
+ const targetOption = timezones[ 2 ];
const onChangeSpy = jest.fn();
render(
<Component
@@ -163,7 +160,7 @@ describe.each( [
it( 'should select the correct option via keypress events', async () => {
const user = setupUser();
const targetIndex = 4;
- const targetOption = getTargetOption( timezones, targetIndex );
+ const targetOption = timezones[ targetIndex ];
const onChangeSpy = jest.fn();
render(
<Component
@@ -192,7 +189,7 @@ describe.each( [
it( 'should select the correct option from a search', async () => {
const user = setupUser();
- const targetOption = getTargetOption( timezones, 13 );
+ const targetOption = timezones[ 13 ];
const onChangeSpy = jest.fn();
render(
<Component
@@ -207,7 +204,7 @@ describe.each( [
await user.tab();
// Type enough characters to ensure a predictable search result
- await user.keyboard( targetOption.searchString );
+ await user.keyboard( getOptionSearchString( targetOption ) );
// Pressing Enter/Return selects the currently focused option
await user.keyboard( '{Enter}' );
@@ -219,7 +216,7 @@ describe.each( [
it( 'should render aria-live announcement upon selection', async () => {
const user = setupUser();
- const targetOption = getTargetOption( timezones, 9 );
+ const targetOption = timezones[ 9 ];
const onChangeSpy = jest.fn();
render(
<Component
@@ -233,7 +230,7 @@ describe.each( [
await user.tab();
// Type enough characters to ensure a predictable search result
- await user.keyboard( targetOption.searchString );
+ await user.keyboard( getOptionSearchString( targetOption ) );
// Pressing Enter/Return selects the currently focused option
await user.keyboard( '{Enter}' );
@@ -248,7 +245,7 @@ describe.each( [
it( 'should process multiple entries in a single session', async () => {
const user = setupUser();
const unmatchedString = 'Mordor';
- const targetOption = getTargetOption( timezones, 6 );
+ const targetOption = timezones[ 6 ];
const onChangeSpy = jest.fn();
render(
<Component
@@ -289,12 +286,13 @@ describe.each( [
} );
// Run a second search with a valid string.
- await user.keyboard( targetOption.searchString );
+ const searchString = getOptionSearchString( targetOption );
+ await user.keyboard( searchString );
const validSearchRenderedOptions = getAllOptions();
// Find option that match the search string.
const matches = timezones.filter( ( option ) =>
- option.label.includes( targetOption.searchString )
+ option.label.includes( searchString )
);
// Confirm the rendered options match the provided dataset based on the current string.Removing the getTargetOption abstraction doesn't necessarily mean that the targetOption can be saved as a variable in each test and invoked multiple times. We could then call the getOptionSearchString only where it's actually necessary to get a search string.
Anyway, not a big deal — we can merge this PR with or without the suggested changes
There was a problem hiding this comment.
Ah, brilliant. My concerns about repetition were more based on your suggestion of possible having no utility function at all. I really do like your proposed getOptionSearchString() approach, it's a lot cleaner than my previous implementation.
Adding your changes to the PR now, which should be the final piece of the puzzle for this one. Thank you @ciampo!
chad1008
force-pushed
the
add/ComboboxControl-unit-tests
branch
from
3a1f950
to
c63c4b2
Compare
chad1008
force-pushed
the
add/ComboboxControl-unit-tests
branch
from
f414529
to
e7bec12
Compare
|
Thanks @ciampo. The changes recommended in that comment are now included in the PR 🙂. Once tests pass I'll get this one merged. Thanks again for all of the feedback! |
chad1008
force-pushed
the
add/ComboboxControl-unit-tests
branch
from
e7bec12
to
4edad41
Compare
cbravobernal
added
[Package] Components
What?
Add unit tests to the ComboboxControl component
Why?
Following up on the conversation in #41417 where it was noted this component currently lacks any tests, making it harder to update it with confidence.
How?
Five tests are currently implemented:
hideLabelFromVisionprop istrue)I did plan to add an additional test to validate the correct option is selected via keypresses, but ran into trouble implementing it. I wasn't able to get
userEffect.keyboard()to registerdownarrowkey presses, nor could I get it to simulate hittingenter. I was able to simulate typing in part of the option's name, but withoutenterto confirm the selection, the test wasn't viable.I'm not 100% sure why that is, but I did find that for some elements, certain keypresses (like arrowdown) aren't fully implemented. This may be related?
Testing Instructions
Run
npm run test-unit packages/components/src/combobox-controlScreenshots or screencast