How can we override reject creating a new Error Object?

[JemiloII]

https://github.com/sinonjs/sinon/blob/4310343393b16b92b56526ab4614da4eb2886efa/lib/sinon/default-behaviors.js#L176

I'm maintaining some code that rejects strings and this is creating issues while testing. my bluebird catches are not behaving correctly.

bluebird.rejects('Something')
    .catch(e => e === 'Something' /* true */, () => console.log(typeof e) /*string*/)
    .catch(() => console.log('should not be here'));

However using sinon.stub().usingPromise('bluebird').rejects('Something'); ends up creating an error object with the name Something. Not expected at all.

Before someone says we are only supporting native Promises, let's investigate what they do!

Promise.reject('Something')
     .catch(e => {
        console.log(e === 'Something'); // true
        console.log(typeof e); // string
    });

Hmmm, they don't create an Error class, they just toss the exception it was given like a throw.

try { 
    throw 'Something'; 
} catch (e) { 
    console.log(e === 'Something');  // true
    console.log(typeof e); // string
}

[mroderick]

Before someone says we are only supporting native Promises, let's investigate what they do!

That's a strawman fallacy. You know well enough that we support promise libraries, as you use them in your example.

The Sinon maintainers are fully aware of how JavaScript promises work, including that Promise.reject() will pass anything you pass to it.

I'm maintaining some code that rejects strings

In my opinion, rejecting promises with String instead of Error is really doing a disservice to Future Developer, who has to debug what we write today. Strings are not very useful for debugging, as they don't capture stack traces. Error on the other hand does. Therefore, I think that rejects() behaviour is desirable, as that makes it easy to use the debugger to quickly find out what went wrong.

It seems you disagree, but the only statement that you've made is that Sinon doesn't meet your expectations. If you think the default behaviour should be changed, then please post arguments for why, keeping in mind that the change has potential to affect many users.

If there are solid arguments for changing the default behaviour, I can't imagine any of the maintainers of Sinon would oppose a pull request to change it.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dtom90]

I'd say there could be differing opinions as to whether or not a stack trace is actually useful in certain scenarios. In my particular situation, I actually don't care about the stack trace, I only care about the error message and then passing it somewhere else. So I've written code to handle both String and Error type reject reasons. However, since sinon does the String -> Error coercion for me, I cannot test the branch of my code that handles strings. As such I cannot get full coverage

[mantoni]

Stack trace usefulness and discussions about whether one should create error objects aside, I agree that generally

const example = sinon.stub().rejects(something);

should behave the same as

const example = () => Promise.reject(something);

Currently, this is not the case if something is a string. I'd say the current behavior is a bug.

[JemiloII]

I agree.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[oleksii-lysenko-fc]

@mroderick It should be up to developers to use strings for rejected promises and thrown exceptions. Developers are responsible for an interface of applications they create and for internal data flow inside their apps.

As a solution, when stub should return a rejected (native) promise one can use returns

sinon.stub().returns(Promise.reject('Something'))

If someone decides to use rejects to get a rejected promise with a non-zero number, it will work without converting value to the Error object, sinon.stub().rejects(42). @mantoni @JemiloII

[dgreenwald-ccs]

This was really confusing to me because I assumed that stub.rejects('string) works the same as throw('string) from an async function. Workaround is to use .returns(Promise.reject('string'). After reading this thread I still don't understand why the behavior does not match throw.

[fatso83]

I am currently trying to weed out the small issues that's been lying dormant for ages and so I am having a look at this.

@dgreenwald-ccs Whether or not an API works similar to some syntax is a bit of weird comparison. The current behavior of #rejects() is totally consistent with how sinon.throws() behaves (docs), which is a better comparison. It would be worse if stub.throw worked one way and stub.rejects a totally different way. Right now, they both have an API that is consistent in accepting one or two parameters and they work alike, just sync vs async.

It's also not a bug per se, as this is the documented behavior, @mantoni. Promise.reject() only takes a single (optional) parameter, reason, whereas this API takes 0 to 2 parameters.

IMHO this is really a missing feature, as the current API does not allow a normal (but unfortunate) practice. The same applies to the newer Fakes API, which also has a #rejects() method. Unfortunately, it is not possible to allow throwing strings using the existing code, without breaking existing user code.

I do think we can validate breaking the API here for some reasons

• we should support testing normal practices (no matter if they smell)
• we should follow the principle of least surprise (which is what brought some people to this issue)

That being said, it is hard/impossible to design the API to allow all the things throw and Promise.reject() allows while still keeping a user friendly and concise. For instance, all of the below are perfectly valid things to write:

throw undefined // err === undefined
Promise.reject() // reason === undefined

We do not allow that in the simplest way of using the API, as we use a check for undefined to determine whether you passed an argument and throw an Error, which is what people mostly would assume throws() and rejects() should do. We still make it possible though: you can pass a function to stub.rejects and have it return whatever you want. EDIT: after thinking a bit, we can support an explicit throws(undefined) and rejects(undefined) by checking argument length first 🤔

So my suggestion is to change the one case where we have a single argument of type string given to stub.rejects and make that just return a string as the reason. The same applies to fake.rejects.

[fatso83]

I want to lump the suggestion (when implemented) as a breaking change along with others for the v18 release, unless anyone objects.

@mroderick what do you think? It's a quite minimal change, but breaking. AFAIK it should only need to update the stubs and fakes code. Anything else?

[fatso83]

Not much discussion here, but I made a PR (https://github.com/sinonjs/sinon/pull/2569) to implement the changes proposed. Think of it as a proposal, not necessarily the fix. In the PR I discuss how it might be just as valid to not change anything, rather just improving docs of how things work and make examples of how to use the existing methods to cover the special cases.

[fatso83]

I closed the PR. What we need is more/better examples how to use the API.

[mroderick]

@hexeberlin and I were working on solving (the now closed) #2580.

We agree with the following points put forwards in the discussion above:

• it would be nice if .rejects and throws would follow the native JS apis and practices (even when silly)
• it is ok to introduce breaking changes, it shouldn't be that difficult for users to fix their tests

So, we propose to solve this by creating a new major version and making .rejects and .throws behave as close to native as possible and improving the documentation with examples (including what not to do).

[fatso83]

I don't oppose changing the API, but do keep in mind my comment above, @mroderick

That being said, it is hard/impossible to design the API to allow all the things throw and Promise.reject() allows while still keeping a user friendly and concise. For instance, all of the below are perfectly valid things to write:

(see the code)

I found that it wasn't possible to be 1:1 either way, so that's part of the reason why I closed my PR with the changes. Replacing one version where you need to consult the docs with another where you have to consult the docs as well was not good enough reason for changing, I thought.