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

fix .toThrow for promises #4884

Merged
merged 4 commits into from
Nov 14, 2017
Merged

fix .toThrow for promises #4884

merged 4 commits into from
Nov 14, 2017

Conversation

lstkz
Copy link
Contributor

@lstkz lstkz commented Nov 13, 2017

fixes #3601

From docs

return expect(Promise.reject('octopus')).rejects.toBe('octopus');

In real app, an Error object must be rejected.
Fixed to

return expect(Promise.reject(new Error('octopus'))).rejects.toThrow('octopus');

fixes #3601

Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>
Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>
Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>
@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

Merging #4884 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4884   +/-   ##
=======================================
  Coverage   59.75%   59.75%           
=======================================
  Files         195      195           
  Lines        6533     6533           
  Branches        4        3    -1     
=======================================
  Hits         3904     3904           
  Misses       2629     2629

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 757f163...b492b59. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! Thanks for tackling it.

Could you update the changelog as well?

@@ -416,15 +416,15 @@ For example, this code tests that the promise rejects with reason `'octopus'`:
```js
test('rejects to octopus', () => {
// make sure to add a return statement
return expect(Promise.reject('octopus')).rejects.toBe('octopus');
return expect(Promise.reject(new Error('octopus'))).rejects.toThrow('octopus');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This old example is still valid, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(although it's not recommended to reject with a non-error)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it still works

Signed-off-by: Łukasz Sentkiewicz <sirmims@gmail.com>
@lstkz
Copy link
Contributor Author

lstkz commented Nov 14, 2017

@SimenB
changelog updated

});
```

Alternatively, you can use `async/await` in combination with `.rejects`.

```js
test('rejects to octopus', async () => {
await expect(Promise.reject('octopus')).rejects.toBe('octopus');
await expect(Promise.reject(new Error('octopus'))).rejects.toThrow('octopus');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should say that toThrow only works in jest 21.3.0+. People copy-paste examples, and having them not working is not a good UX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently, it mentions

### `.rejects`

##### available in Jest **20.0.0+**

Change it to 21.3.0+?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or a separate line describing rejecting promises and toThrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lsentkiewicz this came up in #4945, mind sending a PR with clarification? 🙂

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really painful way to discover why rejects.toThrow was not working on my jest@21.2 setup

@SimenB you were right, it is probably a good idea to have a mention to it in the docs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you send a PR fixing the docs?

@cpojer cpojer merged commit 38c2cf7 into jestjs:master Nov 14, 2017
@cpojer
Copy link
Member

cpojer commented Nov 14, 2017

Nice!

@Turbo87
Copy link

Turbo87 commented Nov 22, 2017

is there a new release planned including this fix? it seems impossible currently to correctly test for Promise rejections 🤔

@rraziel
Copy link

rraziel commented Nov 22, 2017

Shouldn't it be toThrowError rather than toThrow?
Or would both work with this pull request?

@lstkz
Copy link
Contributor Author

lstkz commented Nov 22, 2017

both methods work

@SimenB
Copy link
Member

SimenB commented Nov 22, 2017

@Turbo87 it's out as jest@test (beta 9, I believe)

sdunster added a commit to NSWSESMembers/availability-poc that referenced this pull request Dec 3, 2017
* Beta version because it has a bug fix for testing rejected promises:
  * jestjs/jest#4884
* Needed for upcoming unit tests for events/schedules API
sdunster added a commit to NSWSESMembers/availability-poc that referenced this pull request Dec 3, 2017
* Beta version because it has a bug fix for testing rejected promises:
  * jestjs/jest#4884
* Needed for upcoming unit tests for events/schedules API
@julienw julienw mentioned this pull request Dec 5, 2017
nodkz added a commit to nodkz/jest that referenced this pull request Sep 19, 2018
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rejects.toThrowError
8 participants