Exception message in json formatter

[innocentiv]

Add exception.message to error_message if not already included in exception.stack

Fix #957

[finaruioana]

for consistency over the type of error_message, I would suggest to have a JSON.stringify(exception). It would be a rare exception, but at least we know error_message will be a string all the time.

[innocentiv]

I omitted the JSON.stringify in the pull request because that would change the spec (JSON.stringify add escaped quotes to string object):

cucumber-js/src/formatter/json_formatter_spec.js

Line 128 in 267725e

result: { duration: 1, exception: 'my error', status: Status.FAILED }

cucumber-js/src/formatter/json_formatter_spec.js

Line 141 in 267725e

error_message: 'my error',

Adding a type check would enforce a string without changing the spec.
I wouldn't add more code if not really needed

data.result.error_message = message + stack || _.isString(exception) ? exception :  JSON.stringify(exception)

[innocentiv]

@finaruioana

This may be a better solution, what do you think?

if(_.isString(exception)) {
  data.result.error_message = exception
} else {
  let {message = '', stack = ''} = exception
  message = _.includes(stack, message) ? '' : message + '\n'
  data.result.error_message = message + stack || JSON.stringify(exception)
}

[innocentiv]

I have updated the pull request to force consistency over the type of error_message

[humphreyn]

Great work guys,
Sorry I couldn't be of more assistance. It's been mad at work and home:-)

[charlierudolph]

This is hard for me to follow. Thoughts on breaking out a function stringifyException and focusing on making it easy to understand:

function stringifyException(exception) {
  // if is string 
  //   return it
  // else if has message or stack
  //   if stack includes message
  //     return stack
  //   else 
  //     return message newline stack
  // else 
  //   return stringified exception
}

The function could then go in a util file and be unit tested with some sample exceptions from the different browsers and non-error objects

[innocentiv]

You are right, move this code to a function would be a better solution.
Any suggestion on where to put this utils file?
What about using /src/formatter/helpers/error_helpers.js?

[charlierudolph]

Sounds good to me

[innocentiv]

I have updated the pull request to add function stringifyException and unit test

[charlierudolph]

Thanks for the great test cases but thinking more on this feature I think we should use node-assertion-error-formatter which is what we use when displaying errors in the terminal. It includes the logic for concatenating the message / stack and shows diffs as well. Sorry I didn't remember this sooner.

[innocentiv]

node-assertion-error-formatter does not pass 3 of the tests case I wrote. If those error are acceptable I can modify for the fourth time the pull request :)

1. ErrorHelpers
   stringifyException
   return stack:
   AssertionError: expected '[object Object]stack' to equal 'stack'

2. ErrorHelpers
   stringifyException
   return message and stack:
   AssertionError: expected 'errorstack' to equal 'error\nstack'

3. ErrorHelpers
   stringifyException
   stringify an object:
   TypeError: stack.indexOf is not a function

[charlierudolph]

I just released v2.0.1 of node-assertion-error-formatter which should fix cases 2 and 3 (charlierudolph/node-assertion-error-formatter@ca21cfd). Please update the version in the package.json / yarn.lock.

I think test case number 1 needs to be updated so the message is an empty string instead of undefined. Testing out the error constructor in node 8, this was always the case.

If all your test cases pass, I think the tests / util file you added can be deleted as the test cases are now covered in node-assertion-error-formatter. We can use node-assertion-error-formatter directly in src/formatter/json_formatter.js

[innocentiv]

I have updated the pull request. Now there is a small error when message is an empty string.

1. ErrorHelpers
   format
   return stack:
   AssertionError: expected '{"message":"","stack":"stack"}\nstack' to equal 'stack'

This is not a major error and should be fixed in assertion-error-formatter. I think we can merge this pull request and update later this dependency.

[aslakhellesoy]

Hi @innocentiv,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

• ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
• 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
• 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
• ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
• 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.