Fix reporter bug when rejecting non-errors

[jakwuh]

To reproduce try ava with the following test:

import test from 'ava';

test(t => {
    Promise.reject(1);
});

[novemberborn]

Interesting find. I think the error comes from this line, really:

ava/lib/run-status.js

Line 24 in 98dded5

stack: err

Unhandled rejections are normalized:

ava/lib/run-status.js

Line 72 in 98dded5

err = normalizeError(err);

Perhaps the message should be set to String(err) and stack shouldn't be set at all. Further, error objects that have a non-string message, name or stack should have those properties deleted. That would be in

ava/lib/serialize-error.js

Line 59 in 98dded5

if (err.stack) {

.

Long story short, this means errors have the expected shape in the reporters and we don't have to keep checking if certain properties are strings etc. @avajs/core thoughts?

[sindresorhus]

Long story short, this means errors have the expected shape in the reporters and we don't have to keep checking if certain properties are strings etc. @avajs/core thoughts?

👍

[jakwuh]

@novemberborn thank you for the response. I think having message: String(err) + removing stack property is the best solution here. Isn't it?

[novemberborn]

@jakwuh yea. Though that's just for rejection reasons that aren't objects. We also need to sanitize objects with unexpected properties.

[jakwuh]

@novemberborn here we go, now regardfully listening to any feedback :)

[novemberborn]

I think this really does need to go here:

ava/lib/serialize-error.js

Line 59 in 98dded5

if (err.stack) {

We don't want to modify err since it comes from user code. There may be side-effects. But we can't easily copy error objects since name and message aren't always enumerable. After the call to cleanYamlObject in serialize-error.js we have a copy we can safely modify.

Also, in this case, I'd prefer a little bit of duplication over your forEach trick. IMHO this is a lot easier to follow:

if ('name' in err && typeof err.name !== 'string') {
  delete err.name
}
if ('stack' in err && typeof err.stack !== 'string') {
  delete err.stack
}

Even better, delete only deletes own-properties, so the 'name' in err guard is unnecessary.

[jakwuh]

Yes, this totally makes sense, still I find fairly to say that errors coming to normalizeError are already serialized (as I can see from the test-worker code).
Also thank you a lot for your fast and detailed feedback.

[jakwuh]

@novemberborn here we go 🙂

[novemberborn]

Great!

[novemberborn]

@sindresorhus any thoughts on this? XO doesn't complain 😉

[sindresorhus]

Why should it have complained?

[novemberborn]

Figured it may have opinions on if condition indenting.

[sindresorhus]

Woot! Very good work @jakwuh :)

[jakwuh]

Thanks 😊