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

rejects/resolves does not fail the test on non-promise value #5361

Closed
alexilyaev opened this issue Jan 22, 2018 · 14 comments · Fixed by #5364
Closed

rejects/resolves does not fail the test on non-promise value #5361

alexilyaev opened this issue Jan 22, 2018 · 14 comments · Fixed by #5364

Comments

@alexilyaev
Copy link
Contributor

alexilyaev commented Jan 22, 2018

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
Passing a non-promise value to rejects or resolves does not fail the test

If the current behavior is a bug, please provide the steps to reproduce and
either a repl.it demo through https://repl.it/languages/jest or a minimal
repository on GitHub that we can yarn install and yarn test.

Test code

describe('someMethod', () => {
  it('should reject', () => {
    expect(123).rejects.toThrow('error');
  });
});

Also this one:

describe('someMethod', () => {
  it('should reject', () => {
    const fn = new Promise((resolve, reject) => {
      resolve(1234);
    });

    expect(fn).rejects.toThrow('error');
  });
});

Output

(node:93202) UnhandledPromiseRejectionWarning: Error: expect(received).rejects.toThrow()

received value must be a Promise.
Received:
  number: 123
(node:93202) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 3)
 PASS  app/remote-control/index.spec.js
  test
    someMethod
      ✓ should reject (1ms)

  console.warn node_modules/bluebird/js/release/debuggability.js:873
    Unhandled rejection error


Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        0.226s, estimated 1s

What is the expected behavior?
Should fail the test

Please provide your exact Jest configuration and mention your Jest, node,
yarn/npm version and operating system.

Node v9.3.0
Jest v22.1.2
Yarn v1.3.2
Mac OS X 10.12.6

@SimenB
Copy link
Member

SimenB commented Jan 22, 2018

@incleaf
Copy link
Contributor

incleaf commented Jan 22, 2018

Hi, I'd like to take a look and submit a PR for this issue.

@SimenB
Copy link
Member

SimenB commented Jan 22, 2018

Go for it!

@alexilyaev
Copy link
Contributor Author

alexilyaev commented Jan 22, 2018

Added another test code in the description that's not as expected

@SimenB
Copy link
Member

SimenB commented Jan 22, 2018

Note that the OP uses resolves/rejects incorrectly - it either has to be returned or awaited. See docs. If that is done, the test behaves as expected.

I still think we should give a proper synchronous error before switching into async mode, though.

There is no way for jest to detect this scenario (help welcome in the lint plugin: jest-community/eslint-plugin-jest#54)

@alexilyaev
Copy link
Contributor Author

Oh wow, what a bummer, totally forgot about it.
Would be awesome if Jest would let me know!

@SimenB
Copy link
Member

SimenB commented Jan 22, 2018

It should be pretty easy using the eslint plugin. Help welcome 🙂

I don't think it's possible to detect in Jest itself as jest is runtime only - no static analysis (beyond what we do in the babel plugin)

@alexilyaev
Copy link
Contributor Author

alexilyaev commented Jan 22, 2018

I see.
Might take a try at the eslint rule later.

Is there still something to do in this ticket then?

@SimenB
Copy link
Member

SimenB commented Jan 22, 2018

Will close once #5364 is merged

@tur-nr
Copy link

tur-nr commented Jan 23, 2018

It's also reports false positives. I think there is something wrong the resolves api.

it('should fail', () => {
    expect(Promise.resolve(false)).resolves.toBeTruthy();
});

This test case reports back as passed.

However if I write it another way;

it('should fail', async () => {
    const res = await Promise.resolve(false);
    expect(res).toBeTruthy();
});

This test case fails, as expected.


$ node --version
v8.9.2

$ jest --version
v22.1.4

EDIT

I do get the following in the console output

(node:9596) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: expect(received).toBeTruthy()

@incleaf
Copy link
Contributor

incleaf commented Jan 23, 2018

I think return expect(Promise.resolve(false)).resolves.toBeTruthy(); would be correct in the first case as doc says. But we can also give a synchronous error in resolves as rejects will do.

@SimenB
Copy link
Member

SimenB commented Jan 23, 2018

@tur-nr see #5361 (comment)

cpojer pushed a commit that referenced this issue Jan 24, 2018
* Make `makeRejectMatcher` synchronizable (#5361)

* Reorganize code using promise instaed of async/await

* Update changelog

* Move rejectedHandler onto then

* Make `makeResolveMatcher` synchronizable

* Update a snapshot

* Update CHANGELOG.md

* Update CHANGELOG.md
machour added a commit to gitpoint/git-point that referenced this issue Mar 23, 2018
expect(foo): foo must be a promise when using rejects/resolves: jestjs/jest#5361
@alirezabonab
Copy link

Here is what has worked for me.

await expect(asyncFn(arg)).rejects.toEqual(validationError); // validationError can be string | obj | ... 

@github-actions
Copy link

This issue 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 10, 2021
rauljurado620 added a commit to rauljurado620/git-point that referenced this issue Jun 15, 2023
expect(foo): foo must be a promise when using rejects/resolves: jestjs/jest#5361
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants