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.

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

useEffect not triggering inside jest #215

Closed
peterjuras opened this issue Nov 3, 2018 · 32 comments

Comments

@peterjuras
Copy link

commented Nov 3, 2018

  • react-testing-library version: 5.2.3
  • react version: 16.7.0-alpha.0
  • node version: CodeSandbox
  • npm (or yarn) version: CodeSandbox

Relevant code or config:

  let a = false;
  function Test() {
    React.useEffect(() => {
      a = true;
    });
    return <div>Hello World</div>;
  }
  render(<Test />);
  expect(a).toBe(true);

What you did:

I'm trying to test functions with the useEffect hook inside jest.

What happened:

The useEffect hook is not executed correctly inside a jest environment after calling render(<Test />). However it appears to be working correctly if it is called directly in a browser environment.

Reproduction:

Working example: https://codesandbox.io/s/l947z8v6xq (index.js)
Not working example in jest: https://codesandbox.io/s/7k5oj92740 (index.test.js)

Problem description:

The useEffect hook should have been executed after the render call

@peterjuras

This comment has been minimized.

Copy link
Author

commented Nov 3, 2018

After more testing, it appears that the working example is also not working :/

@sivkoff

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2018

That's because React doesn't run the hook in sync mode. If you rerender the component the second time, it changes a to true: https://codesandbox.io/s/m5m5yqrr18

@vovaguguiev

This comment has been minimized.

Copy link

commented Nov 3, 2018

@sivkoff any idea why this happens and whether this will be fixed either in react or in react-testing-library?

@sivkoff

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2018

@wzrdzl, I'm not sure if it will be fixed, because it is a documented behavior:

Unlike componentDidMount and componentDidUpdate, the function passed to useEffect fires after layout and paint, during a deferred event. This makes it suitable for the many common side effects, like setting up subscriptions and event handlers, because most types of work shouldn’t block the browser from updating the screen.

More details in React Hooks reference: https://reactjs.org/docs/hooks-reference.html#timing-of-effects

@alexkrolick

This comment has been minimized.

Copy link
Collaborator

commented Nov 4, 2018

You might want to use the waitForElement or wait utils to asynchronously await the result of the hook, or try these auto-retrying queries if your environment is highly asynchronous https://github.com/alexkrolick/dom-testing-addon-async

@vovaguguiev

This comment has been minimized.

Copy link

commented Nov 4, 2018

@sivkoff thanks for pointing to the docs, so as I understand the root cause of this problem is not really the "sync" test code. My test code looks like this and it also breaks without a re-render (I also tried to wait for assertion in a real setTimeout instead of using fake timers API, it didn't work either):

it('should render a spinner after `showAfterDelay` milliseconds', () => {
      jest.useFakeTimers();
      // LoadingIndicator uses useEffect internal to schedule the rendering after a delay (setTimeout)
      const { getByTestId, rerender } = render(<LoadingIndicator showAfterDelay={500} />);

      // no spinner initially
      expect(() => getByTestId(LOADING_SPINNER_CONTAINER_TEST_ID)).toThrow();

      // now you need to rerender for React to run the `useEffect` hook
      // this should hopefully be fixed or worked around with some new react-testing-library API
      // this is the price you pay for riding the hype train :)
      // https://github.com/kentcdodds/react-testing-library/issues/215#issuecomment-435592444
      rerender(<LoadingIndicator showAfterDelay={500} />);

      jest.advanceTimersByTime(500);

      expect(getByTestId(LOADING_SPINNER_CONTAINER_TEST_ID)).toBeTruthy();
    });

Looks like the issue is that the React's "determine paint event" logic doesn't work in jsdom (my wild guess) so I wonder if we can somehow fix that, because using rerender looks like a workaround but not really a solution

@alexkrolick

This comment has been minimized.

Copy link
Collaborator

commented Nov 4, 2018

Could it be that showAfterDelay only schedules after 500ms, so you need to wait a bit longer than that before running synchronous expects? Try waiting with the mutation observer helper waitForDomChange or one of the other helpers.

import {waitForDomChange} from 'dom-testing-library'

it('should render a spinner after `showAfterDelay` milliseconds', () => {
      // LoadingIndicator uses useEffect internal to schedule the rendering after a delay (setTimeout)
      const { getByTestId, rerender } = render(<LoadingIndicator showAfterDelay={500} />);

      // no spinner initially
      expect(() => getByTestId(LOADING_SPINNER_CONTAINER_TEST_ID)).toThrow();

      // now you need to rerender for React to run the `useEffect` hook
      // this should hopefully be fixed or worked around with some new react-testing-library API
      // this is the price you pay for riding the hype train :)
      // https://github.com/kentcdodds/react-testing-library/issues/215#issuecomment-435592444
      
      await waitForDomChange()

      expect(getByTestId(LOADING_SPINNER_CONTAINER_TEST_ID)).toBeTruthy();
    });
@sivkoff

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2018

@wzrdzl There is another simple workaround for your case — you just need to rerender it once again 😄

Let me explain how it works (probably I'm wrong):

  • the first render initializes the hook
  • the second render resolves the callback and runs the timer
  • before the third render you need to advance your timer, so when you render it again, your component will already have changed state.

I agree this workaround is ugly, but not sure if it makes sense to fix the case before React core team releases the stable version. Furthermore example with lifecycle alternative works well, so I think jsdom handles React event logic correctly.

I've created a repo with the example so you can clone and make sure:
https://github.com/sivkoff/hooks-testing-sandbox
Unfortunatelly CodeSandbox doesn't support mocking timers, so you need to clone it to localhost.

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 4, 2018

You may also be interesting in http://kcd.im/hooks-and-suspense

I have a video in there that explains one of the big nuances with testing effect hooks.

I do plan on creating a small function called flushEffects or something that'll make this easier.

@alexkrolick

This comment has been minimized.

Copy link
Collaborator

commented Nov 4, 2018

@kentcdodds special case handling for side-effectful components seems like it's letting implementation details shape the tests. What about adding { flushEffects: [true] } to the render options?

EDIT: (where true is the default)

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 4, 2018

If prefer that to just be the default. If we can make that work then let's do it. I just don't think it's possible

@FredyC FredyC referenced this issue Nov 4, 2018
@FredyC

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2018

I feel a bit dumb now. I spent some time writing an issue to linked RFC ⬆️ only to find out later that there only a single issue in this repo and it's exactly that problem 😅 Oh well...

I am wondering, why using wait does not work? In my attempts, it just timeouts after 5 seconds, but DOM should be updated by that time no matter if React is postponing that.

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

@FredyC, don't feel dumb 🤗

It's really odd, but I observed that wait works if you run your tests in the browser but does not work if you run them in jsdom (which most people testing react will be doing). I never dug into the reason (my guess is it's an issue with react's scheduler and jsdom/jest timers) because I don't want to have to use wait. I want to find another solution.

@zaguiini

This comment has been minimized.

Copy link

commented Nov 13, 2018

I think the key part is when @kentcdodds explains that you need to rerender to get useEffect results. Really took me all morning to fix that 👎

Anyways, thanks a lot!

(I don't think React will change the way that useEffect behaves because that's what it is intended to: don't block the rendering process.)

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Currently the way that I dislike the least for making this work nicely is to put this in my setup file:

beforeAll(() => jest.spyOn(React, 'useEffect').mockImplementation(React.useLayoutEffect))
afterAll(() => React.useEffect.mockRestore())

I don't like it, but it's my favorite anyway...

@zaguiini

This comment has been minimized.

Copy link

commented Nov 13, 2018

Currently the way that I dislike the least for making this work nicely is to put this in my setup file:

beforeAll(() => jest.spyOn(React, 'useEffect').mockImplementation(React.useLayoutEffect))
afterAll(() => React.useEffect.mockRestore())

I don't like it, but it's my favorite anyway...

But are they exactly the same apart from don't blocking the rendering process?

EDIT: Yes, they are. Per docs:

The signature is identical to useEffect, but it fires synchronously after all DOM mutations. Use this to read layout from the DOM and synchronously re-render. Updates scheduled inside useLayoutEffect will be flushed synchronously, before the browser has a chance to paint.

@FredyC

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

@kentcdodds Have you considering this workaround approach? Feels much easier imo ... facebook/react#14050 (comment)

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

Basically the same except there's no way to clean up. So if you wanted to get the original useEffect back for a certain test for some reason you wouldn't be able to. I suppose you could use that mock to do the spy stuff though 🤷‍♂️ I cool with either. But I'm still not super happy with it. Hoping the react team comes up with something else...

@ivan-kleshnin

This comment has been minimized.

Copy link

commented Nov 23, 2018

For some reason, this workaround:

beforeAll(() => 
  console.log("@ beforeAll")
  jest.spyOn(React, 'useEffect').mockImplementation(React.useLayoutEffect)
)
afterAll(() => React.useEffect.mockRestore())

changes nothing to my Jest tests, effects just never happen. Only manual replacement of useEffect with useLayoutEffect in components does the trick. I know it's not a QA thread, but maybe other people are experiencing the same... so any help will be appreciated.

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

I'm actually starting to think that it's better to manually trigger effects. That's why I added the experiential flushEffects function (see the docs).

@zaguiini

This comment has been minimized.

Copy link

commented Nov 23, 2018

I'm actually starting to think that it's better to manually trigger effects. That's why I added the experiential flushEffects function (see the docs).

Noooooooo. That would be an implementation detail. The user don't actually flush the effects. In fact, they don't even know what an effect isssss :/

@pelotom

This comment has been minimized.

Copy link

commented Nov 23, 2018

@kentcdodds idk, it feels a little like the test having too much awareness of implementation details

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

I agree, but at the same time, one could argue that running anything synchronously is an implementation detail. If we really wanted to be free of implementation details then everything we do with react-testing-library would be asynchronous.

The thing is that if you force useEffect to run synchronously, you're actually not simulating what the user's experiencing. What about the state between when the render runs and effect callback runs? How would you test that state?

@pelotom

This comment has been minimized.

Copy link

commented Nov 23, 2018

The thing is that if you force useEffect to run synchronously, you're actually not simulating what the user's experiencing. What about the state between when the render runs and effect callback runs? How would you test that state?

Is that an actual observable frame? I didn’t realize that, but then I’m not too familiar with concurrent react yet. Still, I’m having trouble imagining a case where I’d care to test that intermediate state.

@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

Is that an actual observable frame?

It can be. If you don't want it to be then you're either initializing your state incorrectly or you should use useLayoutEffect.

@alexkrolick

This comment has been minimized.

Copy link
Collaborator

commented Nov 23, 2018

Options right now:

  1. As close as possible to production mode: useEffect runs async in JSDOM the same way it does in-browser, and flushEffects is only used to skip to the end state for slow effects or debugging. Without flushEffects the end state should be awaited using waitForElement or an async query. As we know, this doesn't work right now for some reason, so you need flushEffects to get useEffect (unless you mock it to point at useLayoutEffect).
  2. Force sync mode for the entire React tree changing what's passed to ReactDOM.render. I think the intermediate state would still be committed to the DOM, but useEffect effects would process immediately afterward and do whatever they do. I am not sure if the APIs to do this are stable or exposed at all at the moment.
@kentcdodds

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

I think it would be best if we come out with something that's minimally helpful/opinionated just to enable testing at all. Then when we get real world experience with things we'll be better suited to solving this problem with the right abstraction. It's too early to form opinions.

For that reason, I'm going to close this. We can bring it up again in ~6 months or so.

@alflennik

This comment has been minimized.

Copy link

commented Feb 25, 2019

The lack of coverage for my useEffect code led me here. It's been three months since the issue was closed, but I'm feeling the pain right now. I would absolutely love an automatic flushEffects feature. Without it, how am I supposed to test my side effects? I am not going to introduce timeouts into my tests.

@FredyC

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

@matewilk

This comment has been minimized.

Copy link

commented Apr 23, 2019

It seems the topic diverged a little bit but going back to the original issue.
Why not just use jest.spyOn(React, 'useEffect') and test it this way?

@FredyC

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@matewilk Because you are testing implementation detail then? Seriously, just grab the awesome act (especially the one from 16.9 alpha) and you are golden.

@afenton90

This comment has been minimized.

Copy link

commented Jul 9, 2019

@FredyC I followed the documentation you have linked to, but no wrapping of render or event in the act function allows for the effects to asynchronously update the component after its initial render.

Would be great to work together on this issue to get something more obvious working for tests which need to assert side effects on the DOM. At the moment anything in useEffect is untestable if it has an effect on the DOM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.