Update simulate() to return a promise

[milesj]

simulate() works great for handlers that are synchronous, but have timing issues for any handler that is async (like promises). Take the following for example:

class Wrapper extends React.Component {
  state = {
    success: false,
    error: null,
  };

  handleClick = () => {
    return this.props.onClick()
      .then(() => {
        this.setState({ success: true });
      })
      .catch((error) => {
        this.setState({ error });
      });
  };

  render() {
    return <Form onClick={this.handleClick} />;
  }
}

const wrapper = shallow(<Wrapper onClick={jest.fn(() => Promise.resolve())}) />);

wrapper.find(Form).simulate('click');

expect(wrapper.state('success')).toBe(true); // Doesn't work

Although the example is quite crude, the expect call may fail in specific conditions as the inner promise is firing off in a different tick. To resolve this issue, one must wrap the expect in a setTimeout, which is quite hacky.

setTimeout(() => {
  expect(wrapper.state('success')).toBe(true); // Works
}, 0); 

This change will update simulate() to return a promise, allowing this to easily work (plus promises are better).

return wrapper.find(Form).simulate('click').then(() => {
  expect(wrapper.state('success')).toBe(true); // Works
});

[ljharb]

Simulate is for events. Events are inappropriate for promises because a promise can only resolve once, whereas events can fire multiple times.

See #823.

[ljharb]

Note that if there's a way to make simulate take a callback that works for every event type, that'd be great, but since simulate invokes user code, there's no way to ensure we can ever get notified of its completion.

[milesj]

Good point. Forgot about that use case.

[audiolion]

so how do people test this type of code?

[ljharb]

If the implementation provides no way to do so (takes a callback or returns a promise) then it has successfully denied you the ability to correctly wait on the results.

[audiolion]

this is out of the scope of enzyme but could you provide a modified version of the above Wrapper component that would return a callback/promise to test against? Perhaps we could take that then and document somewhere for people who are asking this question.

If you don't have time, np, thanks for answering prior ?'s

[ljharb]

It's not about having time, that would be incorrect. If the implementation does not provide an async hook, none can exist.

[milesj]

That's a bit irrelevant. We need to trigger expect in the next tick, so wrapping simulate in a promise was the easiest (albeit, best solution), as simulate is the majority cause of problematic async issues. Even more so when fake timers are being used.

I'll also respond to your previous comment:

Simulate is for events. Events are inappropriate for promises because a promise can only resolve once, whereas events can fire multiple times.

While that's true for the DOM, we're testing against React by simply executing prop functions. At most, once per test. Furthermore, each simulate call will resolve its own promise, so that's still a non-issue (it even allows for promise chaining).

Since simulate invokes user code, there's no way to ensure we can ever get notified of its completion.

We only care about the next tick, not whether the event is successful or not. Leave it up to the consumer (e.g., us), to return values correctly.

If you're still worried about the DOM, then we can only support ShallowWrapper, and leave ReactWrapper as-is.

[ljharb]

All of your criteria/circumstances make perfect sense; but since you (the app developer) have that context, and not enzyme itself, you're the one who should be making that decision - ie, you should advance to the next tick yourself, either by returning a new Promise, or using the test's done callback in a setTimeout.

In other words, "at most, once per test" is absolutely not necessarily true for all enzyme users, so it's not an assumption enzyme can/should make.

[milesj]

The done argument is something that will be going away, so relying on that should be avoided.

The only solution we found so far is the following (which is far too much annoying bootstrap).

const promise = Promise.resolve();
const wrapper = shallow(<Wrapper onClick={jest.fn(() => promise)}) />);

wrapper.find(Form).simulate('click');

return Promise.resolve(promise).then(() => {
  expect(wrapper.state('success')).toBe(true); // Works
});

There comes a time when developer productivity and efficiency should be taken into account.

[ljharb]

(you don't need to rewrap promise in Promise.resolve there)

While you can guarantee that Form's onClick is calling Wrapper's onClick prior to the tick on which promise resolves (because you implemented it), enzyme can't. If enzyme added this feature, then it would only be useful for scenarios where the simulated function invokes the user code (in this case, the onClick jest spy) on the same tick - and there's no assurance that'd be the case even a majority of the time.

If, however, Form's onClick returned a promise - then instead of simulate, you could simply return wrapper.find(Form).prop('onClick')().then(() => { expect(…); }); without the promise wrapper nor an enzyme change.

In short, I think that simulate tends to appear attractive, but isn't actually that useful a thing to use in tests.

[milesj]

I've actually proposed calling the prop directly (wrapper.find(Form).prop('onClick')()), but have been shot down by others, as it doesn't test the entire accurate flow. Perhaps we can document some "best practices" or "guidelines" for specific scenarios like this.

I've also tinkered with extending ShallowWrapper before, which works for the most part, but a lot of internal enzyme functions aren't exported, so I basically hit a road block.

[kesne]

@ljharb To follow-up on Mile's point, I think the issue with calling the prop directly is that you loose the semantic meaning of simulate().

However, what if simulate() returned the return value of the prop function that gets invoked? That would retain the nice clean semantics, and provides a clear testing story:

const wrapper = shallow(<Component />);
const promise = wrapper.find('CloseButton').simulate('press');
return promise.then(() => {
  expect(wrapper.state('isClosed')).toBeTruthy();
});

[ljharb]

@milesj we should not be testing the entire accurate flow, because that's testing React. We don't test React, we only test our own code.

@kesne passing along the return value seems totally fine, since that allows you to return a promise explicitly and depend on it, if you so choose.