cucumber / cucumber-js

Cucumber for JavaScript
https://cucumber.io
MIT License
5.04k stars 1.09k forks source link

JSON Formatter: Logs stack trace exception and should log cucumber assertions errors #957

Closed humphreyn closed 6 years ago

humphreyn commented 6 years ago

Hi,

I think there is a bug in the 'json_formatter.js' at line 213, see snippet below:

    if (status === _status2.default.FAILED && exception) {
      data.result.error_message = exception.stack || exception;
    }

Any failed test will result in the stack trace being logged when IMO it should be the exception.message at a minimum and potentially the stack trace as well (it would be nice if you could turn on/off the stack trace errors).

Can someone confirm if this is a bug and how it will be resolved.

I am running NightwatchJS with CucumberJS, my environment details is as follows:

Node - v8.7.0 Cucumber - v3.03 Nightwatch - v0.9.16 Nightwatch cucumber - v8.2.2 Browser - Chrome v61.0.3163.100 Chrome driver - v2.32 Selenium - v3.5.3 Os - Windows 7

I have attached a copy of json formatter output showing the error message at line 86

Regards, Neville Humphrey

chrome.cucumber.json.zip

humphreyn commented 6 years ago

Has no one else seen this bug?

I maintain this is a bug. The line of code in the json_formatter @ 213 should be:

      data.result.error_message = exception.message + exception.stack;

and not

  data.result.error_message = exception.stack || exception;

That way you get the assertion error and the stack trace.

stevehipwell commented 6 years ago

I've seen the same issue, please can someone confirm that this is a bug?

charlierudolph commented 6 years ago

It is my experience in chrome and node that Error: <message> is the first line of the stack. Error: <message> is the result of error.toString(). We could use something like:

let msg = exception
if (exception.stack) {
  msg = exception.stack
  if (!_.includes(msg, error.toString()) {
    msg = error.toString() + '\n' + msg
  }
}
data.result.error_message = msg

Can confirm the stack does not include the message in safari. I see the message in the stack in chrome 61 and node 8.

innocentiv commented 6 years ago

I encounter the same problem using nightwatch cucumber (failure will often result in an error message without a stack). It's a bug to me

let error_message = _.includes(exception.stack, exception.message) ? '' : exception.message + '\n';
error_message += exception.stack;
data.result.error_message = error_message || exception;

If the exception message is included in the stack, this solution might provide an error_message without repetition. Otherwise the exception message is put before.

finaruioana commented 6 years ago

Hello,

We have a similar issue at the same line, but due to the type of this element. Into the json report file generated, error_message is a string OR an object. Could we have a consistency here? Waiting to hear people's thoughts on it.

The issue for us is when we try to generate the html report from json. At the minute, the cucumber-html-reporter library expects this error_message to be a string. When we get it as an object, the html reporter fails.

Going further, we get this type difference due to tests using different browser libraries. Whenever we run the tests using webdriverio, the report generation passes. But using webdriverjs-angular, the report.json will have the error_message as an object and thus the failure when generating the html report.

Based on your response, I might need to raise the issue in a different place, but I thought the best place to start with this is to see if we could have a consistency over the error_message type.

Thanks!

humphreyn commented 6 years ago

Hi finaruioana,

In your local version of cucumber/lib/formatter/json_formatter.js could you test if changing line 213 to the following works for you:

      let error_message = _lodash.includes(exception.stack, exception.message) ? '' : exception.message + '\n';
      error_message += exception.stack;
      data.result.error_message = error_message || exception;

If it does I may put in pull request with the above fix, as there does not seem to be any traction on this issue.

finaruioana commented 6 years ago

Hi @humphreyn

Indeed, making the above changes solved my issue. I'd also add an if on the stack before appending it, in my case was appending undefined.


    let error_message = _lodash.includes(exception.stack, exception.message) ? '' : exception.message + '\n';
    if (exception.stack) {
        error_message += exception.stack;
    }
    data.result.error_message = error_message || exception; ```

Thanks! 
innocentiv commented 6 years ago

I took for granted that exception is an object with message and stack defined, but if that is not always the case small adjustment are needed:

let msg = exception.message || '';
let stack = exception.stack || '';
msg = _.includes(stack, msg) ? '' : (msg + '\n');
data.result.error_message = (msg + stack) || exception;

This code prevent "undefined" to be added to the error_message if message or stack are not defined. If both are undefined or empty string the exception object is assigned to error_message. If we want to enforce a string error_message we can use JSON.stringify:

let msg = exception.message || '';
let stack = exception.stack || '';
msg = _.includes(stack, msg) ? '' : (msg + '\n');
data.result.error_message = (msg + stack) || JSON.stringify(exception);

@finaruioana you think that this solution would be ok (to test in local environment you have to use _lodash instead of _)

let msg = exception.message || '';
let stack = exception.stack || '';
msg = _lodash.includes(stack, msg) ? '' : (msg + '\n');
data.result.error_message = (msg + stack) || exception;
finaruioana commented 6 years ago

yeah, looks good @innocentiv. Tested it and seems to work fine.

humphreyn commented 6 years ago

@innocentiv , I can also confirm that those changes work locally for me in chrome, ie and firefox with the following set up: nodejs: v8.9.0 cucumberjs: v 3.1.0 nightwatch-cucumber: v0.9.16 selenium-standalone: v3.4.0 ie webdriver: v3.4.0 chrome webdriver: v2.33 gecko (firefox) webdriver: v0.19.1

Will you submit a pull request to apply this fix?

innocentiv commented 6 years ago

@humphreyn Pull request: #973

I just modified the pull request to check for string exception. You can test it locally using:

          if (_lodash2.default.isString(exception)) {
            data.result.error_message = exception;
          } else {
            var _exception$message = exception.message,
                message = _exception$message === undefined ? '' : _exception$message,
                _exception$stack = exception.stack,
                stack = _exception$stack === undefined ? '' : _exception$stack;

            message = _lodash2.default.includes(stack, message) ? '' : message + '\n';
            data.result.error_message = message + stack || JSON.stringify(exception);
          }
lock[bot] commented 5 years ago

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.