Skip to content

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

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

Merged
jsnajdr merged 6 commits into from
Nov 3, 2022
Merged

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

Changes from 1 commit
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Replace rerender(null) with unmount()
  • Loading branch information
@jsnajdr
jsnajdr committed Nov 2, 2022
commit e2784baea74264cbde2138267674437a0ccbf292
30 changes: 18 additions & 12 deletions packages/compose/src/hooks/use-merge-refs/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ describe( 'useMergeRefs', () => {
} );

it( 'should work', () => {
const { container, rerender } = render( <MergedRefs /> );
const { container, rerender, unmount } = render( <MergedRefs /> );

const originalElement = container.firstElementChild;
Copy link
Member

@tyxla tyxla Nov 1, 2022

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

@jsnajdr jsnajdr Nov 2, 2022

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?


Expand All @@ -95,7 +95,7 @@ describe( 'useMergeRefs', () => {
[ [], [] ],
] );

rerender( null );
unmount();

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

it( 'should work for node change', () => {
const { container, rerender } = render( <MergedRefs /> );
const { container, rerender, unmount } = render( <MergedRefs /> );

const originalElement = container.firstElementChild;

Expand All @@ -129,7 +129,7 @@ describe( 'useMergeRefs', () => {
[ [], [] ],
] );

rerender( null );
unmount();

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

it( 'should work with dependencies', () => {
const { container, rerender } = render( <MergedRefs count={ 1 } /> );
const { container, rerender, unmount } = render(
<MergedRefs count={ 1 } />
);

const originalElement = container.firstElementChild;

Expand All @@ -161,7 +163,7 @@ describe( 'useMergeRefs', () => {
[ [], [ originalElement ] ],
] );

rerender( null );
unmount();

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

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

const originalElement = container.firstElementChild;

Expand All @@ -199,7 +203,7 @@ describe( 'useMergeRefs', () => {
[ [], [ newElement ] ],
] );

rerender( null );
unmount();

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

it( 'should work for dependency change after node change', () => {
const { container, rerender } = render( <MergedRefs /> );
const { container, rerender, unmount } = render( <MergedRefs /> );

const originalElement = container.firstElementChild;

Expand Down Expand Up @@ -248,7 +252,7 @@ describe( 'useMergeRefs', () => {
[ [], [ newElement ] ],
] );

rerender( null );
unmount();

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

it( 'should allow disabling a ref', () => {
const { container, rerender } = render( <MergedRefs disable1 /> );
const { container, rerender, unmount } = render(
<MergedRefs disable1 />
);

const originalElement = container.firstElementChild;

Expand Down Expand Up @@ -295,7 +301,7 @@ describe( 'useMergeRefs', () => {
[ [], [ originalElement ] ],
] );

rerender( null );
unmount();

// Unmount: current callback functions should receive null.
expect( renderCallback.history ).toEqual( [
Expand Down