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

Testing component with async componentDidMount #1587

Closed
2 of 10 tasks
kazagkazag opened this issue Mar 20, 2018 · 27 comments
Closed
2 of 10 tasks

Testing component with async componentDidMount #1587

kazagkazag opened this issue Mar 20, 2018 · 27 comments

Comments

@kazagkazag
Copy link

Current behavior

When I have to wait for some data in componentDidMount, I can't test that after data is fetched component rendered with expected content or when async method failed, component rendered error.

I used create-react-app in version 1.5.2.
Enzyme in version 3.3.0.

My App.js:

import React, { Component } from "react";
import logo from "./logo.svg";
import "./App.css";

class App extends Component {
  state = {
    initialized: false,
    error: false
  }

  componentDidMount() {
    return Promise
      .all([
        this.props.getUserData(),
        this.props.getAppData()
      ])
      .then(() => {
        this.setState({
          initialized: true
        });
      })
      .catch((error) => {
        this.setState({
          error: true
        });
      });
  }

  render() {
    return this.renderLoader() || this.renderError() || this.renderApp();
  }

  renderError() {
    return !this.state.initialized && this.state.error ? <p>Error</p> : null;
  }

  renderLoader() {
    return !this.state.initialized && !this.state.error ? <p>Loading</p> : null;
  }

  renderApp() {
    return <p>App!</p>
  }
}

export default App;

App.test.js:

import Enzyme, { mount } from "enzyme";
import Adapter from "enzyme-adapter-react-16";
import React from "react";

Enzyme.configure({ adapter: new Adapter() });

import App from "./App";

const resolvePromise = () => Promise.resolve();
const rejectPromise = () => Promise.reject("Error");

describe("Test", () => {
  test("should render content if initial data is available", () => {
    const mounted = mount(
      <App
        getUserData={resolvePromise}
        getAppData={resolvePromise}
      />
    );

    return Promise
      .resolve(mounted)
      .then(() => mounted.update())
      .then(() => {
        expect(mounted.text()).toContain("App!");
      });
  });

  test("should render error if initial data is NOT available", () => {
    const mounted = mount(
      <App
        getUserData={rejectPromise}
        getAppData={resolvePromise}
      />
    );

    return Promise
      .resolve(mounted)
      .then(() => mounted.update())
      .then(() => {
        expect(mounted.text()).toContain("Error");
      });
  });
});

Result:

 FAIL  src/App.test.js
  ● Test › should render error if initial data is NOT available

    expect(string).toContain(value)
    
    Expected string:
      "Loading"
    To contain value:
      "Error"
      
      at Promise.resolve.then.then (src/App.test.js:41:30)
          at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:188:7)

  Test
    ✓ should render content if initial data is available (17ms)
    ✕ should render error if initial data is NOT available (3ms)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 passed, 2 total
Snapshots:   0 total
Time:        0.841s, estimated 1s
Ran all test suites related to changed files.

I've tried several methods mentioned here: #346 .
Test for positive case works, but for negative doesn't, so I think that there must by an issue with entire approach, and "green light" for first test is just an accident.

Expected behavior

I would like to have way to test both positive and negative paths.

API

  • shallow
  • mount
  • render

Version

library version
Enzyme 3.3.0
React 16

Adapter

  • enzyme-adapter-react-16
  • enzyme-adapter-react-15
  • enzyme-adapter-react-15.4
  • enzyme-adapter-react-14
  • enzyme-adapter-react-13
  • enzyme-adapter-react-helper
  • others ( )
@koba04
Copy link
Contributor

koba04 commented Mar 20, 2018

@kazagkazag What about this?

  test("should render error if initial data is NOT available", () => {
    const mounted = mount(
      <App
        getUserData={rejectPromise}
        getAppData={resolvePromise}
      />
    );

    return Promise
      .resolve(mounted)
      .then(() => {})
      .then(() => {
        mounted.update()
        expect(mounted.text()).toContain("Error");
      });
  });

@kazagkazag
Copy link
Author

kazagkazag commented Mar 20, 2018

It doesn't work either.

But:

return Promise
      .resolve(mounted)
      .then(() => mounted.update())
      .then(() => mounted.update())
      .then(() => {
        expect(mounted.text()).toContain("Error");
      });

works. (notice second update())

I don't know why, I have to investigate that...

This also happens to work:

test("should render error if initial data is NOT available", (done) => {
    const mounted = mount(
      <App
        getUserData={rejectPromise}
        getAppData={resolvePromise}
      />
    );

    setImmediate(() => {
      expect(mounted.text()).toContain("Error");
      done();
    });
  });

I don't know If I can rely on that tests...

@koba04
Copy link
Contributor

koba04 commented Mar 20, 2018

@kazagkazag
It's caused by the order for Promises(microtasks).
For example, the following script prints 1 3 2 4.

Promise.resolve()
.then(() => console.log('1'))
.then(() => console.log('2'))

Promise.resolve()
.then(() => console.log('3'))
.then(() => console.log('4'))

Also, the following prints 3 2 4, not 2 3 4.

Promise.reject()
.then(() => console.log('1'))
.catch(() => console.log('2'))

Promise.resolve()
.then(() => console.log('3'))
.then(() => console.log('4'))

Even if Promise is already resolved, Promise isn't processed synchronously.
In addition to that, chaining promises isn't processed synchronously, which are cued as microtasks and be processed.

In your case,

    return Promise
      .all([
        this.props.getUserData(),
        this.props.getAppData()
      ])
      .then(() => {
        this.setState({
          initialized: true
        });
      })
      .catch((error) => {
        this.setState({
          error: true
        });
      });

When getUserData is rejected, the promise is processed through the then and catch so the setState is called at the second microtask.
It's not the same case with all promises are resolved.
If all promises are resolved, the setState is called at the first microtask.
As a result, you need extra then calling when the promise is rejected.

If you use setImmediate, it's processed after all microtasks are processed so it will work fine.

@kazagkazag
Copy link
Author

Ok, thanks for explanation, but I have one more question:

When getUserData is rejected, the promise is processed through the then and catch so the setState is called at the second microtask.

Why promise is processed through .then() ? If any of those promises reject it should skip to .catch() section, shouldn't it? Moreover, .then callback wasn't invoked at all.

@koba04
Copy link
Contributor

koba04 commented Mar 20, 2018

@kazagkazag Because .then always returns a new Promise object regardless whether passing functions as arguments.

You can try it.

Promise.resolve().then().then().then(() => console.log('1'));
Promise.resolve().then().then(() => console.log('2'));
Promise.resolve().then(() => console.log('3'));

@savv
Copy link

savv commented May 11, 2018

One hacky solution for now, is to flush all promises, as explained here:
jestjs/jest#2157 (comment)

@ljharb
Copy link
Member

ljharb commented Jun 28, 2018

That isn't a valid solution.

The proper solution is, whatever you're doing in your code that creates a promise - you'll have to get access to that promise in tests so that you can wait until it's resolved.

@ljharb ljharb closed this as completed Jul 7, 2018
@m-architek
Copy link

Problem is that componentDidMount function is called automatically on mount, so it is not possible to access its return value.

Two others possible solutions:

  1. Assign promise to field of the class:
class App extends Component {
  busy;

  componentDidMount() {
    this.busy = callApi().then(state => this.setState(state));
  }
}

it('should work', async () => {
     callApi.mockReturnValue(Promise.resolve(...));
     wrapper = shallow(<App  />);
     await wrapper.instance().busy;
   // state should be updated at this moment
});
  1. Wait for promise created after promise you wait for:
class App extends Component {

  componentDidMount() {
    callApi().then(state => this.setState(state));
  }
}

it('should work', async () => {
     callApi.mockReturnValue(Promise.resolve(...));
     wrapper = shallow(<App  />);
     await Promise.resolve();
   // state should be updated at this moment
});

@philipyoungg
Copy link

philipyoungg commented Oct 10, 2018

@m-architek interesting result. Could you share to us why the second example (async) works without having reference to the Promise(s) return value?

This is result of what I tried in Chrome console

// notice that no promises is returned
const runPromises = () => {
  Promise.resolve().then().then().then(() => console.log('1'));
  Promise.resolve().then().then(() => console.log('2'));
  Promise.resolve().then(() => console.log('3'));
  Promise.reject().then(() => console.log('success')).catch(() => console.log('rejected'));
}
const xxx = async() => {
  runPromises()
  await Promise.resolve()
  console.log('last')
}



xxx()

3
2
rejected
1
last

const yyy = () => Promise.resolve(runPromises()).then(() => console.log('last'))
yyy()

3
last
2
rejected
1

@koba04
Copy link
Contributor

koba04 commented Oct 11, 2018

Interesting. The behavior is only in console panels, which include Chrome, Firefox and Safari.
Running it as a script, the result is different, of which await doesn't wait to resolve promises.
https://codepen.io/koba04/pen/XxRqoZ?editors=0011

@m-architek
Copy link

@philipyoungg I understand it that way:
Every Promise is async, so it goes on queue and wait for event loop to process it. So promises would be resolved in order of creation (ofcourse if thay not contains any other async operation).

I must say I forgot about that each then clause creates new promise, so my second solution will work only when number of then clauses in tests match number of then cloasues in code that is tested.
Anyway, I've always prefered first solution, it is much more clearer.

@dgrcode
Copy link

dgrcode commented Nov 14, 2018

What do people think about this approach?

export class MyComponent extends React.Component {
  constructor (props) {
    super(props)

    this.hasFinishedAsync = new Promise((resolve, reject) => {
      this.finishedAsyncResolve = resolve
    })
  }

  componentDidMount () {
    this.doSomethingAsync()
  }

  async doSomethingAsync () {
    try {
      actuallyDoAsync()
      this.props.callback()
      this.finishedAsyncResolve('success')
    } catch (error) {
      this.props.callback()
      this.finishedAsyncResolve('error')
    }
  }

  // the rest of the component
}

And in the tests:

it(`should properly await for async code to finish`, () => {
  const mockCallback = jest.fn()
  const wrapper = shallow(<MyComponent callback={mockCallback}/>)

  expect(mockCallback.mock.calls.length).toBe(0)

  await wrapper.instance().hasFinishedAsync

  expect(mockCallback.mock.calls.length).toBe(1)
})

I had an issue when the async call was not done straight in componentDidMount, but it was calling an async function, that was calling another async function and so on. If I added an extra async step in all the async chain, I'd need to add an extra .then() or an extra await, but this is working just fine.

I wonder if there's a reason why I shouldn't be using this approach or if this looks good to people.

@ljharb
Copy link
Member

ljharb commented Nov 15, 2018

@dgrcode it seems like it works, but it adds code to your production implementation solely for testing, which imo is always a bad practice.

@pietrofxq
Copy link

pietrofxq commented Nov 28, 2018

Another solution:

class App extends Component {
  state = { error: false }
  componentDidMount() {
    callApi().then(state => this.setState(state)).catch(() => this.setState({ error: true });
  }
 render() {
    if (this.state.error) return <ErrorComponent />
  }
}

const waitForAsync = () => new Promise(resolve => setImmediate(resolve))

it('should work', async () => {
     callApi.mockImplementationOnce(() => new Promise((r, reject) => reject());
     const wrapper = shallow(<App  />);
     await waitForAsync()
     wrapper.update()
     expect(wrapper.find('ErrorComponent').exists()).toBeTruthy()
});

@prawn-cake
Copy link

Another solution is to wrap the check into setTimeout(() => { ... }, 0) with done callback.
In my case I have multiple calls in the componentDidMount and I need to check the state if one of the responses is 404

    it('should create an empty object when the second API call returns 404', done => {
        fetchMock
            .once(JSON.stringify({ items: [] }), { status: 200 })
            .once(JSON.stringify({}), { status: 404 });

        const wrapper = shallow(<App  />);

        setTimeout(() => {
            const state = wrapper.state();
            expect(Object.keys(state.myObject)).toEqual(['start', 'end']);
            done();
        }, 0);
    });

By setting the timeout we postpone our checks to the moment when all other promises (within componentDidMount) are resolved.

@ljharb
Copy link
Member

ljharb commented Apr 8, 2019

That’s not actually ensuring that though; it’s a race condition.

@prawn-cake
Copy link

prawn-cake commented Apr 8, 2019

It may look like a race condition.
According to my limited JS concurrency model understanding before being handled by the event loop, tasks go to the task FIFO queue wherefrom are being handled by the loop, so when we defer something (using network call or setTimeout) it goes to the queue first.

Given the fact that no real network calls involved here, I assumed that tasks from the componentDidMount (mocked network calls) will be in the task queue by the time setTimeout is called.

My assumptions could be wrong.

UPD: https://developer.mozilla.org/en-US/docs/Web/JavaScript/EventLoop

@ljharb
Copy link
Member

ljharb commented Apr 8, 2019

@prawn-cake while it's certainly possible for fetchMock to be set up so that you're correct, it's not a good idea to rely on that in code, for clarity even if not for correctness. It'd be better to wait on Promise.all of the relevant promises.

@quantuminformation
Copy link

quantuminformation commented Apr 30, 2019

If I set some props which will then cause state to be updated (via didComponentUpdate), how can I wait for the state to be updated before testing the UI?

The following doesn't work

wrapper.setProps({ downtimes: [downTimeMock] }, () => {
      console.log(wrapper.debug())
    })

However, if I set the state directly then the callback works, but want to avoid this.

@ljharb
Copy link
Member

ljharb commented Apr 30, 2019

There’s not a good mechanism because react doesn’t expose one, I’m afraid.

@mlodato517
Copy link

I just came across this and am interested in the community's opinion on:

class App extends Component {
  state = { ... }
  async componentDidMount() {
    this.setState({ loading: true })
    const foo = await bar()
    this.setState({ loading: false, ...foo })
  }
  render() {
    ... // render spinner if loading, else other stuff or whatever
  }
}

it('should work', () => {
     const spy = jest.spyOn(whateverModule, 'bar')
     const app = new App(props)
     app.setState = jest.fn()
     app.componentDidMount.then(() => expect(spy).toHaveBeenCalled())
});

I'm guessing it's bad taste to new up the component and probably doesn't work with functional components but just interested in what opinions people have on this style in this case.

In our case, we were testing that the component was starting a spinner, loading some data, and stopping the spinner. This test locks that functionality to the componentDidMount hook, but if we can trust that React will call componentDidMount, then it seems potentially reasonable to test the function directly?

@ljharb
Copy link
Member

ljharb commented Jun 20, 2019

@mlodato517 both using new and spying on setState are practices i would strenuously avoid.

You should use mount to test that. You can also await wrapper.state(‘foo’) in your test, it seems.

@mlodato517
Copy link

mlodato517 commented Jun 20, 2019

Yeah the spy on setState was to stop react from complaining because it wasn't mounted (which was probably a smell to indicate I shouldn't have been doing it haha).

That makes sense then, didn't know you could do that! I saw the await wrapper.instance().busy; above but didn't understand it well enough to think to use it. Thanks!

EDIT:
Sorry, my example was actually incorrect, we have

  async componentDidMount() {
    this.setState({ loading: true })
    const foo = await bar()
    if (foo) { doOtherStuff() }
    this.setState({ loading: false })
  }
}

so I don't think we can await any state fields? How does await state fields work ...?

EDIT 2:
Just saw this so I think we're going to try and mock out bar() to return a promise and then await that ...

EDIT 3:
Yup, creating a promise, mocking bar to return it, and then awaiting that promise before testing worked! We aren't 100% sure if mocking out bar to test things that aren't bar make sense (the test gets a little misleading) but at least now we have two solutions and can try our best judgement moving forward :-)

@Markkop
Copy link

Markkop commented Jul 20, 2019

This topic has a huge amount of valuable information. Is there any newer approach for this?

@mrdulin
Copy link

mrdulin commented Nov 14, 2019

I hit the same issue. None of above ways work for my case. I am using the
method of #1587 (comment) , trying to flush the promises.

import React, { Component } from 'react';
import svc from './contants/svc';

class MFASection extends Component<any, any> {
  constructor(props) {
    super(props);
    this.state = {
      enabledMFA: true
    };
  }
  componentDidMount() {
    svc.getMe().then(res => {
      console.log(res);
      this.setState({ enabledMFA: res.data.mfa_enabled });
    });
  }
  render() {
    return <div>enabledMFA: {this.state.enabledMFA}</div>;
  }
}

export default MFASection;
import React from 'react';
import { shallow } from 'enzyme';
import MFASection from '.';
import svc from './contants/svc';
function flushPromises() {
  return new Promise(resolve => setImmediate(resolve));
}

describe('MFASection', () => {
  test('molecules/MFASection mounts', async () => {
    const getMeSpy = jest.spyOn(svc, 'getMe').mockResolvedValueOnce({ data: { mfa_enabled: false } });
    const wrapper = shallow(<MFASection></MFASection>);
    expect(wrapper.exists()).toBe(true);
    expect(wrapper.state('enabledMFA')).toBeTruthy();
    await flushPromises();
    expect(wrapper.text()).toBe('enabledMFA: false');
    expect(getMeSpy).toBeCalledTimes(1);
  });
});
Unit test result
  FAIL  src/stackoverflow/58648463-todo/index.spec.tsx
MFASection
  ✕ molecules/MFASection mounts (30ms)

● MFASection › molecules/MFASection mounts

  expect(received).toBe(expected) // Object.is equality

  Expected: "enabledMFA: true"
  Received: "enabledMFA: "

    14 |     expect(wrapper.state('enabledMFA')).toBeTruthy();
    15 |     await flushPromises();
  > 16 |     expect(wrapper.text()).toBe('enabledMFA: false');
       |                            ^
    17 |     expect(getMeSpy).toBeCalledTimes(1);
    18 |   });
    19 | });

    at src/stackoverflow/58648463-todo/index.spec.tsx:16:28
    at step (src/stackoverflow/58648463-todo/index.spec.tsx:33:23)
    at Object.next (src/stackoverflow/58648463-todo/index.spec.tsx:14:53)
    at fulfilled (src/stackoverflow/58648463-todo/index.spec.tsx:5:58)

console.log src/stackoverflow/58648463-todo/index.tsx:13
  { data: { mfa_enabled: false } }

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        4.207s, estimated 12s

I expected the state of component should update and the mfa_enabled of state should be updated to false.

@ljharb
Copy link
Member

ljharb commented Nov 14, 2019

@mrdulin in your case, instead of "flushing promises", you should spy on svc.getMe and get at the promise inside your tests, and then wait until that's resolved.

@mrdulin
Copy link

mrdulin commented Nov 14, 2019

@ljharb Do you mean like this?

Solution 1:

import React from 'react';
import { shallow } from 'enzyme';
import MFASection from '.';
import svc from './contants/svc';

describe('MFASection', () => {
  test('molecules/MFASection mounts', done => {
    const mRepsonse = { data: { mfa_enabled: false } };
    let successHandler;
    const getMeSpy = jest.spyOn(svc, 'getMe').mockImplementation((): any => {
      const mThen = jest.fn().mockImplementationOnce((onfulfilled: any): any => {
        successHandler = onfulfilled;
      });
      return { then: mThen };
    });
    const wrapper = shallow(<MFASection></MFASection>);
    expect(wrapper.exists()).toBe(true);
    expect(wrapper.state('enabledMFA')).toBeTruthy();
    successHandler(mRepsonse);
    expect(wrapper.text()).toBe('enabledMFA: 2');
    expect(getMeSpy).toBeCalledTimes(1);
    done();
  });
});

But it still does not work.

Unit test result
FAIL  src/stackoverflow/58648463-todo/index.spec.tsx (11.897s)
MFASection
 ✕ molecules/MFASection mounts (35ms)

● MFASection › molecules/MFASection mounts

 expect(received).toBe(expected) // Object.is equality

 Expected: "enabledMFA: false"
 Received: "enabledMFA: "

   23 |     successHandler(mRepsonse);
   24 |     wrapper.update();
 > 25 |     expect(wrapper.text()).toBe('enabledMFA: false');
      |                            ^
   26 |     expect(getMeSpy).toBeCalledTimes(1);
   27 |     done();
   28 |   });

   at Object.<anonymous> (src/stackoverflow/58648463-todo/index.spec.tsx:25:28)

console.log src/stackoverflow/58648463-todo/index.tsx:13
 { data: { mfa_enabled: false } }

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        13.936s, estimated 14s

UPDATE

Sorry, it works. My fault. I forget that react will not render a primitive boolean value. After I change the JSX to <div>enabledMFA: {this.state.enabledMFA ? '1' : '2'}</div>; and

expect(wrapper.text()).toBe('enabledMFA: false');

to

expect(wrapper.text()).toBe('enabledMFA: 2');

the test passes.

Solution 2: use setImmediate works as well.

test('molecules/MFASection mounts - 2', done => {
    const mRepsonse = { data: { mfa_enabled: false } };
    const getMeSpy = jest.spyOn(svc, 'getMe').mockResolvedValueOnce(mRepsonse);
    const wrapper = shallow(<MFASection></MFASection>);
    expect(wrapper.exists()).toBe(true);
    expect(wrapper.state('enabledMFA')).toBeTruthy();
    setImmediate(() => {
      expect(wrapper.text()).toBe('enabledMFA: 2');
      done();
    });
    expect(getMeSpy).toBeCalledTimes(1);
  });

Unit test result:

PASS  src/stackoverflow/58648463-todo/index.spec.tsx (9.834s)
  MFASection
    ✓ molecules/MFASection mounts (19ms)
    ✓ molecules/MFASection mounts - 2 (4ms)

  console.log src/stackoverflow/58648463-todo/index.tsx:13
    { data: { mfa_enabled: false } }

  console.log src/stackoverflow/58648463-todo/index.tsx:13
    { data: { mfa_enabled: false } }

Test Suites: 1 passed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        11.154s

Source code of the example: https://github.com/mrdulin/jest-codelab/tree/master/src/stackoverflow/58648463

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests