Add promise support to all the matchers

[excitement-engineer]

This PR addresses issue #1377 and adds promise support to all the existing matchers in jest.

This allows us to run assertions on promises by adding the keywords resolves and rejects after the expect statement as follows:

it('should reject', () => {
    return expect(Promise.reject(4)).rejects.toBe(4);
});
it('should resolve', () => {
   return expect(Promise.resolve(4)).resolves.toBe(4);
});

It can also easily be used in combination with async/await:

it('should reject', async () => {
   await expect(Promise.reject(4)).rejects.toBe(4);
   await expect(Promise.reject(4)).rejects.not.toBe(5);
});
it('should resolve', async () => {
   await expect(Promise.resolve(4)).resolves.toBe(4);
   await expect(Promise.resolve(4)).resolves.not.toBe(5);
});

I would love to hear some feedback on this API!

Other API Design Choices

@julien-f I also considered the proposal you made in your gist. You proposed the following API:

it('should resolve', async () => {
   await expect(Promise.resolve(4)).toResolve().toBe(4);
});

As far as I can tell the API in your proposal doesn't work unfortunately, we must first await for the toResolve() before we can run assertions on it. When I tried to implement it I found that the API would look as follows in reality:

it('should resolve', async () => {
   (await expect(Promise.resolve(4)).toResolve()).toBe(4);
});

The above code snippet seemed less intuitive and clean to me. That is why I decided to go with the API I proposed above.

TBD: Add a matcher for checking if promise rejects/resolve:

I think there is a place for a matcher that simply checks whether a promise resolves/rejects similar to the one proposed by @julien-f! This is not something that I implemented in this PR, but just an idea.

it('should resolve', () => {
   return expect(Promise.resolve(4)).toResolve();
});

it('should reject', () => {
   return expect(Promise.reject(4)).toReject();
});

Docs

I have not updated the docs yet. First I would like to get some feedback on the API:)

[cpojer]

it seems quite repetitive to receive all the keys every time. Can we avoid it and only iterate over them once?

[excitement-engineer]

Good point! I fixed this:)

[cpojer]

This is really super awesome. I didn't expect that we could find such an elegant solution not just for toThrow but for all matchers. Nice!

We should iterate on this and ship it. One thing I noticed is that this implementation does more work than what we do currently. Could we potentially turn .resolves and .rejects into a lazy getter or a function (.resolves()?) so that we do all this work only when requested? Note that the function body for expect really does a lot of work for every single call to it. If we could optimize this, I think that may help with Jest's performance a bit :)

Overall I wish there was an easier way to support this API. I'd love to be able to simply do await expect(foo).toThrow(…) but we cannot know upfront whether a function was supposed to be invoked synchronously or asynchronously, of course. resolves and rejects aligns nicely with the Promise API and is probably better than resolves and catches.

For toResolve – I'm not opposed to it but it seems to me like your diff makes that proposal obsolete. We'll have all matchers available rather than checking whether a promise resolves or rejects. Is it valuable to know whether a promise resolves/rejects without testing for any other data? It seems similar to me like toThrow() without arguments, which arguably is a bad API that we inherited from Jasmine.

Before we go further into this, here are some proposals of what the API could look like:

• await expect(banana).resolves.toBe(…) – your proposal.
• await expect(banana).resolvesToBe(…) – this looks less like chai. I may actually prefer this one.
• await expect(banana).resolves().toBe(…) – explicit function call.
• await expect(banana).<insertProposalHere>.toBe(…) – is there anything else we can come up with?

What do you think?

[julien-f]

@excitement-engineer Thank you very much for your work :)

Maybe .toResolve() is a bit harder to build than .resolve with current architecture but IMHO it would be much clearer that it changes the behavior and it could be used by itself.

@cpojer As said in my proposal, I find .toResolve().toBe() to be much clearer and composable.

[cpojer]

I think when I'm reading it, toResolveToBe is more confusing than resolvesToBe.

[julien-f]

I disagree, everywhere full infinitives are used instead of conjugated at the third person, IMHO, it should stay the same.

And toResolve() can be used by itself if the resolved value is not tested (like .not.toThrow()).

[cpojer]

Hm, unsure about .not.resolvesToBe, I think that works as well? I may need to pull in a native speaker to help us out with this :D

[excitement-engineer]

I am glad that you guys like the initial proposal!

@cpojer

Could we potentially turn .resolves and .rejects into a lazy getter or a function (.resolves()?) so that we do all this work only when requested? Note that the function body for expect really does a lot of work for every single call to it. If we could optimise this, I think that may help with Jest's performance a bit :)

Good point! We are effectively loading in all the matchers 3 times on every expect call, I agree that this can be optimised further;) Thanks for the tip! I like the idea of adding a lazy getter here, keeps it as simple as possible.

For toResolve – I'm not opposed to it but it seems to me like your diff makes that proposal obsolete. We'll have all matchers available rather than checking whether a promise resolves or rejects. Is it valuable to know whether a promise resolves/rejects without testing for any other data? It seems similar to me like toThrow() without arguments, which arguably is a bad API that we inherited from Jasmine.

Good point! We could ship without this API. If there are people with a serious use case for simply checking whether a Promise resolves/rejects then we can consider adding it again, but the simpler jest remains the better of course!

Before we go further into this, here are some proposals of what the API could look like:

await expect(banana).resolves.toBe(…) – your proposal.
await expect(banana).resolvesToBe(…) – this looks less like chai. I may actually prefer this one.
await expect(banana).resolves().toBe(…) – explicit function call.
await expect(banana)..toBe(…) – is there anything else we can come up with?

I think there may be a potential issue with resolvesToBe(…) when calling its negation, i.e. what would not.resolvesToBe() mean, there may be 2 possible interpretations:

• The Promise should not resolve
• The Promise should resolve but not be the specified value.

Secondly, I don't think it's possible to add some lazy getter in the API resolvesToBe, resolveToEqual etc. Personally I am in favour of sticking with the current API and introducing a lazy getter to make the code more performant:) What do you think?

@julien-f Thanks again for your gist, it served as inspiration for this PR, awesome work!

Maybe .toResolve() is a bit harder to build than .resolve with current architecture but IMHO it would be much clearer that it changes the behaviour and it could be used by itself.

I agree with @cpojer here, toResolve.toBe is more confusing than resolves.toBe. In addition, as I mentioned in my initial comment, I think that the API:

(await expect(Promise.resolve(4)).toResolve()).toBe(4);

is harder to use and less intuitive than:

await expect(Promise.resolve(4)).resolves.toBe(4);

[julien-f]

@excitement-engineer Of course the second example is much harder than the first one, but what I suggest is simply to have the same behavior that you implemented but instead of creating resolve matchers in expect, they are created on demand by the toResolve() function.

And to avoid recreating those matchers, we would simply need to create them once and for all on a prototype and just create a new instance when necessary:

return Object.create(asyncMatchers, { actual: { value: actual } })

@cpojer I'm not a native English speaker but I think “expect something resolves to be” is incorrect unlike “expect something to resolve to be”.

[alisnic]

for perfect readability, I'd go with:

// expectation can fail due to 2 different reasons
//   promise was not resolved
//   promise was not resolved with expected value
await expect(Promise.resolve(4)).toBeResolvedWith(4)

// same logic as above applies
await expect(Promise.reject(4)).toBeRejectedWith(4)

But I agree with @cpojer:

Personally I am in favour of sticking with the current API and introducing a lazy getter to make the code more performant:)

My proposal increases the API surface, since separate matchers are required for both assertions and negations

[robinpokorny]

I'd say there are two things we try to solve here, as @excitement-engineer notes:

• If the promise was resolved or rejected.
• What the value of a resolved promise or the reason of a rejected promise is.

Why not to separate it?

For the first, we can have simple matcher:

await expect(Promise.resolve(4)).toBeResolved()
await expect(Promise.reject(4)).toBeRejected()

For the latter, there can be an aforementioned lazy getter which just unwraps the value or reason. (IMHO I like whenResolved but resolves is OK, too)

await expect(Promise.resolve(4)).whenResolved.toBe(4)
await expect(Promise.reject(4)).whenRejected.toBe(4)

Also, I would be inclined to make the test fail when the getter is not called. But the opposite could make sense, too.

// fails (?)
await expect(Promise.reject()).whenResolved.toBe(4)

Then the not would be allowed only after the getter:

// good
await expect(Promise.resolve(4)).whenResolved.not.toBe(4)
// bad
await expect(Promise.resolve(4)).not.whenResolved.toBe(4)

---

If we used toBeResolvedWith we would lose some nice use cases like:

await expect(Promise.resolve(4)).whenResolved.toMatchSnapshot()

[robinpokorny]

A small note that might be taken into account about naming:

There is a difference between ‘resolved’ and ‘fulfilled’ even though they are used interchangeably in everyday talk.

Cf. https://github.com/domenic/promises-unwrapping/blob/master/docs/states-and-fates.md

[Phoenixmatrix]

Just a devil's advocate-ish thought (and I already probably know the answer, but I figure I'd post it anyway, hehe).

Is there a huge gain to chaining promises into assertions in a world where you can just await the promise and run expectations on the result on a separate line? Especially since even with this API you have to await the promise anyway?

Is the 1 line save that big?

[julien-f]

@Phoenixmatrix the real gain is for rejection :) (#1377 (comment))

[Phoenixmatrix]

fair enough :)

[quantizor]

How about the word yields for both?

expect(assertion).yields.{suffix}?

[excitement-engineer]

When it comes to english I thnk that the expect(promise).resolves.toBe(4) is ok!

If I put it in a sentence it seems fine to me

• I expect it resolves to be 6
• I expect is resolves to be greater than 6
• I expect it rejects to match the snapshot

@julien-f

what I suggest is simply to have the same behaviour that you implemented but instead of creating resolve matchers in expect, they are created on demand by the toResolve() function.

I actually implemented this initially! I was able to write the following code just like your proposal:

await expect(promise).toResolve().toBe();

However, I then tried to run the following code and found that it didn't work:

await expect(promise).toResolve()

When we try to make toResolve() composable with all the matchers we first need to evaluate whether the promise resolves i.e. await expect(promise).toResolve() before we can run the matchers on it: (await expect(promise).toResolve()).toBe(...).

This is why I decided to forego this API and went with expect(promise).resolves.toBe().

@robinpokorny

Thanks for your comment! you highlighted the issue we are trying to solve really well:)

@probablyup

How about the word yields for both?
expect(assertion).yields.{suffix}?

Unfortunately such an API would not allow us to distinguish between promises that reject and resolve!

[cpojer]

Thanks everyone for your input.

@Phoenixmatrix you are actually making a good point. What is the incremental value that is added when doing something with one of the more complex matchers. For example returning a mock function from a Promise and calling

await expect(Promise.resolve(jest.fn())).resolves.toBeCalledWith('foo')`
// vs
const result = await Promise.resolve(jest.fn());
expect(result).toBeCalledWith();

And the answer is probably: not that much. However, as julien-f pointed out, this is much more useful when something is supposed to throw. If we are adding helpers to make it easier to check whether promises fail it doesn't seem like it increases the API surface too much to add checks for proper resolution.

@excitement-engineer I still prefer resolvesToBe() etc. better. That's the last bit to iron out. I think perf should be fine either way – one thing I was thinking is we could move the Object.keys(allMatchers) call to expect.extend because that's the only time it can change. That should pretty much normalize performance in the average case here.

Your argument about .not.resolvesToBe makes sense, which is probably why we should go with your proposal. However, I'm not entirely sure whether .resolves.not.toBe(…) makes sense either. Can you explain when I would write a test like that? I'm wondering if we should even get rid of the negated matchers from resolves/rejects but that of course also means that we'd definitely go with the dot-syntax proposal.

Please make the CI pass, too :) (don't worry in case you see a segfault, that's a node issue, but everything else should be green).

As a follow-up PR, it would be awesome to add documentation for this feature to the Expect API docs page.

What do you think?

[suchipi]

What about:

expect.resolution(Promise.resolve(5)).toBe(5);
expect.rejection(Promise.reject(4)).toBe(4);
expect.fulfillment(Promise.resolve(3)).toBe(3); // for either resolved or rejected

This feels like more natural English than "expect resolves to be", to me.

[excitement-engineer]

@cpojer

Your argument about .not.resolvesToBe makes sense, which is probably why we should go with your proposal. However, I'm not entirely sure whether .resolves.not.toBe(…) makes sense either. Can you explain when I would write a test like that? I'm wondering if we should even get rid of the negated matchers from resolves/rejects but that of course also means that we'd definitely go with the dot-syntax proposal.

Something I do quite often is checking that the resolved promise is not null for example. It would be nice to include the negation statement for these use cases i.e. expect(promise).resolves.not.toBeNull(). In addition, these are some other use cases that could occur:

it("does something", async () => {
await expect(Promise.resolve(jest.fn())).resolves.not.toBeCalled()
});

it("successfully calls login API", async () => {
await expect(loginAPICall).resolves.not.toHaveProperty("auth_error");
});

However, in general I think that the use cases for .not in promises (asynchronous) are very similar to the use cases for synchronous value; in both cases we are just running assertions on values. Therefore, I'm not sure that restricting the assertions to not include .not would be a good idea.

Do you agree with my reasoning @cpojer?

I will update the PR this weekend, introduce the lazy getter and make sure that the CI passes.

Thanks a lot everyone for your feedback and help:)

[robinpokorny]

Is ‘resolved’ the word we want to use?

I know I posted this here already but I feel it was ignored. This one is mainly my conscience.

A Promise is in one of these states:

pending: initial state, not fulfilled or rejected.
fulfilled: meaning that the operation completed successfully.
rejected: meaning that the operation failed.

A promise is said to be:
settled: if it is either fulfilled or rejected, but not pending.
resolved: the promise is settled, or it is locked into a promise chain.

~ Promise on MDN (definition of settled and resolved taken from the note)

cf. Promise Objects in ECMAScript 2015 Language Specification

[cpojer]

@robinpokorny I think this comes mostly from the executor of the Promise function having arguments commonly called "resolve" and "reject". I think it makes sense to stick with what people use colloquially even if it isn't 100 % technically correct.

[julien-f]

IMHO, after thinking I think it would be best to use fulfilled instead of resolved (like Bluebird), because resolved meaning is a bit complicated (jamiebuilds/proposal-promise-prototype-inspect#1 (comment)) and might be confusing for some people.

[robinpokorny]

@cpojer, in the executor the naming is correct. ‘Fullfill’ is used e.g. in description of Promise.prototype.then: promise.then(onFulfilled [, onRejected]);.

As said, I just want this matter to be discussed. Of course, an alias could solve it later.

[excitement-engineer]

@cpojer

Could we potentially turn .resolves and .rejects into a lazy getter or a function (.resolves()?) so that we do all this work only when requested? Note that the function body for expect really does a lot of work for every single call to it. If we could optimise this, I think that may help with Jest's performance a bit :)

I did some investigation here and implemented the lazy getter to compare it to the current implementation. I implemented the lazy getter as follows:

const expect: Expect = (actual: any): ExpectationObject => {
  const allMatchers = global[GLOBAL_STATE].matchers;
  const expectation = {
    not: {},
    rejects: {not: {}},
    resolves: {not: {}},
  };

  Object.keys(allMatchers).forEach(name => {
    expectation[name] =
      makeThrowingMatcher(allMatchers[name], false, actual);
    expectation.not[name] =
      makeThrowingMatcher(allMatchers[name], true, actual);
  });

  Object.defineProperties(expectation, {
    rejects: ({
      get: () => {
        const rejectExpectation = {
          not: {},
        };
        Object.keys(allMatchers).forEach(name => {
          rejectExpectation[name] =
           makeRejectMatcher(name, allMatchers[name], false, actual);
          rejectExpectation.not[name] =
           makeRejectMatcher(name, allMatchers[name], true, actual);

        });
        return rejectExpectation;
      },
    }: Object),
    resolves: ({
      get: () => {
        const resolveExpectation = {
          not: {},
        };
        Object.keys(allMatchers).forEach(name => {
          resolveExpectation[name] =
            makeResolveMatcher(name, allMatchers[name], false, actual);
          resolveExpectation.not[name] =
            makeResolveMatcher(name, allMatchers[name], true, actual);
        });
        return resolveExpectation;
      },
    }: Object),
  });

  return expectation;
};

In order to check whether it was actually more efficient I ran the following test using both the current and "lazy" implementation:

describe('performance', () => {

  beforeAll(() => {
    console.time('perf');
  });

  afterAll(() => {
    console.timeEnd('perf');
  });

  for (let i = 0; i < 10000; i++) {
    it('tests', () => {
      jestExpect(4).toBe(4);
      jestExpect(6).toBeGreaterThan(5);
      jestExpect({bla: 5}).toEqual({bla: 5});
    });
  }
});

These were the results that I got:

• Without resolves/rejects in expect: 11597.401ms
• Current implementation: 12866.832ms
• Lazy implementation: 15235.753ms

The first thing to note is that the added runtime of adding the resolves and rejects in expect is very small (about 1 second). Is it an idea to keep the implementation the way it currently is because the overhead is negligible, what do you think?

It turns out that my "lazy" implementation is actually slower! Anyone notices a mistake in my implementation of the lazy getter or has some way of improving perf?

I also ran into an unexpected result when running the same tests in an asynchronous "it" function:

describe('async performance', () => {

  beforeAll(() => {
    console.time('perf');
  });

  afterAll(() => {
    console.timeEnd('perf');
  });

  for (let i = 0; i < 10000; i++) {
    it('tests', async () => {
      jestExpect(4).toBe(4);
      jestExpect(6).toBeGreaterThan(5);
      jestExpect({bla: 5}).toEqual({bla: 5});
    });
  }
});

These were the results for the async test:

• Without resolves/rejects in expect: 4711.577ms
• Current implementation: 5142.480ms
• Lazy implementation: 6215.790ms

This change cut the execution time down by more than 50%! Just an unexpected result that I thought I'd share;)

[excitement-engineer]

@cpojer

I don't really understand why circleci is not running. The terminal output contains an error that I don't understand:

Can somebody explain how to fix this:)

[excitement-engineer]

@cpojer @julien-f @robinpokorny

I think this comes mostly from the executor of the Promise function having arguments commonly called "resolve" and "reject". I think it makes sense to stick with what people use colloquially even if it isn't 100 % technically correct.

I agree with this! I think it makes a more sense to follow the promise API, which contains the words resolve and reject but does not contain the word fulfilled anywhere. I think that the API will become less intuitive for people if we start using fulfilled. If you see the words reject/resolve then it is clear to everybody that we are talking about promises.

[thymikee]

I guess this is some bug with transform-es2015-destructuring and it doesn't transform some spread operators. You can use .apply(args) instead.

[excitement-engineer]

Thanks for the help! I have updated the PR:) Let's hope CI runs now

[excitement-engineer]

Is there an issue open for this in babel? We should add it if hasn't been done already.

[excitement-engineer]

I had to update one of the snapshots in an integration test. Can somebody review this change please?

[excitement-engineer]

The CI is passing! Thanks for your help @thymikee:)

[cpojer]

Ok, let's add this to Jest. Everyone, thanks for weighing in here. We are going with the original proposal but I'm looking forward to pointing people to this PR when they ask about this API and the decisions we made.

@excitement-engineer thanks for building this. Would you mind sending a PR to add all of this to the docs?

[excitement-engineer]

Thanks a lot everyone!

No problem @cpojer! Expect a PR from me this week:)

[lennerd]

This is a great feature indeed. The only and very confusing part is the comment in the doc. Would love to use this new API changes in my tests but I don't see any hints about Jest version 20, although the docs are talking about it already. Anyone who can help?

[suchipi]

Same issue (feature was in docs before release with no version hint) occurred in #3188; it should be easy to fix, though personally I feel like a change in process would be a better way to address this.

[ljharb]

Could we please not use getters? .resolves() and .rejects() is much cleaner, much easier to lint against, MUCH more performant (getters and setters are slow), and much more readable.

[robinpokorny]

@ljharb This PR has been merged. If you want to improve/change it, I guess it would be best to create a new PR with proposed code changes.

[ljharb]

@robinpokorny I was asked for a suggestion here. I'm afraid I don't have the time to make a PR, but hopefully that won't stop Jest collaborators from improving the code in this PR prior to a release.

[robinpokorny]

@ljharb, I see 😉

Do I understand it right that you propose to use the following API?

await expect(banana).resolves().toBe(…)

[ljharb]

@robinpokorny yes, if you don't just want to do expect(await banana).toBe(…), which wouldn't have required any changes to jest.

[robinpokorny]

@ljharb The real benefits of changes in this PR are vastly about rejections, as mentioned here: #3068 (comment).

expect(await banana).toBe(…) does produce terrible error messages when banana is rejected.

[ljharb]

Fair enough :-) then yes, return expect(banana).rejects().toBe(…) would be great.

[cpojer]

@ljharb would you mind explaining your argument a bit better? You said that function calls are:

• cleaner
• easier to lint against
• MUCH more performant (getters and setters are slow)
• more readable

than the current implementation. We aren't using getters/setters, we only benchmarked it, so that falls away. I assume that "cleaner" and "more readable" is both the same thing and is very subjective and your last argument is that it is easier to lint against, which I don't quite understand given that we are talking about a slight AST difference between the two. As I see it, the added parenthesis serve no value here. Can you elaborate?

[lennerd]

Same issue (feature was in docs before release with no version hint) occurred in #3188; it should be easy to fix, though personally I feel like a change in process would be a better way to address this.

Beside discussing the syntax (and I favor the property syntax btw.): any more info about the upcoming release date for v20? The current documentation is a little misleading.

[ljharb]

@cpojer I assumed based on the syntax that it was using getters (since everything in mocha/chai that looks like that does) - I do now see in this PR that you're creating an object "resolves" and "rejects" with all possible matchers on it, so my concerns indeed don't apply: "readable" is highly subjective, so I'll just withdraw that one rather than have a style debate here; "easier to lint against" applies to arbitrary getters but not to property chains.

Making it a function would avoid needing to create these objects in advance; and would allow you to avoid someone doing expect(Promise.reject()).resolves; and expecting something to work, and finding instead that their test incorrectly passes; whereas expect(Promise.reject()).resolves() certainly could work.

[drpicox]

What about:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 331): Error: expect(received).toBe(expected)

It does not look very intuitive.

[robinpokorny]

@drpicox It seems you did not return the promise or the assertion with .resolves from the test function. See the guide for examples: http://facebook.github.io/jest/docs/en/asynchronous.html

[drpicox]

Thanks!
I just have seen it here:

• Add promise support to all the matchers #3068 (comment)
• and here: https://facebook.github.io/jest/docs/en/expect.html#resolves

I do not know how I missed it before. Thanks!

I just got confused thinking that expect().resolves would talk with the test itself and make it wait until having the result. May be a warning or similar would be nice.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.