From 0e45cdbd9be8ca39ace49d03b05c9a79fe8a5ad5 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Fri, 28 Oct 2022 19:42:31 +0200 Subject: [PATCH 1/6] useMergeRefs: rewrite unit tests to Testing Library --- .../src/hooks/use-merge-refs/test/index.js | 97 +++++++------------ 1 file changed, 35 insertions(+), 62 deletions(-) diff --git a/packages/compose/src/hooks/use-merge-refs/test/index.js b/packages/compose/src/hooks/use-merge-refs/test/index.js index fe73347b8e237..36801a284b973 100644 --- a/packages/compose/src/hooks/use-merge-refs/test/index.js +++ b/packages/compose/src/hooks/use-merge-refs/test/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import ReactDOM from 'react-dom'; +import { render } from '@testing-library/react'; /** * WordPress dependencies @@ -71,31 +71,22 @@ describe( 'useMergeRefs', () => { return ; } - 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( , rootElement ); + const { container, rerender } = render( ); - const originalElement = rootElement.firstElementChild; + const originalElement = container.firstElementChild; // Render 1: both initial callback functions should be called with node. expect( renderCallback.history ).toEqual( [ [ [ originalElement ], [ originalElement ] ], ] ); - ReactDOM.render( , rootElement ); + rerender( ); // Render 2: the new callback functions should not be called! There has // been no dependency change. @@ -104,7 +95,7 @@ describe( 'useMergeRefs', () => { [ [], [] ], ] ); - ReactDOM.render( null, rootElement ); + rerender( null ); // Unmount: the initial callback functions should receive null. expect( renderCallback.history ).toEqual( [ @@ -117,15 +108,13 @@ describe( 'useMergeRefs', () => { } ); it( 'should work for node change', () => { - const rootElement = document.getElementById( 'root' ); + const { container, rerender } = render( ); - ReactDOM.render( , rootElement ); + const originalElement = container.firstElementChild; - const originalElement = rootElement.firstElementChild; + rerender( ); - ReactDOM.render( , 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 @@ -140,7 +129,7 @@ describe( 'useMergeRefs', () => { [ [], [] ], ] ); - ReactDOM.render( null, rootElement ); + rerender( null ); // Unmount: the initial callback functions should receive null. expect( renderCallback.history ).toEqual( [ @@ -153,17 +142,15 @@ describe( 'useMergeRefs', () => { } ); it( 'should work with dependencies', () => { - const rootElement = document.getElementById( 'root' ); - - ReactDOM.render( , rootElement ); + const { container, rerender } = render( ); - const originalElement = rootElement.firstElementChild; + const originalElement = container.firstElementChild; expect( renderCallback.history ).toEqual( [ [ [ originalElement ], [ originalElement ] ], ] ); - ReactDOM.render( , rootElement ); + rerender( ); // After a second render with a dependency change, expect the inital // callback function to be called with null and the new callback @@ -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( [ @@ -187,22 +174,17 @@ describe( 'useMergeRefs', () => { } ); it( 'should simultaneously update node and dependencies', () => { - const rootElement = document.getElementById( 'root' ); + const { container, rerender } = render( ); - ReactDOM.render( , rootElement ); - - const originalElement = rootElement.firstElementChild; + const originalElement = container.firstElementChild; expect( renderCallback.history ).toEqual( [ [ [ originalElement ], [ originalElement ] ], ] ); - ReactDOM.render( - , - rootElement - ); + rerender( ); - 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 @@ -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( [ @@ -230,15 +212,13 @@ describe( 'useMergeRefs', () => { } ); it( 'should work for dependency change after node change', () => { - const rootElement = document.getElementById( 'root' ); - - ReactDOM.render( , rootElement ); + const { container, rerender } = render( ); - const originalElement = rootElement.firstElementChild; + const originalElement = container.firstElementChild; - ReactDOM.render( , rootElement ); + rerender( ); - 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 @@ -253,10 +233,7 @@ describe( 'useMergeRefs', () => { [ [], [] ], ] ); - ReactDOM.render( - , - rootElement - ); + rerender( ); // After a third render with a dependency change, expect the inital // callback function to be called with null and the new callback @@ -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( [ @@ -285,18 +262,16 @@ describe( 'useMergeRefs', () => { } ); it( 'should allow disabling a ref', () => { - const rootElement = document.getElementById( 'root' ); + const { container, rerender } = render( ); - ReactDOM.render( , rootElement ); - - const originalElement = rootElement.firstElementChild; + const originalElement = container.firstElementChild; // Render 1: ref 1 should be disabled. expect( renderCallback.history ).toEqual( [ [ [], [ originalElement ] ], ] ); - ReactDOM.render( , rootElement ); + rerender( ); // 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 @@ -306,7 +281,7 @@ describe( 'useMergeRefs', () => { [ [], [] ], ] ); - ReactDOM.render( , rootElement ); + rerender( ); // 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 @@ -320,7 +295,7 @@ describe( 'useMergeRefs', () => { [ [], [ originalElement ] ], ] ); - ReactDOM.render( null, rootElement ); + rerender( null ); // Unmount: current callback functions should receive null. expect( renderCallback.history ).toEqual( [ @@ -334,18 +309,16 @@ describe( 'useMergeRefs', () => { } ); it( 'should allow the hook being unused', () => { - const rootElement = document.getElementById( 'root' ); - - ReactDOM.render( , rootElement ); + const { container, rerender } = render( ); - 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( , rootElement ); + rerender( ); // Render 2: ref 2 should be updated as well. expect( renderCallback.history ).toEqual( [ @@ -353,7 +326,7 @@ describe( 'useMergeRefs', () => { [ [], [] ], ] ); - ReactDOM.render( , rootElement ); + rerender( ); // Render 3: ref 2 should be updated with null expect( renderCallback.history ).toEqual( [ From e98cba3eac901d37a0950e94a53edc2acbf66881 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Fri, 28 Oct 2022 19:54:39 +0200 Subject: [PATCH 2/6] PluginPrePublishPanel: rewrite unit tests to Testing Library --- .../plugin-pre-publish-panel/test/index.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js b/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js index 9c011e1cbc2a0..7116bdad0f2de 100644 --- a/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js +++ b/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js @@ -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 @@ -11,21 +15,19 @@ import PluginPrePublishPanel from '../'; describe( 'PluginPrePublishPanel', () => { test( 'renders fill properly', () => { - const div = document.createElement( 'div' ); - render( + const { container } = render( My panel content - , - div + ); - expect( div.innerHTML ).toMatchSnapshot(); + expect( container.innerHTML ).toMatchSnapshot(); } ); } ); From e2784baea74264cbde2138267674437a0ccbf292 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Wed, 2 Nov 2022 07:51:39 +0100 Subject: [PATCH 3/6] Replace rerender(null) with unmount() --- .../src/hooks/use-merge-refs/test/index.js | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/packages/compose/src/hooks/use-merge-refs/test/index.js b/packages/compose/src/hooks/use-merge-refs/test/index.js index 36801a284b973..81512d8cd2341 100644 --- a/packages/compose/src/hooks/use-merge-refs/test/index.js +++ b/packages/compose/src/hooks/use-merge-refs/test/index.js @@ -77,7 +77,7 @@ describe( 'useMergeRefs', () => { } ); it( 'should work', () => { - const { container, rerender } = render( ); + const { container, rerender, unmount } = render( ); const originalElement = container.firstElementChild; @@ -95,7 +95,7 @@ describe( 'useMergeRefs', () => { [ [], [] ], ] ); - rerender( null ); + unmount(); // Unmount: the initial callback functions should receive null. expect( renderCallback.history ).toEqual( [ @@ -108,7 +108,7 @@ describe( 'useMergeRefs', () => { } ); it( 'should work for node change', () => { - const { container, rerender } = render( ); + const { container, rerender, unmount } = render( ); const originalElement = container.firstElementChild; @@ -129,7 +129,7 @@ describe( 'useMergeRefs', () => { [ [], [] ], ] ); - rerender( null ); + unmount(); // Unmount: the initial callback functions should receive null. expect( renderCallback.history ).toEqual( [ @@ -142,7 +142,9 @@ describe( 'useMergeRefs', () => { } ); it( 'should work with dependencies', () => { - const { container, rerender } = render( ); + const { container, rerender, unmount } = render( + + ); const originalElement = container.firstElementChild; @@ -161,7 +163,7 @@ describe( 'useMergeRefs', () => { [ [], [ originalElement ] ], ] ); - rerender( null ); + unmount(); // Unmount: current callback functions should be called with null. expect( renderCallback.history ).toEqual( [ @@ -174,7 +176,9 @@ describe( 'useMergeRefs', () => { } ); it( 'should simultaneously update node and dependencies', () => { - const { container, rerender } = render( ); + const { container, rerender, unmount } = render( + + ); const originalElement = container.firstElementChild; @@ -199,7 +203,7 @@ describe( 'useMergeRefs', () => { [ [], [ newElement ] ], ] ); - rerender( null ); + unmount(); // Unmount: current callback functions should be called with null. expect( renderCallback.history ).toEqual( [ @@ -212,7 +216,7 @@ describe( 'useMergeRefs', () => { } ); it( 'should work for dependency change after node change', () => { - const { container, rerender } = render( ); + const { container, rerender, unmount } = render( ); const originalElement = container.firstElementChild; @@ -248,7 +252,7 @@ describe( 'useMergeRefs', () => { [ [], [ newElement ] ], ] ); - rerender( null ); + unmount(); // Unmount: current callback functions should be called with null. expect( renderCallback.history ).toEqual( [ @@ -262,7 +266,9 @@ describe( 'useMergeRefs', () => { } ); it( 'should allow disabling a ref', () => { - const { container, rerender } = render( ); + const { container, rerender, unmount } = render( + + ); const originalElement = container.firstElementChild; @@ -295,7 +301,7 @@ describe( 'useMergeRefs', () => { [ [], [ originalElement ] ], ] ); - rerender( null ); + unmount(); // Unmount: current callback functions should receive null. expect( renderCallback.history ).toEqual( [ From 05e97891038409cf8ba3961eb11b15269a8e83db Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Wed, 2 Nov 2022 08:03:36 +0100 Subject: [PATCH 4/6] Use screen.getByRole --- .../src/hooks/use-merge-refs/test/index.js | 44 ++++++++----------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/packages/compose/src/hooks/use-merge-refs/test/index.js b/packages/compose/src/hooks/use-merge-refs/test/index.js index 81512d8cd2341..4c883ff97c608 100644 --- a/packages/compose/src/hooks/use-merge-refs/test/index.js +++ b/packages/compose/src/hooks/use-merge-refs/test/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; /** * WordPress dependencies @@ -38,7 +38,7 @@ describe( 'useMergeRefs', () => { function MergedRefs( { count, - tagName: TagName = 'div', + tagName: TagName = 'ul', disable1, disable2, unused, @@ -77,9 +77,9 @@ describe( 'useMergeRefs', () => { } ); it( 'should work', () => { - const { container, rerender, unmount } = render( ); + const { rerender, unmount } = render( ); - const originalElement = container.firstElementChild; + const originalElement = screen.getByRole( 'list' ); // Render 1: both initial callback functions should be called with node. expect( renderCallback.history ).toEqual( [ @@ -108,13 +108,13 @@ describe( 'useMergeRefs', () => { } ); it( 'should work for node change', () => { - const { container, rerender, unmount } = render( ); + const { rerender, unmount } = render( ); - const originalElement = container.firstElementChild; + const originalElement = screen.getByRole( 'list' ); rerender( ); - const newElement = container.firstElementChild; + const newElement = screen.getByRole( 'button' ); // After a render with the original element and a second render with the // new element, expect the initial callback functions to be called with @@ -142,11 +142,9 @@ describe( 'useMergeRefs', () => { } ); it( 'should work with dependencies', () => { - const { container, rerender, unmount } = render( - - ); + const { rerender, unmount } = render( ); - const originalElement = container.firstElementChild; + const originalElement = screen.getByRole( 'list' ); expect( renderCallback.history ).toEqual( [ [ [ originalElement ], [ originalElement ] ], @@ -176,11 +174,9 @@ describe( 'useMergeRefs', () => { } ); it( 'should simultaneously update node and dependencies', () => { - const { container, rerender, unmount } = render( - - ); + const { rerender, unmount } = render( ); - const originalElement = container.firstElementChild; + const originalElement = screen.getByRole( 'list' ); expect( renderCallback.history ).toEqual( [ [ [ originalElement ], [ originalElement ] ], @@ -188,7 +184,7 @@ describe( 'useMergeRefs', () => { rerender( ); - const newElement = container.firstElementChild; + const newElement = screen.getByRole( 'button' ); // Both the node changes and the dependencies update for the second // callback, so expect the old callback function to be called with null @@ -216,13 +212,13 @@ describe( 'useMergeRefs', () => { } ); it( 'should work for dependency change after node change', () => { - const { container, rerender, unmount } = render( ); + const { rerender, unmount } = render( ); - const originalElement = container.firstElementChild; + const originalElement = screen.getByRole( 'list' ); rerender( ); - const newElement = container.firstElementChild; + const newElement = screen.getByRole( 'button' ); // After a render with the original element and a second render with the // new element, expect the initial callback functions to be called with @@ -266,11 +262,9 @@ describe( 'useMergeRefs', () => { } ); it( 'should allow disabling a ref', () => { - const { container, rerender, unmount } = render( - - ); + const { rerender, unmount } = render( ); - const originalElement = container.firstElementChild; + const originalElement = screen.getByRole( 'list' ); // Render 1: ref 1 should be disabled. expect( renderCallback.history ).toEqual( [ @@ -315,9 +309,9 @@ describe( 'useMergeRefs', () => { } ); it( 'should allow the hook being unused', () => { - const { container, rerender } = render( ); + const { rerender } = render( ); - const originalElement = container.firstElementChild; + const originalElement = screen.getByRole( 'list' ); // Render 1: ref 1 should updated, ref 2 should not. expect( renderCallback.history ).toEqual( [ From 49ff38fd722da0f10dc3d0d7087569b6965ff9a3 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Wed, 2 Nov 2022 08:51:54 +0100 Subject: [PATCH 5/6] Check panel rendering by looking up the title element, avoid snapshots --- .../test/__snapshots__/index.js.snap | 3 --- .../sidebar/plugin-pre-publish-panel/test/index.js | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) delete mode 100644 packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/__snapshots__/index.js.snap diff --git a/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/__snapshots__/index.js.snap b/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/__snapshots__/index.js.snap deleted file mode 100644 index 7540e75611134..0000000000000 --- a/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/__snapshots__/index.js.snap +++ /dev/null @@ -1,3 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`PluginPrePublishPanel renders fill properly 1`] = `"

My panel content
"`; diff --git a/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js b/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js index 7116bdad0f2de..aaa4f31a074a4 100644 --- a/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js +++ b/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; /** * WordPress dependencies @@ -15,7 +15,7 @@ import PluginPrePublishPanel from '../'; describe( 'PluginPrePublishPanel', () => { test( 'renders fill properly', () => { - const { container } = render( + render( { ); - expect( container.innerHTML ).toMatchSnapshot(); + expect( screen.getByText( 'My panel title' ) ).toBeInTheDocument(); } ); } ); From 3b5e2a069e2e7e49e9e4bba20e201a1eabfc0d71 Mon Sep 17 00:00:00 2001 From: Jarda Snajdr Date: Thu, 3 Nov 2022 13:07:08 +0100 Subject: [PATCH 6/6] use .toBeVisible() --- .../components/sidebar/plugin-pre-publish-panel/test/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js b/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js index aaa4f31a074a4..2b6e410a01d96 100644 --- a/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js +++ b/packages/edit-post/src/components/sidebar/plugin-pre-publish-panel/test/index.js @@ -28,6 +28,6 @@ describe( 'PluginPrePublishPanel', () => { ); - expect( screen.getByText( 'My panel title' ) ).toBeInTheDocument(); + expect( screen.getByText( 'My panel title' ) ).toBeVisible(); } ); } );