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

Call the inspect() function if message is not set #1848

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@kevinburke
Contributor

kevinburke commented Aug 27, 2015

Unfortunately some frameworks throw error objects which do not have a message
property but do have an inspect() function. This means Mocha reports a test
failure, but prints the empty string instead of any useful information about
the error message. For an example, see versions of the Waterline ORM before
v0.10.19: balderdashy/waterline@0965d13

If the message key is not set on an error object, attempt to print the same
output as console.log in Node, by calling the object's inspect() function,
if it exists.

We could try to fallback to calling util.inspect on the err object and
logging that, but it would break outside of Node and I'm not sure the format would be appropriate.

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

This comment has been minimized.

@kevinburke

kevinburke Aug 27, 2015

Contributor

Another option here would be to call util.inspect(err), but this wouldn't work outside of Node.

This comment has been minimized.

@danielstjules

danielstjules Sep 8, 2015

Contributor

The return value of an object's inspect function doesn't have to be a string. Would be worth calling toString to ensure that message is always a string.

From the docs: https://nodejs.org/api/util.html#util_util_inspect_object_options

You may also return another Object entirely, and the returned String will be formatted according to the returned Object. This is similar to how JSON.stringify() works:

var obj = { foo: 'this will not show up in the inspect() output' };
obj.inspect = function(depth) {
  return { bar: 'baz' };
};

util.inspect(obj);
  // "{ bar: 'baz' }"
@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Aug 27, 2015

If you're curious, this is what the output from Mocha currently looks like when we get test failures:

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Aug 27, 2015

How it looks on my branch:

Call the inspect() function if message is not set
Unfortunately some frameworks throw error objects which do not have a `message`
property but do have an `inspect()` function. This means Mocha reports a test
failure, but prints the empty string instead of any useful information about
the error message. For an example, see versions of the Waterline ORM before
v0.10.19: balderdashy/waterline@0965d13

If the `message` key is not set on an error object, attempt to print the same
output as `console.log` in Node, by calling the object's `inspect()` function,
if it exists.

We could try to fallback to calling `util.inspect` on the `err` object and
logging that, but I'm not sure the format would be appropriate.
@JamesCropcho

This comment has been minimized.

JamesCropcho commented Aug 31, 2015

I too have been experiencing this and would much prefer the proper solution here to the workaround I've been using.

Are you folks solving this in a different way in your own setups?

What do you think of this enhancement?

@danielstjules

This comment has been minimized.

Contributor

danielstjules commented Sep 8, 2015

Thanks for the PR! Aside from what I mentioned regarding non-string values returned by err.inspect(), it looks good :)

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Sep 11, 2015

Thanks for the code review @danielstjules! I started going down a path where I called the object's inspect() method and then checking whether the result was a string. But in that case if the result is an object and you called .toString() on it, you would get "[object Object]" as the test output.

I am starting to think that I should have just called util.inspect(), which always returns a string and usually doesn't return "[object Object]". However I believe Mocha needs to run in non-Node environments, which makes that tricky? A polyfill seems like it would be way too cumbersome, the source for util.inspect runs over 200 lines, https://github.com/nodejs/node/blob/master/lib/util.js#L195. Thoughts?

@danielstjules

This comment has been minimized.

Contributor

danielstjules commented Sep 11, 2015

I am starting to think that I should have just called util.inspect(), which always returns a string and usually doesn't return "[object Object]".

I think I'm less worried about that, and mostly just want to make sure we won't hit a path where mocha throws. For example, var match = message.match(/^([^:]+): expected/); If error.inspect() returned something that isn't a string, then we're gonna throw a TypeError.

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Sep 11, 2015

Ok! Let me try to write something up and then see if it makes sense.

Kevin Burke
phone: 925.271.7005 | twentymilliseconds.com

On Fri, Sep 11, 2015 at 7:52 AM, Daniel St. Jules notifications@github.com
wrote:

I am starting to think that I should have just called util.inspect(),
which always returns a string and usually doesn't return "[object Object]".

I think I'm less worried about that, and mostly just want to make sure we
won't hit a path where mocha throws. For example, var match =
message.match(/^([^:]+): expected/); If error.inspect() returned
something that isn't a string, then we're gonna throw a TypeError.


Reply to this email directly or view it on GitHub
#1848 (comment).

@danielstjules

This comment has been minimized.

Contributor

danielstjules commented Sep 11, 2015

Oh, and I think we do get utils.inspect as part of the browser bundle.

mocha/mocha.js

Lines 10786 to 10818 in c7a8fed

/**
* Echos the value of a value. Trys to print the value out
* in the best way possible given the different types.
*
* @param {Object} obj The object to print out.
* @param {Object} opts Optional options object that alters the output.
*/
/* legacy: obj, showHidden, depth, colors*/
function inspect(obj, opts) {
// default options
var ctx = {
seen: [],
stylize: stylizeNoColor
};
// legacy...
if (arguments.length >= 3) ctx.depth = arguments[2];
if (arguments.length >= 4) ctx.colors = arguments[3];
if (isBoolean(opts)) {
// legacy...
ctx.showHidden = opts;
} else if (opts) {
// got an "options" object
exports._extend(ctx, opts);
}
// set default options
if (isUndefined(ctx.showHidden)) ctx.showHidden = false;
if (isUndefined(ctx.depth)) ctx.depth = 2;
if (isUndefined(ctx.colors)) ctx.colors = false;
if (isUndefined(ctx.customInspect)) ctx.customInspect = true;
if (ctx.colors) ctx.stylize = stylizeWithColor;
return formatValue(ctx, obj, ctx.depth);
}
exports.inspect = inspect;
So you could just call utils.inspect(err) instead of invoking err.inspect(), since it'll cast to string. :)

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Sep 11, 2015

Hmm I just tried utils.inspect and I'm getting undefined is not a function.. are you in IRC or somewhere where I can talk this thru for 2 mins?

@danielstjules

This comment has been minimized.

Contributor

danielstjules commented Sep 11, 2015

Hmm I just tried utils.inspect and I'm getting undefined is not a function..

I lied, sorry. Looks like utils is just getting picked up to support the browserify buffer module. So that would make sense :( Sorry

@kevinburkeshyp

This comment has been minimized.

kevinburkeshyp commented Sep 11, 2015

Had to pick up some paid work - sorry! Will push something soon & tag you again when I do.

@danielstjules

This comment has been minimized.

Contributor

danielstjules commented Sep 12, 2015

Merged in 31fb4be Doing a quick cast in 0956c02 I'd rather avoid using utils.inspect for this, since it'd be nice to reduce the browserify bundle size in the near future :)

Thanks again for the PR!

@kevinburke

This comment has been minimized.

Contributor

kevinburke commented Sep 14, 2015

Thanks @danielstjules! Sorry I couldn't push this over the finish line in time.

@danielstjules

This comment has been minimized.

Contributor

danielstjules commented Sep 14, 2015

Nah, don't worry about it - there was nothing more to do! Appreciate the help!

@danielstjules danielstjules referenced this pull request Sep 16, 2015

Closed

no method 'match' #1894

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment