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

Make rejects and resolves synchronously validate its argument #5364

Merged
merged 9 commits into from
Jan 24, 2018

Conversation

incleaf
Copy link
Contributor

@incleaf incleaf commented Jan 22, 2018

Summary
Fix #5361

Test plan
Pass non-promise values into reject matchers synchronously

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@incleaf
Copy link
Contributor Author

incleaf commented Jan 22, 2018

@SimenB Please let me know if I missed something 😇

@codecov-io
Copy link

codecov-io commented Jan 22, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5364   +/-   ##
=======================================
  Coverage   61.32%   61.32%           
=======================================
  Files         205      205           
  Lines        6925     6925           
  Branches        3        4    +1     
=======================================
  Hits         4247     4247           
  Misses       2677     2677           
  Partials        1        1

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 4080d98...2ef8358. Read the comment docs.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

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.

Please update the changelog as well

} catch (e) {
return makeThrowingMatcher(matcher, isNot, e).apply(null, args);
}
return (async () => {
Copy link
Member

Choose a reason for hiding this comment

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

you can do

return result
.catch(e => makeThrowingMatcher(matcher, isNot, e).apply(null, args))
.then(() => Promise.reject(
  // new JestAssertionError(...
));

And drop the async

);
return Promise.reject(err);
})
.catch(e => makeThrowingMatcher(matcher, isNot, e).apply(null, args));
Copy link
Member

Choose a reason for hiding this comment

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

This catch has to be first

Copy link
Member

Choose a reason for hiding this comment

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

Wait no, that won't work either. You have to use .then(resolvedHandler, rejectedHandler) to avoid handling the rejection here.

So just move your catch into the second argument of the then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://repl.it/repls/RevolvingEvergreenBlackbuck
Is there any difference between A and B, besides syntax preference? Both a and b pass the test cases.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, yes. since we expect it to fail we need to have our catch first so we can resolve the promise with it. But if it does not reject we need to reject it ourselves.

If we add a then after the catch, it's not called. If we add our catch onto a then, we locally catch the rejection we want to bubble up

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.

Can you add the same for makeResolveMatcher?

@incleaf
Copy link
Contributor Author

incleaf commented Jan 23, 2018

OK, I'll add the same for makeResolveMatcher 🙂

@incleaf incleaf changed the title Make makeRejectMatcher synchronizable (#5361) Make rejects and resolves synchronizable (#5361) Jan 24, 2018
@SimenB
Copy link
Member

SimenB commented Jan 24, 2018

@incleaf can you run ./jest integration-tests/__tests__/failures.test.js -u locally and commit it to fix the failing test?

Also, please fix the conflict.

@incleaf
Copy link
Contributor Author

incleaf commented Jan 24, 2018

@SimenB Just updated the snapshot and fixed the conflict.

@SimenB SimenB changed the title Make rejects and resolves synchronizable (#5361) Make rejects and resolves synchronous (#5361) Jan 24, 2018
@SimenB SimenB changed the title Make rejects and resolves synchronous (#5361) Make rejects and resolves synchronous Jan 24, 2018
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.

Thanks!

@SimenB
Copy link
Member

SimenB commented Jan 24, 2018

Yay, ci. Some weird permission stuff, but your last commit is green and my change was just to the changelog

@cpojer cpojer merged commit 4585c73 into jestjs:master Jan 24, 2018
@incleaf
Copy link
Contributor Author

incleaf commented Jan 25, 2018

@SimenB Thank you for the review. It helped me a lot 😀

@incleaf incleaf changed the title Make rejects and resolves synchronous Make rejects and resolves synchronously validate its argument Jan 25, 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/resolves does not fail the test on non-promise value
5 participants