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

Unit Tests: rewrite ReactDOM.render usages to RTL #45453

Merged
merged 6 commits into from Nov 3, 2022
Merged

Unit Tests: rewrite ReactDOM.render usages to RTL #45453

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e45cdb
useMergeRefs: rewrite unit tests to Testing Library
jsnajdr Oct 28, 2022
e98cba3
PluginPrePublishPanel: rewrite unit tests to Testing Library
jsnajdr Oct 28, 2022
e2784ba
Replace rerender(null) with unmount()
jsnajdr Nov 2, 2022
05e9789
Use screen.getByRole
jsnajdr Nov 2, 2022
49ff38f
Check panel rendering by looking up the title element, avoid snapshots
jsnajdr Nov 2, 2022
3b5e2a0
use .toBeVisible()
jsnajdr Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
97 changes: 35 additions & 62 deletions packages/compose/src/hooks/use-merge-refs/test/index.js
View file
Open in desktop
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import ReactDOM from 'react-dom';
import { render } from '@testing-library/react';

/**
* WordPress dependencies
Expand Down Expand Up @@ -71,31 +71,22 @@ describe( 'useMergeRefs', () => {
return <TagName ref={ mergedRefs } />;
}

beforeEach( () => {
const rootElement = document.createElement( 'div' );
rootElement.id = 'root';
document.body.appendChild( rootElement );
} );

afterEach( () => {
// Reset all history and DOM.
// Reset all history.
renderCallback.history = [];
document.body.innerHTML = '';
} );

it( 'should work', () => {
const rootElement = document.getElementById( 'root' );

ReactDOM.render( <MergedRefs />, rootElement );
const { container, rerender } = render( <MergedRefs /> );

const originalElement = rootElement.firstElementChild;
const originalElement = container.firstElementChild;
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind altering the default tagName in MergedRefs above to be ul for example? That way instead of creating new ESLint violations for no-node-access, we'll be able to use screen.getByRole( 'list' )?

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's a neat trick, I used it 🙂 getByRole also ensures that the element actually exists. That was one of my worries when migrating the test: is firstElementChild really an element, or is it undefined?


// Render 1: both initial callback functions should be called with node.
expect( renderCallback.history ).toEqual( [
[ [ originalElement ], [ originalElement ] ],
] );

ReactDOM.render( <MergedRefs />, rootElement );
rerender( <MergedRefs /> );

// Render 2: the new callback functions should not be called! There has
// been no dependency change.
Expand All @@ -104,7 +95,7 @@ describe( 'useMergeRefs', () => {
[ [], [] ],
] );

ReactDOM.render( null, rootElement );
rerender( null );
Copy link
Member

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 unmount from the result of render() above. It expresses our intentions more explicitly. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

unmount() works, changed rerender( null ) instances to that 👍


// Unmount: the initial callback functions should receive null.
expect( renderCallback.history ).toEqual( [
Expand All @@ -117,15 +108,13 @@ describe( 'useMergeRefs', () => {
} );

it( 'should work for node change', () => {
const rootElement = document.getElementById( 'root' );
const { container, rerender } = render( <MergedRefs /> );

ReactDOM.render( <MergedRefs />, rootElement );
const originalElement = container.firstElementChild;

const originalElement = rootElement.firstElementChild;
rerender( <MergedRefs tagName="button" /> );

ReactDOM.render( <MergedRefs tagName="button" />, rootElement );

const newElement = rootElement.firstElementChild;
const newElement = container.firstElementChild;

// After a render with the original element and a second render with the
// new element, expect the initial callback functions to be called with
Expand All @@ -140,7 +129,7 @@ describe( 'useMergeRefs', () => {
[ [], [] ],
] );

ReactDOM.render( null, rootElement );
rerender( null );

// Unmount: the initial callback functions should receive null.
expect( renderCallback.history ).toEqual( [
Expand All @@ -153,17 +142,15 @@ describe( 'useMergeRefs', () => {
} );

it( 'should work with dependencies', () => {
const rootElement = document.getElementById( 'root' );

ReactDOM.render( <MergedRefs count={ 1 } />, rootElement );
const { container, rerender } = render( <MergedRefs count={ 1 } /> );

const originalElement = rootElement.firstElementChild;
const originalElement = container.firstElementChild;

expect( renderCallback.history ).toEqual( [
[ [ originalElement ], [ originalElement ] ],
] );

ReactDOM.render( <MergedRefs count={ 2 } />, rootElement );
rerender( <MergedRefs count={ 2 } /> );

// After a second render with a dependency change, expect the inital
// callback function to be called with null and the new callback
Expand All @@ -174,7 +161,7 @@ describe( 'useMergeRefs', () => {
[ [], [ originalElement ] ],
] );

ReactDOM.render( null, rootElement );
rerender( null );

// Unmount: current callback functions should be called with null.
expect( renderCallback.history ).toEqual( [
Expand All @@ -187,22 +174,17 @@ describe( 'useMergeRefs', () => {
} );

it( 'should simultaneously update node and dependencies', () => {
const rootElement = document.getElementById( 'root' );
const { container, rerender } = render( <MergedRefs count={ 1 } /> );

ReactDOM.render( <MergedRefs count={ 1 } />, rootElement );

const originalElement = rootElement.firstElementChild;
const originalElement = container.firstElementChild;

expect( renderCallback.history ).toEqual( [
[ [ originalElement ], [ originalElement ] ],
] );

ReactDOM.render(
<MergedRefs count={ 2 } tagName="button" />,
rootElement
);
rerender( <MergedRefs count={ 2 } tagName="button" /> );

const newElement = rootElement.firstElementChild;
const newElement = container.firstElementChild;

// Both the node changes and the dependencies update for the second
// callback, so expect the old callback function to be called with null
Expand All @@ -217,7 +199,7 @@ describe( 'useMergeRefs', () => {
[ [], [ newElement ] ],
] );

ReactDOM.render( null, rootElement );
rerender( null );

// Unmount: current callback functions should be called with null.
expect( renderCallback.history ).toEqual( [
Expand All @@ -230,15 +212,13 @@ describe( 'useMergeRefs', () => {
} );

it( 'should work for dependency change after node change', () => {
const rootElement = document.getElementById( 'root' );

ReactDOM.render( <MergedRefs />, rootElement );
const { container, rerender } = render( <MergedRefs /> );

const originalElement = rootElement.firstElementChild;
const originalElement = container.firstElementChild;

ReactDOM.render( <MergedRefs tagName="button" />, rootElement );
rerender( <MergedRefs tagName="button" /> );

const newElement = rootElement.firstElementChild;
const newElement = container.firstElementChild;

// After a render with the original element and a second render with the
// new element, expect the initial callback functions to be called with
Expand All @@ -253,10 +233,7 @@ describe( 'useMergeRefs', () => {
[ [], [] ],
] );

ReactDOM.render(
<MergedRefs tagName="button" count={ 1 } />,
rootElement
);
rerender( <MergedRefs tagName="button" count={ 1 } /> );

// After a third render with a dependency change, expect the inital
// callback function to be called with null and the new callback
Expand All @@ -271,7 +248,7 @@ describe( 'useMergeRefs', () => {
[ [], [ newElement ] ],
] );

ReactDOM.render( null, rootElement );
rerender( null );

// Unmount: current callback functions should be called with null.
expect( renderCallback.history ).toEqual( [
Expand All @@ -285,18 +262,16 @@ describe( 'useMergeRefs', () => {
} );

it( 'should allow disabling a ref', () => {
const rootElement = document.getElementById( 'root' );
const { container, rerender } = render( <MergedRefs disable1 /> );

ReactDOM.render( <MergedRefs disable1 />, rootElement );

const originalElement = rootElement.firstElementChild;
const originalElement = container.firstElementChild;

// Render 1: ref 1 should be disabled.
expect( renderCallback.history ).toEqual( [
[ [], [ originalElement ] ],
] );

ReactDOM.render( <MergedRefs disable2 />, rootElement );
rerender( <MergedRefs disable2 /> );

// Render 2: ref 1 should be enabled and receive the ref. Note that the
// callback hasn't changed, so the original callback function will be
Expand All @@ -306,7 +281,7 @@ describe( 'useMergeRefs', () => {
[ [], [] ],
] );

ReactDOM.render( <MergedRefs disable1 count={ 1 } />, rootElement );
rerender( <MergedRefs disable1 count={ 1 } /> );

// Render 3: ref 1 should again be disabled. Ref 2 to should receive a
// ref with the new callback function because the count has been
Expand All @@ -320,7 +295,7 @@ describe( 'useMergeRefs', () => {
[ [], [ originalElement ] ],
] );

ReactDOM.render( null, rootElement );
rerender( null );

// Unmount: current callback functions should receive null.
expect( renderCallback.history ).toEqual( [
Expand All @@ -334,26 +309,24 @@ describe( 'useMergeRefs', () => {
} );

it( 'should allow the hook being unused', () => {
const rootElement = document.getElementById( 'root' );

ReactDOM.render( <MergedRefs unused />, rootElement );
const { container, rerender } = render( <MergedRefs unused /> );

const originalElement = rootElement.firstElementChild;
const originalElement = container.firstElementChild;

// Render 1: ref 1 should updated, ref 2 should not.
expect( renderCallback.history ).toEqual( [
[ [ originalElement ], [] ],
] );

ReactDOM.render( <MergedRefs />, rootElement );
rerender( <MergedRefs /> );

// Render 2: ref 2 should be updated as well.
expect( renderCallback.history ).toEqual( [
[ [ originalElement, null, originalElement ], [ originalElement ] ],
[ [], [] ],
] );

ReactDOM.render( <MergedRefs unused />, rootElement );
rerender( <MergedRefs unused /> );

// Render 3: ref 2 should be updated with null
expect( renderCallback.history ).toEqual( [
Expand Down
16 changes: 9 additions & 7 deletions packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js
View file
Open in desktop
@@ -1,8 +1,12 @@
/**
* External dependencies
*/
import { render } from '@testing-library/react';

/**
* WordPress dependencies
*/
import { SlotFillProvider } from '@wordpress/components';
import { render } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -11,21 +15,19 @@ import PluginPrePublishPanel from '../';

describe( 'PluginPrePublishPanel', () => {
test( 'renders fill properly', () => {
const div = document.createElement( 'div' );
render(
const { container } = render(
<SlotFillProvider>
<PluginPrePublishPanel
className="my-plugin-pre-publish-panel"
title="My panel title"
initialOpen={ true }
initialOpen
>
My panel content
</PluginPrePublishPanel>
<PluginPrePublishPanel.Slot />
</SlotFillProvider>,
div
</SlotFillProvider>
);

expect( div.innerHTML ).toMatchSnapshot();
expect( container.innerHTML ).toMatchSnapshot();
Copy link
Member

Choose a reason for hiding this comment

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

Mind going with container instead? We'll need to update the snapshot, but at least we'll get rid of a no-node-access violation.

Suggested change
expect( container.innerHTML ).toMatchSnapshot();
expect( container ).toMatchSnapshot();

Copy link
Member Author

@jsnajdr jsnajdr Nov 2, 2022 •
edited

Choose a reason for hiding this comment

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

As the purpose of the test is to check whether the PluginPrePublishPanel was rendered at all, by filling in a slot, I modified the assertion to check for the panel title element (with getByText). Checking the entire markup with a snapshot is redundant, so I removed the snapshot completely.

} );
} );