Can't control stringification of error messages

[jasonk]

Prerequisites

• Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend that you not install Mocha globally.

Description

I've been running into issues with error stringification, due to the change that was introduced in #2101.

My application adds additional context to error messages in development by adding additional properties to the Error objects, and I've struggled to get Mocha to display any of this additional information.

The main problem is that the code used to ensure that message is a string is this (from lib/reporters/list.js):

    if (err.message && typeof err.message.toString === 'function') {
      message = err.message + '';
    } else if (typeof err.inspect === 'function') {
      message = err.inspect() + '';
    } else {
      message = '';
    }

The problem with that is that you can (theoretically) customize how Mocha displays your error messages, but only if the error doesn't have a message property that includes a toString method, which seems like an odd edge case for error messages.

So in my case, I have the following chain of events making this impossible.

• All of my errors have a message property, and it is always a String
• String.prototype has a toString method that just returns itself
• You can't replace the toString method on a string.

This also means that if you are using a custom Error subclass that includes an inspect method, then whether that function gets called or not can be affected by what the message property contains. It will work if that property doesn't have a toString method, but won't work if it does.

The only way I've found to make this work is with new Error( undefined ), which seems to be the only way to get a value into the message property that doesn't have a toString method, unfortunately that fixes it for Mocha but makes it useless for everything else.

My recommendation would be to just swap the order of the checks, if the error has an inspect method it seems like that should be used regardless of what the message is.

Steps to Reproduce

describe( 'should show my extra props', () => {
  it( 'with a string error', () => error( 'something' ) );
  it( 'without a string error', () => error( undefined ) );
} );

function error( msg ) {
  const err = new Error( msg );
  err.inspect = function inspect() {
    return `Custom Formatted Error: ${msg}`;
  };
  throw err;
}

Expected behavior: [What you expect to happen]

Both of the test should fail with an error message that gets displayed with a prefix of Custom Formatted Error.

Actual behavior: [What actually happens]

Only one of them gets formatted correctly.

  should show my extra props
    1) with a string error
    2) without a string error


  0 passing (8ms)
  2 failing

  1) should show my extra props
       with a string error:
     Error: something
      at error (mocha-repro.js:7:15)
      at Context.it (mocha-repro.js:2:36)
      at processImmediate (internal/timers.js:443:21)

  2) should show my extra props
       without a string error:
     Custom Formatted Error: undefined
  Error
      at error (mocha-repro.js:7:15)
      at Context.it (mocha-repro.js:3:39)
      at processImmediate (internal/timers.js:443:21)

Reproduces how often: [What percentage of the time does it reproduce?]

100% reproducible

Versions

• The output of mocha --version and node node_modules/.bin/mocha --version: 6.2.0

[boneskull]

I think this is probably a bug, and would encourage a PR to switch the order of the conditionals.

It'd be helpful to understand what assertion library has these errors with inspect() methods (I don't know offhand). This isn't any sort of convention that I'm aware of, but I've been wrong before!

[Munter]

the inspect() method on an error is news to me as well. I think most libraries look at the message, name or stack properties, which are standard

[jasonk]

It was not assertions where I was seeing this, the problem I was having was with tests that were failing because they were throwing actual exceptions from buggy code, and this was preventing me from getting the information that I needed to determine why those exceptions had been thrown.

[vkarpov15]

What about util.inspect.custom? That's the preferred way to declare custom inspect functions in Node. Perhaps we should change it so that the priority is:

1. util.inspect.custom
2. inspect()
3. message

[rulatir]

Another year, another crippling bug in essential tooling that seems to receive zero attention from developers...