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

[breaking] Don't convert non-Error promise rejection reason #582

Closed
wants to merge 1 commit into from
Closed

[breaking] Don't convert non-Error promise rejection reason #582

wants to merge 1 commit into from

Conversation

kasperlewau
Copy link
Contributor

Promises that are rejected with a non-Error are currently passing expectations as we're converting to an Error in lib/assert.

This PR changes the behaviour of t.throws so that it no longer considers the following example a pass:

test(async t => {
 await t.throws(Promise.reject('foo'), 'foo');
});

If the rejection reason is a non-Error, core-assert catches this which results in an AssertionError.

Whereas the following would pass with flying colors:

test(async t => {
  await t.throws(Promise.reject(new Error('foo'), 'foo');
});

Docs for t.throws saw a minor addition;

Assert that function throws an error**,** or promise rejects with an error.

Fixes #468

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @therealklanni, @sotojuan and @SamVerschueren to be potential reviewers

@vadimdemedes
Copy link
Contributor

I remember we had some back and forth with this decision, but atm I'd also like to merge this and strictly expect Error objects.

@kasperlewau
Copy link
Contributor Author

I suppose yielding a custom Error to the consumer with a recommendation to reject Promises with actual Errors would be somewhat of a middle ground, if we're in 'back and forth land' that is?

A missing expectation AssertionError doesn't explicitly highlight that fact (imho), but I didn't take a good look at what the error stack yielded as it stands. A custom Error could just be extraneous information.

@novemberborn
Copy link
Member

I remember we had some back and forth with this decision, but atm I'd also like to merge this and strictly expect Error objects.

We all seem to be in agreement in #468 so let's not restart this argument 😉

@@ -81,8 +81,6 @@ x.throws = function (fn, err, msg) {
x.throws(function () {
if (fnErr instanceof Error) {
throw fnErr;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Remove the if-guard as well, simply throw fnError. This makes it equivalent to t.throws(() => { throw 'foo' }).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very well! I'll go ahead and amend my commit if you don't mind, and save myself the trouble of having to squash later.

Copy link
Member

Choose a reason for hiding this comment

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

Sure. It's going to conflict with #576 though 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I'll get on this once #576 is merged.

@novemberborn
Copy link
Member

@sindresorhus this will be a breaking change.

@sindresorhus sindresorhus force-pushed the master branch 2 times, most recently from c6214aa to 2f10448 Compare March 2, 2016 07:42
@sindresorhus sindresorhus changed the title Don't convert non-Error promise rejection reason [breaking] Don't convert non-Error promise rejection reason Mar 2, 2016
@sindresorhus
Copy link
Member

this will be a breaking change.

True, although I really doubt (hope) nobody depends on this behavior.

@sindresorhus
Copy link
Member

Thanks @kasperlewau :)

@kasperlewau
Copy link
Contributor Author

Thank you(s)!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants