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

Data: Refactor 'useDispatch' tests to use RTL #44802

Merged
merged 6 commits into from
Oct 10, 2022
+124 −117
Merged

Data: Refactor 'useDispatch' tests to use RTL #44802

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f11e488
Data: Refactor 'useDispatch' tests to use RTL
Mamaduka Oct 9, 2022
2454cdb
Clear mock function between assertions
Mamaduka Oct 9, 2022
6751742
Simplify userEvent setup
Mamaduka Oct 10, 2022
91169fc
Update tests to observer actual store behavior
Mamaduka Oct 10, 2022
1bdc796
Remove leftover comment
Mamaduka Oct 10, 2022
f6c1a3c
Return action call tests
Mamaduka Oct 10, 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
150 changes: 69 additions & 81 deletions packages/data/src/components/use-dispatch/test/use-dispatch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
/**
* External dependencies
*/
import TestRenderer, { act } from 'react-test-renderer';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

/**
* WordPress dependencies
*/
import { createRef, useEffect } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -11,6 +17,9 @@ import createReduxStore from '../../../redux-store';
import { createRegistry } from '../../../registry';
import { RegistryProvider } from '../../registry-provider';

const noop = () => ( { type: '__INERT__' } );
const reducer = ( state ) => state;

describe( 'useDispatch', () => {
let registry;
beforeEach( () => {
Expand All @@ -19,39 +28,39 @@ describe( 'useDispatch', () => {

it( 'returns dispatch function from store with no store name provided', () => {
Copy link
Member Author

@Mamaduka Mamaduka Oct 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here we could use a new renderHook feature, but it's only available in RTL v13.1.0, which also requires React 18.

Meanwhile, I'm using a similar method as renderHook to test the hook's return value - https://github.com/eps1lon/react-testing-library/blob/v13.4.0/src/pure.js#L223-L233.

registry.registerStore( 'demoStore', {
reducer: ( state ) => state,
reducer,
actions: {
foo: () => 'bar',
},
} );

const result = createRef();
const TestComponent = () => {
return <div></div>;
};
const Component = () => {
const dispatch = useDispatch();
return <TestComponent dispatch={ dispatch } />;
};

let testRenderer;
act( () => {
testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<Component />
</RegistryProvider>
);
} );
useEffect( () => {
result.current = dispatch;
Copy link
Member

Choose a reason for hiding this comment

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

result can be just a variable, no need to make it a ref.

const result = null;
/* ... */
useEffect( () => { result = dispatch; } );
/* ... */
expect( result ).toBe( registry.dispatch );

We're testing the useDispatch return value, nothing else.

} );

const testInstance = testRenderer.root;
return null;
};

expect( testInstance.findByType( TestComponent ).props.dispatch ).toBe(
registry.dispatch
render(
<RegistryProvider value={ registry }>
<TestComponent />
</RegistryProvider>
);

expect( result.current ).toBe( registry.dispatch );
} );
it( 'returns expected action creators from store for given storeName', () => {
const noop = () => ( { type: '__INERT__' } );

it( 'returns expected action creators from store for given storeName', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

The descriptions of the two tests, one for "store name," other for "store descriptor," are flipped. The "store name" one actually tests a descriptor and vice versa.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this. I will do a quick follow-up tomorrow.

const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );
Mamaduka marked this conversation as resolved.
Show resolved Hide resolved
const testAction = jest.fn().mockImplementation( noop );
Copy link
Member

Choose a reason for hiding this comment

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

A simple jest.fn() returns a noop function, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like we need special noop (() => ( { type: '__INERT__' } )) to avoid errors.

const store = createReduxStore( 'demoStore', {
reducer: ( state ) => state,
reducer,
actions: {
foo: testAction,
},
Expand All @@ -63,30 +72,24 @@ describe( 'useDispatch', () => {
return <button onClick={ foo } />;
};

let testRenderer;

act( () => {
testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<TestComponent />
</RegistryProvider>
);
} );

const testInstance = testRenderer.root;
render(
<RegistryProvider value={ registry }>
<TestComponent />
</RegistryProvider>
);

act( () => {
testInstance.findByType( 'button' ).props.onClick();
} );
await user.click( screen.getByRole( 'button' ) );

expect( testAction ).toHaveBeenCalledTimes( 1 );
} );

it( 'returns expected action creators from store for given store descriptor', () => {
const noop = () => ( { type: '__INERT__' } );
it( 'returns expected action creators from store for given store descriptor', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );
const testAction = jest.fn().mockImplementation( noop );
registry.registerStore( 'demoStore', {
reducer: ( state ) => state,
reducer,
actions: {
foo: testAction,
},
Expand All @@ -96,32 +99,25 @@ describe( 'useDispatch', () => {
return <button onClick={ foo } />;
};

let testRenderer;

act( () => {
testRenderer = TestRenderer.create(
<RegistryProvider value={ registry }>
<TestComponent />
</RegistryProvider>
);
} );

const testInstance = testRenderer.root;
render(
<RegistryProvider value={ registry }>
<TestComponent />
</RegistryProvider>
);

act( () => {
testInstance.findByType( 'button' ).props.onClick();
} );
await user.click( screen.getByRole( 'button' ) );

expect( testAction ).toHaveBeenCalledTimes( 1 );
} );

it( 'returns dispatch from correct registry if registries change', () => {
const reducer = ( state ) => state;
const noop = () => ( { type: '__INERT__' } );
it( 'returns dispatch from correct registry if registries change', async () => {
const user = userEvent.setup( {
advanceTimers: jest.advanceTimersByTime,
} );
const firstRegistryAction = jest.fn().mockImplementation( noop );
const secondRegistryAction = jest.fn().mockImplementation( noop );

const firstRegistry = registry;
const firstRegistry = createRegistry();
firstRegistry.registerStore( 'demo', {
reducer,
actions: {
Expand All @@ -134,22 +130,17 @@ describe( 'useDispatch', () => {
return <button onClick={ () => dispatch( 'demo' ).noop() } />;
};

let testRenderer;
act( () => {
testRenderer = TestRenderer.create(
<RegistryProvider value={ firstRegistry }>
<TestComponent />
</RegistryProvider>
);
} );
const testInstance = testRenderer.root;

act( () => {
testInstance.findByType( 'button' ).props.onClick();
} );
const { rerender } = render(
<RegistryProvider value={ firstRegistry }>
<TestComponent />
</RegistryProvider>
);

await user.click( screen.getByRole( 'button' ) );
expect( firstRegistryAction ).toHaveBeenCalledTimes( 1 );
expect( secondRegistryAction ).toHaveBeenCalledTimes( 0 );
expect( secondRegistryAction ).not.toHaveBeenCalled();

firstRegistryAction.mockClear();
Mamaduka marked this conversation as resolved.
Show resolved Hide resolved

const secondRegistry = createRegistry();
secondRegistry.registerStore( 'demo', {
Expand All @@ -159,17 +150,14 @@ describe( 'useDispatch', () => {
},
} );

act( () => {
testRenderer.update(
<RegistryProvider value={ secondRegistry }>
<TestComponent />
</RegistryProvider>
);
} );
act( () => {
testInstance.findByType( 'button' ).props.onClick();
} );
expect( firstRegistryAction ).toHaveBeenCalledTimes( 1 );
rerender(
<RegistryProvider value={ secondRegistry }>
<TestComponent />
</RegistryProvider>
);

await user.click( screen.getByRole( 'button' ) );
expect( firstRegistryAction ).not.toHaveBeenCalled();
expect( secondRegistryAction ).toHaveBeenCalledTimes( 1 );
Copy link
Member

Choose a reason for hiding this comment

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

I'd say that there is some value to keep at least one test that ensures that the actions are called. While it somewhat tests the underlying registry and not useDispatch in particular, it still allows us to ensure that the registry is working as before, and in a way we expect when useDispatch is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

The store values won't match if actions aren't called correctly. Would you agree that we're also implicitly testing the action calls here?

Copy link
Member

Choose a reason for hiding this comment

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

Yup 👍 I agree with that. The only reason why I'd want to see us specifically dispatching the right action is because it's actually useDispatch we're testing, so we want to ensure that it's dispatching correctly. Also it would make things more explicit. But I agree that from the user perspective it's enough to test the result store values, so feel free to keep it as-is, I have no strong feelings either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added new test for this case - f6c1a3c.

Copy link
Member

Choose a reason for hiding this comment

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

I'd personally avoid the jest.fn() mocks completely and check only store changes. We're testing that useDispatch returns something that actually dispatches the right action to the right store in the right registry when called, and checking the store state afterwards verifies all that 100%.

} );
} );