.throws with no message creates error with empty message

[mroderick]

When calling throwsException with a String as the error argument and no message argument, it will create errors with an empty message as seen in the code sample below.

function throwsException(fake, error, message) {
    if (typeof error === "function") {
        fake.exceptionCreator = error;
    } else if (typeof error === "string") {
        fake.exceptionCreator = function () {
            const newException = new Error(message || "");
            newException.name = error;
            return newException;
        };
    } else if (!error) {
        fake.exceptionCreator = function () {
            return new Error("Error");
        };
    } else {
        fake.exception = error;
    }
}

While this is not a bug as such, I do think it is poor design to return an Error instance with an empty message property.

Instead, we could calculate a reasonable error message from the error argument.

To Reproduce
Steps to reproduce:

const sinon = require("sinon");

const callback = sinon.stub();
callback.withArgs(1).throws("apple pie");

try {
    callback(1);
} catch (error) {
    console.log(`error.message: "${error.message}"`;
}

Expected behavior
Non-empty error.message

[ahmed1hsn]

function throwsException(fake, error, message) {
    if (typeof error === "function") {
        fake.exceptionCreator = error;
    } else if (typeof error === "string") {
        fake.exceptionCreator = function () {
            const newException = new Error(message || error || ""); // conditionally use error in case there is no message.
            newException.name = error;
            return newException;
        };
    } else if (!error) {
        fake.exceptionCreator = function () {
            return new Error("Error");
        };
    } else {
        fake.exception = error;
    }
}

@mroderick as error is already string in the block, and message is undefined (probably), we can conditionally use error in case there is no message inside this block.
Would it solve the issue?

[peanutenthusiast]

Hi, @mroderick, is this issue still open? I'd be happy to submit a PR with @ahmed1hsn's suggested changes if helpful.

[fatso83]

Please do. This is not really a breaking change AFAIK? More like a bug fix.

[peanutenthusiast]

Hi @fatso83,

When I applied @ahmed1hsn's suggestion and ran npm test, I got one failing test

1472 passing (2s)
  11 pending
  1 failing

  1) sinonSpy.call
       call.toString
         includes exception:

      AssertionError: [assert.equals] 'doIt() !TypeError(TypeError)' expected to be equal to 'doIt() !TypeError'
      + expected - actual

      -doIt() !TypeError(TypeError)
      +doIt() !TypeError
      
      at Object.fail (node_modules/@sinonjs/referee/lib/create-fail.js:5:21)
      at Object.fail (node_modules/@sinonjs/referee/lib/define-assertion/index.js:47:17)
      at assertion (node_modules/@sinonjs/referee/lib/define-assertion/index.js:65:11)
      at referee.<computed>.<computed> [as equals] (node_modules/@sinonjs/referee/lib/define-assertion/index.js:92:22)
      at Context.<anonymous> (test/proxy-call-test.js:1135:20)
      at process.processImmediate (node:internal/timers:476:21)

Should this test pass as originally expected? If not, let me know what the new expectation(s) should be.

[fatso83]

Not sure what a good message would be ... it looks a bit weird now. We can make it a bit more specific to tell the user Sinon made the error, just to give some value. Something like "Sinon-provided ${error}". What do you think? 🤔

[peanutenthusiast]

Very well. That sounds fine for now! Pushed the suggested change here:
2a83dfe

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

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