Data: Refactor 'useDispatch' tests to use RTL #44802
Conversation
Mamaduka
added
[Type] Enhancement
| @@ -19,39 +28,39 @@ describe( 'useDispatch', () => { | |||
|
|
|||
| it( 'returns dispatch function from store with no store name provided', () => { | |||
There was a problem hiding this comment.
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.
|
Size Change: +340 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
The refactor looks good, I only have a few minor suggestions.
I don't like the style of the original tests very much though. They should test observable behavior instead of number of calls etc. Like:
- don't check that
useDispatchreturns some function identical to an expected value, check that it returns a function with expected behavior, i.e., one that can be called withdispatch( 'store' ).foo()and that really performs actionfooon storestore. - don't check that action function has been called 0 or 1 or X times, check that the store state has changed as expected. Let the action increment a counter, for example, and then verify that
select( 'store' ).getCounter()returns an incremented value.
This would be nice to change either in this PR or in a followup.
| ); | ||
| } ); | ||
| useEffect( () => { | ||
| result.current = dispatch; |
There was a problem hiding this comment.
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.
| it( 'returns expected action creators from store for given storeName', async () => { | ||
| const user = userEvent.setup( { | ||
| advanceTimers: jest.advanceTimersByTime, | ||
| } ); | ||
| const testAction = jest.fn().mockImplementation( noop ); |
There was a problem hiding this comment.
A simple jest.fn() returns a noop function, too.
There was a problem hiding this comment.
It looks like we need special noop (() => ( { type: '__INERT__' } )) to avoid errors.
There was a problem hiding this comment.
It's arguable whether the tests here actually test behavior introduced by useDispatch(), but I find some value in them, especially from the perspective of an end user.
I've left some minor thoughts and suggestions, but this already looks good, especially after addressing @jsnajdr's feedback.
| testInstance.findByType( 'button' ).props.onClick(); | ||
| } ); | ||
| expect( firstRegistryAction ).toHaveBeenCalledTimes( 1 ); | ||
| expect( secondRegistryAction ).toHaveBeenCalledTimes( 1 ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added new test for this case - f6c1a3c.
There was a problem hiding this comment.
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%.
| }, | ||
| } ); | ||
| it( 'returns dispatch function from store with no store name provided', async () => { | ||
| const user = userEvent.setup(); |
There was a problem hiding this comment.
For consistency with the rest of the tests, let's make sure to initiate user-event like this:
| const user = userEvent.setup(); | |
| const user = userEvent.setup( { | |
| advanceTimers: jest.advanceTimersByTime, | |
| } ); |
There was a problem hiding this comment.
I've no strong preference here. Using jest.useRealTimers() was suggested above - #44802 (comment).
There was a problem hiding this comment.
My suggestion is mostly to keep consistency with the rest of the tests throughout the codebase, where we use jest.advanceTimersByTime(). It is kind of predictable to know that timers work the same way across all tests 😉 No strong feelings towards it, either, I just recall @ciampo suggesting we keep consistency with this and I always favor consistency when possible.
There was a problem hiding this comment.
I think the test should choose between fake/real timers primarily according to what it tests and what timers it needs. Consistency is secondary.
The actual tested code doesn't use any timers at all. It's just that userEvent.click internally dispatches two events, "move mouse to the element" and "click on the element," with a zero delay between them (setTimeout( x, 0 )). So it has to deal with Jest timers in some way.
Here real timers are perfectly fine. Fake timers provide two services and we use neither of them: i) execute timeouts like 5000ms without actually waiting for 5 seconds, by "compressing" time and moving through it manually; ii) be able to stop at a precise moment in time and execute code there.
There was a problem hiding this comment.
In my previous experience, I seem to recall that relying on real timers could sometimes cause tests to fail with a timeout error — although things may have changed as time passed.
There was a problem hiding this comment.
Jest allows a test to run up to 5 seconds and then aborts it and reports error. That might be the timeout you've seen. But userEvent.click merely does a setTimeout( continue, 0 ) to do an event loop tick between two input events. With userEvent.click( { delay: 5000 } ) there would be a longer, configurable delay.
There was a problem hiding this comment.
I believe that in my case it was more of a glitch in how user-event's calls to setTimeout worked, although I don't have much evidence at hand. I just remember that I initially got around that issue by using delay: null as suggested in this GitHub issue. Later I ended up using the advanceTimers option after it was introduced in version 14.2
There was a problem hiding this comment.
Yes, delay: null works because when it's not a number, the waiting code is skipped:
https://github.com/testing-library/user-event/blob/main/src/utils/misc/wait.ts
The default value of delay is 0, and that leads to a setTimeout( r, 0 ). That's confusing because when your project uses fake timers by default, and even when there are no timers at all in the tested code, the userEvent internals still get stuck because nobody is advancing the timers for them.
| } ); | ||
|
|
||
| it( 'returns expected action creators from store for given storeName', async () => { | ||
| const user = userEvent.setup(); |
There was a problem hiding this comment.
| const user = userEvent.setup(); | |
| const user = userEvent.setup( { | |
| advanceTimers: jest.advanceTimersByTime, | |
| } ); |
| }, | ||
| } ); | ||
| it( 'returns expected action creators from store for given store descriptor', async () => { | ||
| const user = userEvent.setup(); |
There was a problem hiding this comment.
| const user = userEvent.setup(); | |
| const user = userEvent.setup( { | |
| advanceTimers: jest.advanceTimersByTime, | |
| } ); |
| const firstRegistryAction = jest.fn().mockImplementation( noop ); | ||
| const secondRegistryAction = jest.fn().mockImplementation( noop ); | ||
| it( 'returns dispatch from correct registry if registries change', async () => { | ||
| const user = userEvent.setup(); |
There was a problem hiding this comment.
| const user = userEvent.setup(); | |
| const user = userEvent.setup( { | |
| advanceTimers: jest.advanceTimersByTime, | |
| } ); |
|
I will merge this, and I am happy to follow up if a different style is preferred for |
| }, | ||
| } ); | ||
|
|
||
| it( 'returns expected action creators from store for given storeName', async () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for catching this. I will do a quick follow-up tomorrow.
| get: ( state ) => state, | ||
| }, | ||
| }; | ||
|
|
||
| let registry; | ||
| beforeEach( () => { | ||
| registry = createRegistry(); | ||
| } ); |
There was a problem hiding this comment.
Each test could createRegistry separately, and have registry as local variable. Very little extra verbosity in exchange for self-contained code with no external deps.
| }, | ||
| } ); | ||
| it( 'returns dispatch function from store with no store name provided', async () => { | ||
| const user = userEvent.setup(); |
There was a problem hiding this comment.
I think the test should choose between fake/real timers primarily according to what it tests and what timers it needs. Consistency is secondary.
The actual tested code doesn't use any timers at all. It's just that userEvent.click internally dispatches two events, "move mouse to the element" and "click on the element," with a zero delay between them (setTimeout( x, 0 )). So it has to deal with Jest timers in some way.
Here real timers are perfectly fine. Fake timers provide two services and we use neither of them: i) execute timeouts like 5000ms without actually waiting for 5 seconds, by "compressing" time and moving through it manually; ii) be able to stop at a precise moment in time and execute code there.
| testInstance.findByType( 'button' ).props.onClick(); | ||
| } ); | ||
| expect( firstRegistryAction ).toHaveBeenCalledTimes( 1 ); | ||
| expect( secondRegistryAction ).toHaveBeenCalledTimes( 1 ); |
There was a problem hiding this comment.
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%.
What?
PR of #44780.
PR refactors
useDispatchhook tests to use@testing-library/reactinstead ofreact-test-renderer.Why?
It is a part of recent efforts to use
@testing-library/reactas the project's primary testing library.How?
We're just using the render method from RTL, improving how assertions, and using
userEventfor user actions.Testing Instructions