Skip to content
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.

Sign up for GitHub

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

Jump to bottom

CustomSelectControl: add additional unit tests #56575

Merged
merged 13 commits into from
Dec 11, 2023
Merged

CustomSelectControl: add additional unit tests #56575

Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8be9700
`CustomSelectControl`: add additional unit tests
brookewp Nov 27, 2023
ec0847f
Update tests based on PR feedback
brookewp Dec 2, 2023
5306bf8
Update naming
brookewp Dec 5, 2023
de49c49
Add an extra selection to bolster test
brookewp Dec 5, 2023
2b33a3b
Test both uncontrolled and controlled
brookewp Dec 5, 2023
37b3cff
Add test for custom event handlers
brookewp Dec 5, 2023
2e1da1e
Add alliterative options for testing character matches
brookewp Dec 5, 2023
948a131
Add tests for styles and classes
brookewp Dec 7, 2023
f95022a
Make tests more dynamic
brookewp Dec 7, 2023
8db564a
Adjust specificity of button queries to check for expanded value
brookewp Dec 7, 2023
220e01f
Add label and adjust listbox queries
brookewp Dec 7, 2023
cc7f225
Add additional assertions for focus
brookewp Dec 7, 2023
33cdc33
Update test to include all items from options
brookewp Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
334 changes: 299 additions & 35 deletions packages/components/src/custom-select-control/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,54 +4,173 @@
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import CustomSelectControl from '..';

describe( 'CustomSelectControl', () => {
it( 'Captures the keypress event and does not let it propagate', async () => {
const user = userEvent.setup();
const onKeyDown = jest.fn();
const options = [
{
key: 'one',
name: 'Option one',
},
{
key: 'two',
name: 'Option two',
},
{
key: 'three',
name: 'Option three',
const props = {
options: [
{
key: 'flower1',
name: 'violets',
},
{
key: 'flower2',
name: 'crimson clover',
},
{
key: 'flower3',
name: 'poppy',
},
{
key: 'color1',
name: 'amber',
className: 'amber-skies',
},
{
key: 'color2',
name: 'aquamarine',
style: {
backgroundColor: 'rgb(127, 255, 212)',
rotate: '13deg',
},
];
},
brookewp marked this conversation as resolved.
Show resolved Hide resolved
],
__nextUnconstrainedWidth: true,
};

render(
<div
// This role="none" is required to prevent an eslint warning about accessibility.
role="none"
onKeyDown={ onKeyDown }
>
<CustomSelectControl
options={ options }
__nextUnconstrainedWidth
/>
</div>
const ControlledCustomSelectControl = ( { options } ) => {
const [ value, setValue ] = useState( options[ 0 ] );
return (
<CustomSelectControl
{ ...props }
onChange={ ( { selectedItem } ) => setValue( selectedItem ) }
value={ options.find( ( option ) => option.key === value.key ) }
/>
);
};

describe.each( [
[ 'uncontrolled', CustomSelectControl ],
[ 'controlled', ControlledCustomSelectControl ],
] )( 'CustomSelectControl %s', ( ...modeAndComponent ) => {
const [ , Component ] = modeAndComponent;

it( 'Should replace the initial selection when a new item is selected', async () => {
const user = userEvent.setup();

render( <Component { ...props } /> );

const currentSelectedItem = screen.getByRole( 'button' );
brookewp marked this conversation as resolved.
Show resolved Hide resolved

expect( currentSelectedItem ).toHaveTextContent( 'violets' );
Copy link
Member

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

Copy link
Contributor Author

@brookewp brookewp Dec 7, 2023 •
edited
Loading

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',
} )

Copy link
Contributor Author

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?

Suggested change
expect( currentSelectedItem ).toHaveTextContent( 'violets' );
expect( currentSelectedItem ).toBeVisible();

Copy link
Member

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.


await user.click( currentSelectedItem );

await user.click(
screen.getByRole( 'option', {
name: 'crimson clover',
} )
);

expect( currentSelectedItem ).toHaveTextContent( 'crimson clover' );

await user.click( currentSelectedItem );

await user.click(
screen.getByRole( 'option', {
name: 'poppy',
} )
);

expect( currentSelectedItem ).toHaveTextContent( 'poppy' );
} );

it( 'Should keep current selection if dropdown is closed without changing selection', async () => {
const user = userEvent.setup();

render( <CustomSelectControl { ...props } /> );

const currentSelectedItem = screen.getByRole( 'button' );
expect( currentSelectedItem ).toHaveTextContent( 'violets' );

await user.tab();
await user.keyboard( '{enter}' );
expect( screen.getByRole( 'listbox' ) ).toBeVisible();
expect( screen.getByRole( 'listbox' ) ).toHaveAttribute(
'aria-hidden',
'false'
);
brookewp marked this conversation as resolved.
Show resolved Hide resolved

await user.keyboard( '{escape}' );
expect( screen.queryByRole( 'listbox' ) ).not.toBeInTheDocument();

expect( currentSelectedItem ).toHaveTextContent( 'violets' );
} );

it( 'Should apply class only to options that have a className defined', async () => {
const user = userEvent.setup();
const customClass = 'amber-skies';

render( <CustomSelectControl { ...props } /> );

await user.click( screen.getByRole( 'button', { text: 'violets' } ) );

// return an array of items without a className added
const classlessItems = props.options.filter(
( option ) => option.className === undefined
);
const toggleButton = screen.getByRole( 'button' );
await user.click( toggleButton );

const customSelect = screen.getByRole( 'listbox' );
await user.type( customSelect, '{enter}' );
// assert against filtered array
classlessItems.map( ( { name } ) =>
expect( screen.getByRole( 'option', { name } ) ).not.toHaveClass(
customClass
)
);

expect( onKeyDown ).toHaveBeenCalledTimes( 0 );
expect( screen.getByRole( 'option', { name: 'amber' } ) ).toHaveClass(
brookewp marked this conversation as resolved.
Show resolved Hide resolved
customClass
);
} );

it( 'Should apply styles only to options that have styles defined', async () => {
const user = userEvent.setup();
const customStyles =
'background-color: rgb(127, 255, 212); rotate: 13deg;';

render( <CustomSelectControl { ...props } /> );

await user.click( screen.getByRole( 'button', { text: 'violets' } ) );

// return an array of items without styles added
const unstyledItems = props.options.filter(
( option ) => option.style === undefined
);

// assert against filtered array
unstyledItems.map( ( { name } ) =>
expect( screen.getByRole( 'option', { name } ) ).not.toHaveStyle(
customStyles
)
);

expect(
screen.getByRole( 'option', {
name: 'aquamarine',
} )
).toHaveStyle( customStyles );
} );

it( 'does not show selected hint by default', () => {
render(
<CustomSelectControl
{ ...props }
label="Custom select"
options={ [
{
Expand All @@ -60,7 +179,6 @@ describe( 'CustomSelectControl', () => {
__experimentalHint: 'Hint',
},
] }
__nextUnconstrainedWidth
/>
);
expect(
Expand All @@ -71,6 +189,7 @@ describe( 'CustomSelectControl', () => {
it( 'shows selected hint when __experimentalShowSelectedHint is set', () => {
render(
<CustomSelectControl
{ ...props }
label="Custom select"
options={ [
{
Expand All @@ -80,11 +199,156 @@ describe( 'CustomSelectControl', () => {
},
] }
__experimentalShowSelectedHint
__nextUnconstrainedWidth
/>
);
expect(
screen.getByRole( 'button', { name: 'Custom select' } )
).toHaveTextContent( 'Hint' );
} );

describe( 'Keyboard behavior and accessibility', () => {
Copy link
Member

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.

Copy link
Contributor Author

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 );

Copy link
Contributor Author

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!

it( 'Captures the keypress event and does not let it propagate', async () => {
const user = userEvent.setup();
const onKeyDown = jest.fn();

render(
<div
// This role="none" is required to prevent an eslint warning about accessibility.
role="none"
onKeyDown={ onKeyDown }
>
<CustomSelectControl { ...props } />
</div>
);
const toggleButton = screen.getByRole( 'button' );
await user.click( toggleButton );

const customSelect = screen.getByRole( 'listbox' );
brookewp marked this conversation as resolved.
Show resolved Hide resolved
await user.type( customSelect, '{enter}' );

expect( onKeyDown ).toHaveBeenCalledTimes( 0 );
} );

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();

await user.keyboard( '{enter}' );
await user.keyboard( '{arrowdown}' );
await user.keyboard( '{enter}' );

expect( currentSelectedItem ).toHaveTextContent( 'crimson clover' );
} );

it( 'Should be able to type characters to select matching options', async () => {
const user = userEvent.setup();

render( <CustomSelectControl { ...props } /> );

const currentSelectedItem = screen.getByRole( 'button' );
expect( currentSelectedItem ).toHaveTextContent( 'violets' );

await user.tab();
await user.keyboard( '{enter}' );
await user.keyboard( '{a}' );
await user.keyboard( '{enter}' );

expect( currentSelectedItem ).toHaveTextContent( 'amber' );
} );

it( 'Can change selection with a focused input and closed dropdown if typed characters match an option', async () => {
const user = userEvent.setup();

render( <CustomSelectControl { ...props } /> );

const currentSelectedItem = screen.getByRole( 'button' );
expect( currentSelectedItem ).toHaveTextContent( 'violets' );

await user.tab();
await user.keyboard( '{a}' );
await user.keyboard( '{q}' );
await user.keyboard( '{enter}' );

expect( currentSelectedItem ).toHaveTextContent( 'aquamarine' );
} );

it( 'Should have correct aria-selected value for selections', async () => {
const user = userEvent.setup();

render( <CustomSelectControl { ...props } /> );

const currentSelectedItem = screen.getByRole( 'button' );

await user.click( currentSelectedItem );

expect(
screen.getByRole( 'option', {
name: 'poppy',
selected: false,
} )
).toBeVisible();
expect(
screen.getByRole( 'option', {
name: 'crimson clover',
selected: false,
} )
).toBeVisible();

expect(
screen.getByRole( 'option', {
name: 'violets',
selected: true,
} )
).toBeVisible();

await user.click( screen.getByRole( 'option', { name: 'poppy' } ) );
await user.click( currentSelectedItem );
expect(
screen.getByRole( 'option', {
name: 'violets',
selected: false,
} )
).toBeVisible();
expect(
screen.getByRole( 'option', {
name: 'poppy',
selected: true,
} )
).toBeVisible();
} );

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 );
} );
} );
} );
Copy link
Member

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

Copy link
Contributor Author

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. 😕

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 👍

Copy link
Contributor Author

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.

Copy link
Member

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.

Loading