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

ESLint: Fix testing-library/no-render-in-setup violations #45097

Merged
merged 2 commits into from
Oct 19, 2022
Merged

ESLint: Fix testing-library/no-render-in-setup violations #45097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8fb2ce
ESLint: Fix testing-library/no-render-in-setup violations
tyxla Oct 18, 2022
87ac0e4
Use local variables only
tyxla Oct 19, 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
10 changes: 4 additions & 6 deletions packages/components/src/base-field/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,32 +14,30 @@ const TestField = ( props ) => {
};

describe( 'base field', () => {
let base;

beforeEach( () => {
base = render( <TestField /> ).container;
} );

it( 'should render correctly', () => {
const base = render( <TestField /> ).container;
expect( base.firstChild ).toMatchSnapshot();
} );

describe( 'props', () => {
it( 'should render error styles', () => {
const base = render( <TestField /> ).container;
const { container } = render( <TestField hasError /> );
expect( container.firstChild ).toMatchStyleDiffSnapshot(
base.firstChild
);
} );

it( 'should render inline styles', () => {
const base = render( <TestField /> ).container;
const { container } = render( <TestField isInline /> );
expect( container.firstChild ).toMatchStyleDiffSnapshot(
base.firstChild
);
} );

it( 'should render subtle styles', () => {
const base = render( <TestField /> ).container;
const { container } = render( <TestField isSubtle /> );
expect( container.firstChild ).toMatchStyleDiffSnapshot(
base.firstChild
Expand Down
9 changes: 3 additions & 6 deletions packages/components/src/ui/spinner/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,21 @@ import { render } from '@testing-library/react';
import { Spinner } from '..';

describe( 'props', () => {
let base;

beforeEach( () => {
base = render( <Spinner /> );
} );

test( 'should render correctly', () => {
const base = render( <Spinner /> );
expect( base.container.firstChild ).toMatchSnapshot();
} );

test( 'should render color', () => {
const base = render( <Spinner /> );
const { container } = render( <Spinner color="blue" /> );
expect( container.firstChild ).toMatchDiffSnapshot(
base.container.firstChild
);
} );

test( 'should render size', () => {
const base = render( <Spinner /> );
const { container } = render( <Spinner size={ 31 } /> );
expect( container.firstChild ).toMatchDiffSnapshot(
base.container.firstChild
Expand Down
8 changes: 6 additions & 2 deletions packages/components/src/ui/tooltip/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,22 @@ describe( 'props', () => {
const baseTooltipId = 'base-tooltip';
const baseTooltipTriggerContent = 'WordPress.org - Base trigger content';
const byId = ( id ) => ( t ) => t.id === id;
beforeEach( () => {
const renderVisibleTooltip = () => {
render(
<Tooltip baseId={ baseTooltipId } content="Code is Poetry" visible>
<Text>{ baseTooltipTriggerContent }</Text>
</Tooltip>
);
} );
};

test( 'should render correctly', () => {
renderVisibleTooltip();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitty, but I usually prefer to have the render call inlined into each test for better readability (and in case we needed to grab anything from return value of the render call).

Maybe we could extract a VisibleTooltip React component, and then refactor each test to:

render( <VisibleTooltip /> );

@Mamaduka , is that also what you were referring to?

Copy link
Member

Choose a reason for hiding this comment

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

Correct, and I like your suggestion of extracting this into a new component on the test module level.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds like an improvement, I'm happy to follow-up with this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested in #45105. Thanks for the feedback, both 🙌

const tooltip = screen.getByRole( /tooltip/i );
expect( tooltip ).toMatchSnapshot();
} );

test( 'should render invisible', () => {
renderVisibleTooltip();
const invisibleTooltipTriggerContent = 'WordPress.org - Invisible';
render(
<Tooltip
Expand All @@ -50,6 +52,7 @@ describe( 'props', () => {
} );

test( 'should render without children', () => {
renderVisibleTooltip();
const childlessTooltipId = 'tooltip-without-children';
render(
<Tooltip
Expand All @@ -64,6 +67,7 @@ describe( 'props', () => {
} );

test( 'should not render a tooltip without content', () => {
renderVisibleTooltip();
const contentlessTooltipId = 'contentless-tooltip';
render(
<Tooltip baseId={ contentlessTooltipId } visible>
Expand Down