-
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
CustomSelectControl
: add additional unit tests
#56575
Conversation
Size Change: +6.16 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
Marking this as ready for review to see if anyone has ideas on other tests that might be useful here. |
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.
Nice work @brookewp 👍
I've left some questions and suggestions, everything is optional and opinionated obviously. Let me know what you think!
await user.click( newValue ); | ||
|
||
expect( currentValue ).toHaveTextContent( /poppy/i ); | ||
expect( mock ).toHaveBeenCalled(); |
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.
Isn't this testing an implementation detail? Isn't it sufficient to check the text content, and what I'd add, the value
of the selected item?
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.
Checking that the onChange
callback has been called correctly (or hasn't been called) has proven useful in my experience, since onChange
is part of the component's public APIs and it can cause regressions when the component is used in controlled mode
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.
Which leads me to a suggestion — we should be probably checking for onChange
to have been called (or not) after every interaction. toHaveBeenCalledTimes( n )
and toHaveLastBeenCalledWith( x )
can be helpful in this case.
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.
I'm not sure how we can test for the onChange
mock now that we have controlled and uncontrolled tests. So it's been removed for now. If there is a way and it's beneficial still, I might need a bit of help there.
name: /violets/i, | ||
} ) | ||
).toHaveAttribute( 'aria-selected', 'true' ); | ||
} ); | ||
} ); |
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.
Maybe it could be nice to see some tests or assertions for:
- Testing the selected value in addition to the selected item text content
- Deselecting, and/or selecting an item without a value.
- Selecting out of multiple options that match a query
- Selecting an option after another option has been selected previously
- Multiple selection?
- Additional item styles
- All additional props that weren't tested/covered here, including the event handler ones
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.
Thank you for the suggestions! A couple comments/questions in regards to them:
Multiple selection?
Not possible currently, but its in the experimental version 😄 so hopefully soon
Additional item styles
Are you referring to adding styles and classnames to options? Would that just be to ensure that it isn't applied to all items but specific ones? Just wondering how to test it without it being solely about implementation details. 😕
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.
Yes! I don't think it's going to be about testing implementation details - it's the opposite, we would be testing the API, and how we can apply specific styles to separate items.
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.
Thanks! I've added a couple tests for this in: 948a131
Hopefully, I've captured what you suggested, otherwise I'd be curious for your thoughts/feedback.
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.
Thanks!
As mentioned above, it might make sense to add additional assertions for testing the selected value in addition to the selected option text content. Also, what happens if we select an item with an empty value (similar to having an empty string as value
for an option
element in HTML)?
I think you've covered the rest well enough already 👍
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.
As mentioned above, it might make sense to add additional assertions for testing the selected value in addition to the selected option text content.
Are you referring to adding commented-out assertions that will fail, as suggested earlier? Would that help clarify why those assertions aren't there in the first place?
I'm curious as I was leaning toward the other suggestion - the reason being I think it's likely other things will come up in the new component that we may not have anticipated testing for here. As we bolster the tests in the new component, we may have additional tests/assertions to bring back to this one.
Also, what happens if we select an item with an empty value (similar to having an empty string as value for an option element in HTML)?
Since there isn't a value DOM attribute (just the prop for controlling the component), do you mean adding an empty string to options.name
? Or something else? An empty string is valid for that and also for value
in Ariakit.
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.
As a second consideration, I'm happy with the existing tests. Let's move forward and ship what we have, and we can always add another case if needed.
expect( currentValue ).toHaveTextContent( /crimson clover/i ); | ||
} ); | ||
|
||
it( 'Current selection has attribute: aria-selected="true"', async () => { |
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.
One way to make this test more valuable is to:
- make sure that the options that are not supposed to be selected have the
aria-selected="false"
attribute - add some user interaction to select another option, and make sure that the
aria-selected
attributes update accordingly (this may fail if what I noticed in https://github.com/WordPress/gutenberg/pull/56575/files#r1411951559 is true)
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.
I noticed this when I looked in dev tools, it didn't appear to work correctly, so I was surprised when the test passed.
Adding your suggestions to the test doesn't cause it to fail, but since the options are unmounted when the menu is closed, it has to be opened again to verify this. 😅
Flaky tests detected in 33cdc33. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7134942190
|
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.
Nice work! I've left some more feedback. I wouldn't say it's blocking, but rather, take whatever you prefer. With tests, it can always be better coverage and more cases, and we have to draw the line somewhere.
|
||
const currentSelectedItem = screen.getByRole( 'button' ); | ||
|
||
expect( currentSelectedItem ).toHaveTextContent( 'violets' ); |
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.
Noting that if we reorder the fixture above, the test will break. Could make sense to access props.options[ 0 ].name
here
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.
Edit: I was thinking about the specificity from your previous comment rather than the ordering. I think we can use what you suggested above instead. Since the purpose of the query is to get the button element, not necessarily the first in the list. So my original comment below can be disregarded.
What do you think of specifying the text instead?
screen.getByRole( 'button', {
text: 'violets',
} )
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.
Then we could check for toBeVisible
instead?
expect( currentSelectedItem ).toHaveTextContent( 'violets' ); | |
expect( currentSelectedItem ).toBeVisible(); |
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.
Either way sounds good to me. Leaving it to your judgment, as it's a minor detail.
/> | ||
); | ||
expect( | ||
screen.getByRole( 'button', { name: 'Custom select' } ) | ||
).toHaveTextContent( 'Hint' ); | ||
} ); | ||
|
||
describe( 'Keyboard behavior and accessibility', () => { |
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.
Do we also need focus tests / assertions? Making sure that as we navigate with keyboard, the focused item is the one that we expect it to be.
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.
We have a few. When testing changing the selection via keyboard:
gutenberg/packages/components/src/custom-select-control/test/index.js
Lines 232 to 241 in 948a131
it( 'Should be able to change selection using keyboard', async () => { | |
const user = userEvent.setup(); | |
render( <CustomSelectControl { ...props } /> ); | |
const currentSelectedItem = screen.getByRole( 'button' ); | |
expect( currentSelectedItem ).toHaveTextContent( 'violets' ); | |
await user.tab(); | |
expect( currentSelectedItem ).toHaveFocus(); |
And when checking that the onFocus
handler is called:
gutenberg/packages/components/src/custom-select-control/test/index.js
Lines 327 to 351 in 948a131
it( 'Should call custom event handlers', async () => { | |
const user = userEvent.setup(); | |
const onFocusMock = jest.fn(); | |
const onBlurMock = jest.fn(); | |
render( | |
<CustomSelectControl | |
{ ...props } | |
onFocus={ onFocusMock } | |
onBlur={ onBlurMock } | |
/> | |
); | |
const currentSelectedItem = screen.getByRole( 'button', { | |
text: 'violets', | |
} ); | |
await user.tab(); | |
expect( currentSelectedItem ).toHaveFocus(); | |
expect( onFocusMock ).toHaveBeenCalledTimes( 1 ); | |
await user.tab(); | |
expect( currentSelectedItem ).not.toHaveFocus(); | |
expect( onBlurMock ).toHaveBeenCalledTimes( 1 ); |
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.
I added a few more in: cc7f225
Let me know if you think there should be any others!
name: /violets/i, | ||
} ) | ||
).toHaveAttribute( 'aria-selected', 'true' ); | ||
} ); | ||
} ); |
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.
Thanks!
As mentioned above, it might make sense to add additional assertions for testing the selected value in addition to the selected option text content. Also, what happens if we select an item with an empty value (similar to having an empty string as value
for an option
element in HTML)?
I think you've covered the rest well enough already 👍
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.
Great work, thank you @brookewp 🙌
What?
Part of #55023
While looking into improving
CustomSelectControl
, the first stop is to refine the current tests and add additional ones.Why?
Since
CustomSelectControl
isn't a nativeselect
element, it seems important to add tests that ensure it behaves the same as one.How?
The additional tests are based on both
listbox
andcombobox
roles, the two appropriate roles for aselect
.Notes
After making changes to the component, the following tests/improvements could be implemented:
toHaveTextContent
withtoHaveValue
select
can be added (i.e.disabled
)Testing Instructions
Ensure tests pass:
npm run test:unit packages/components/src/custom-select-control/test/index.js