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
14 changes: 7 additions & 7 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 @@ -21,7 +21,7 @@ describe( 'Icon', () => {
it( 'renders nothing when icon omitted', () => {
render( <Icon data-testid={ testId } /> );

expect( screen.queryByTestId( testId ) ).not.toBeInTheDocument();
expect( screen.queryByTestId( testId ) ).toBeNull();
Copy link
Contributor Author

@t-hamano t-hamano 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.

Do you think this additional fix is reasonable?
(.not.toBeVisible() didn't work)

Copy link
Member

Choose a reason for hiding this comment

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

No, I think .not.toBeInTheDocument() is ideally expressing our goal here; .toBeNull() makes the assertion vague and unclear, while what we have already is pretty transparent IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

.not.toBeVisible() won't work by design, an element can't be visible if it doesn't exist in the DOM tree, that's why we check with .not.toBeInTheDocument().

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 see!
I have reverted this change and would like to merge it once the test passes.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @t-hamano 🙌

} );

it( 'renders a dashicon by slug', () => {
Expand All @@ -35,24 +35,24 @@ describe( 'Icon', () => {
it( 'renders a function', () => {
render( <Icon icon={ () => <span data-testid={ testId } /> } /> );

expect( screen.getByTestId( testId ) ).toBeInTheDocument();
expect( screen.getByTestId( testId ) ).toBeVisible();
} );

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

expect( screen.getByTestId( testId ) ).toBeInTheDocument();
expect( screen.getByTestId( testId ) ).toBeVisible();
} );

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

expect( screen.getByTestId( testId ) ).toBeInTheDocument();
expect( screen.getByTestId( testId ) ).toBeVisible();
} );

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

expect( icon ).toHaveAttribute( 'width', '24' );
expect( icon ).toHaveAttribute( 'height', '24' );
Expand All @@ -70,15 +70,15 @@ describe( 'Icon', () => {
size={ 32 }
/>
);
const icon = screen.queryByTestId( testId );
const icon = screen.getByTestId( testId );

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', () => {
render( <Icon data-testid={ testId } icon={ svg } size={ 32 } /> );
const icon = screen.queryByTestId( testId );
const icon = screen.getByTestId( testId );

expect( icon ).toHaveAttribute( 'width', '32' );
expect( icon ).toHaveAttribute( 'height', '32' );
Expand Down