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

Components: Refactor Icon tests to @testing-library/react #44051

Merged
merged 6 commits into from
Sep 12, 2022
Merged

Components: Refactor Icon tests to @testing-library/react #44051

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fa8a107
Components: Refactor `Icon` tests to `@testing-library/react`
t-hamano Sep 10, 2022
03efa9f
Merge branch 'trunk' into refactor/icon-test-to-rtl
t-hamano Sep 12, 2022
f2b162f
Delete test for `props`
t-hamano Sep 12, 2022
5f77e01
Update chagelog
t-hamano Sep 12, 2022
385c25a
Matcher and query more streamlined
t-hamano Sep 12, 2022
9d47343
Revert unnecessary update
t-hamano Sep 12, 2022
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
82 changes: 40 additions & 42 deletions packages/components/src/icon/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@
* External dependencies
*/
import { shallow } from 'enzyme';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { render, screen } from '@testing-library/react';

/**
* Internal dependencies
Expand All @@ -16,6 +12,7 @@ import Icon from '../';
import { Path, SVG } from '../../';

describe( 'Icon', () => {
const testId = 'icon';
const className = 'example-class';
const svg = (
<SVG>
Expand All @@ -25,47 +22,49 @@ describe( 'Icon', () => {
const style = { fill: 'red' };

it( 'renders nothing when icon omitted', () => {
const wrapper = shallow( <Icon /> );
render( <Icon data-testid={ testId } /> );

expect( wrapper.type() ).toBeNull();
expect( screen.queryByTestId( testId ) ).not.toBeInTheDocument();
} );

it( 'renders a dashicon by slug', () => {
const wrapper = shallow( <Icon icon="format-image" /> );
render( <Icon data-testid={ testId } icon="format-image" /> );

expect( wrapper.find( 'Dashicon' ).prop( 'icon' ) ).toBe(
'format-image'
expect( screen.getByTestId( testId ) ).toHaveClass(
'dashicons-format-image'
);
} );

it( 'renders a function', () => {
const wrapper = shallow( <Icon icon={ () => <span /> } /> );
render( <Icon icon={ () => <span data-testid={ testId } /> } /> );

expect( wrapper.name() ).toBe( 'span' );
expect( screen.getByTestId( testId ) ).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to use .toBeVisible() which is a bit more specific than .toBeInTheDocument()?

} );

it( 'renders an element', () => {
const wrapper = shallow( <Icon icon={ <span /> } /> );
render( <Icon icon={ <span data-testid={ testId } /> } /> );

expect( wrapper.name() ).toBe( 'span' );
expect( screen.getByTestId( testId ) ).toBeInTheDocument();
} );

it( 'renders an svg element', () => {
const wrapper = shallow( <Icon icon={ svg } /> );
render( <Icon data-testid={ testId } icon={ svg } /> );

expect( wrapper.name() ).toBe( 'SVG' );
expect( screen.getByTestId( testId ) ).toBeInTheDocument();
} );

it( 'renders an svg element with a default width and height of 24', () => {
const wrapper = shallow( <Icon icon={ svg } /> );
render( <Icon data-testid={ testId } icon={ svg } /> );
const icon = screen.queryByTestId( testId );
Copy link
Member

@tyxla tyxla Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to use get* instead of query* here, see this for the rationale.

Suggested change
const icon = screen.queryByTestId( testId );
const icon = screen.getByTestId( testId );


expect( wrapper.prop( 'width' ) ).toBe( 24 );
expect( wrapper.prop( 'height' ) ).toBe( 24 );
expect( icon ).toHaveAttribute( 'width', '24' );
expect( icon ).toHaveAttribute( 'height', '24' );
} );

it( 'renders an svg element and override its width and height', () => {
const wrapper = shallow(
render(
<Icon
data-testid={ testId }
icon={
<SVG width={ 64 } height={ 64 }>
<Path d="M5 4v3h5.5v12h3V7H19V4z" />
Expand All @@ -74,35 +73,33 @@ describe( 'Icon', () => {
size={ 32 }
/>
);
const icon = screen.queryByTestId( testId );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const icon = screen.queryByTestId( testId );
const icon = screen.getByTestId( testId );


expect( wrapper.prop( 'width' ) ).toBe( 32 );
expect( wrapper.prop( 'height' ) ).toBe( 32 );
expect( icon ).toHaveAttribute( 'width', '32' );
expect( icon ).toHaveAttribute( 'height', '32' );
} );

it( 'renders an svg element and does not override width and height if already specified', () => {
const wrapper = shallow( <Icon icon={ svg } size={ 32 } /> );
render( <Icon data-testid={ testId } icon={ svg } size={ 32 } /> );
const icon = screen.queryByTestId( testId );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const icon = screen.queryByTestId( testId );
const icon = screen.getByTestId( testId );


expect( wrapper.prop( 'width' ) ).toBe( 32 );
expect( wrapper.prop( 'height' ) ).toBe( 32 );
expect( icon ).toHaveAttribute( 'width', '32' );
expect( icon ).toHaveAttribute( 'height', '32' );
} );

it( 'renders a component', () => {
class MyComponent extends Component {
render() {
return <span />;
}
}
const wrapper = shallow( <Icon icon={ MyComponent } /> );

expect( wrapper.name() ).toBe( 'MyComponent' );
const MyComponent = () => (
<span data-testid={ testId } className={ className } />
);
render( <Icon icon={ MyComponent } /> );

expect( screen.getByTestId( testId ) ).toHaveClass( className );
} );

describe( 'props passing', () => {
class MyComponent extends Component {
render() {
return <span className={ this.props.className } />;
}
}
const MyComponent = ( props ) => (
<span className={ props.className } style={ props.style } />
);

describe.each( [
[ 'dashicon', { icon: 'format-image' } ],
Expand All @@ -111,7 +108,7 @@ describe( 'Icon', () => {
[ 'svg element', { icon: svg } ],
[ 'component', { icon: MyComponent } ],
] )( '%s', ( label, props ) => {
it( 'should pass through size', () => {
it.skip( 'should pass through size', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is tentatively skipped because it tests the full implementation and I was not sure how this could be replaced.
If you have a better approach to this test, I would like to hear your advice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can delete this whole describe.each call, I don't think it adds any value to the tests (especially since it used to check the value of the size prop on the wrapper component, and not the prop's effects on its children)

if ( label === 'svg element' ) {
// Custom logic for SVG elements tested separately.
//
Expand All @@ -130,16 +127,17 @@ describe( 'Icon', () => {
} );

it( 'should pass through all other props', () => {
const wrapper = shallow(
const { container } = render(
<Icon
{ ...props }
style={ style }
className={ className }
/>
);
const icon = container.firstChild;

expect( wrapper.prop( 'style' ) ).toBe( style );
expect( wrapper.prop( 'className' ) ).toBe( className );
expect( icon ).toHaveStyle( style );
expect( icon ).toHaveClass( className );
} );
} );
} );
Expand Down