1.2.1 regression: dont crash if there is a single stackframe #605

hmalphettes · 2016-07-06T04:45:38Z

Regression from #538:

(...)/node_modules/cucumber/lib/cucumber/support_code/library.js:53
        var line = stackframes[1].getLineNumber();
                                 ^

Cannot read property 'getLineNumber' of undefined
    at Object.module.exports (/apps/ui.stoic/test/cucumber/features/step_definitions/runtime.js:81:8)

In my setup there is either 1 stackframe or 8 of them.
I am not sure this is the perfect fix but it does re-enable running the testsuite.

hmalphettes · 2016-07-06T05:10:44Z

For info, comparing callsite and stacktrace-js in the defineHook I am getting:

defineHook: function defineHook(builder, collection) {
...
console.log('we got a site');
stackframes.forEach((s, i) => {
   console.log('  ' + i + ' ' + s.getFileName() + s.getLineNumber());
});
...

# callsite (1.2.0)
we got a site
  0 /myproj/node_modules/cucumber/lib/cucumber/support_code/library.js52
  1 /myproj/test/cucumber/features/step_definitions/runtime.js81
  2 /myproj/node_modules/cucumber/lib/cucumber/cli/support_code_loader.js65
  3 native array.js960
  4 /myproj/node_modules/cucumber/lib/cucumber/cli/support_code_loader.js62
  5 /myproj/node_modules/cucumber/lib/cucumber/cli/support_code_loader.js24
  6 /myproj/node_modules/cucumber/lib/cucumber/support_code/library.js124
  7 /myproj/node_modules/cucumber/lib/cucumber/cli/support_code_loader.js10
  8 /myproj/node_modules/cucumber/lib/cucumber/cli/configuration.js126
  9 /myproj/node_modules/cucumber/lib/cucumber/runtime.js43

# stacktrace-js
we got a site
  0 /myproj/test/cucumber/features/step_definitions/runtime.js81

charlierudolph · 2016-07-06T16:13:41Z

I'm very curious about your setup. Can you share a minimal setup to reproduce this or add a feature test that captures your setup (fails without your changes and passes with them)?

hmalphettes · 2016-07-07T02:35:31Z

@charlierudolph tough one! But I hear you. Calling my colleagues (@floribon) to the rescue and reviewing the existing cucumber-js tests to try to spot the difference.

floribon · 2016-08-10T10:26:13Z

@charlierudolph this happens when using custom stack traces as suggested in this thread #157 (comment)

I am overriding the Error.prepareStackTrace in order to only have traces from our own files, which speeds up debugging a lot:

/** https://github.com/cucumber/cucumber-js/issues/157#issuecomment-43309248 **/
function setCleanStrackTraces() {

  // monkey-patch stack trace more-or-less compatible with webdriver
  const isMyStack = new RegExp(
    path.join(
      __dirname, '../../../..',
      '[^node_modules/]' // exclude node_modules
    ).replace(/\//g, '\\/')
  );

  Error.prepareStackTrace = function (err, stack) {

    let message = err
      .message.split('\n')
      .filter(s => !/Session|Driver|Build|System|Capabilities|Command/.test(s))
      .join('\n') || '';

    if (!message.trim()) {
      // we would rather see something than not see something at all
      message = '[internal] ' + err.message.split('\n')[0].trim();
    }

    if (message) { message += '\n'; }

    const results = [];
    for (const elt of stack) {
      const frame = elt.toString();
      if (isMyStack.test(frame)) {
        results.push('    at ' + frame);
      }
    }

    return message + results.join('\n');
  };
}

Unfortunately this is breaking since 1.2.1

charlierudolph · 2016-08-29T05:10:28Z

lib/cucumber/support_code/library.js

+          stackframes[i].fileName !== '(native)') {
+        return stackframes[i];
+      }
+    }


Please use lodash's _.find here instead of the manual for loop and _.includes instead of stackframes[i].fileName.indexOf.

Actually can this just be simplified to

var stackframes = StackTrace.getSync(); if (stackframes.length > 1) { return stackframes[1]; } else { return stackframes[0]; }

floribon · 2016-08-31T15:10:04Z

@charlierudolph PR updated with your suggested syntax, it does work for us

charlierudolph · 2016-08-31T16:19:20Z

lib/cucumber/support_code/library.js

+  function getStackframe() {
+    var stackframes = StackTrace.getSync();
+    if (stackframes.length > 1) {
+      return stackframes[1];


Based on the failing feature tests, I think this logic needs to actually be

if (stackframes.length > 2) { return stackframes[2]; } // ...

Since the defineHook / defineStep is now calling this, we have another function on the stack and thus need to look one level further back.

charlierudolph · 2016-08-31T16:20:20Z

Please add a unit test for this to prevent regressions. I believe you can stub out StackTrace.getSync()

Regression from #538 In my setup there is either 1 stackframe or 8 of them. I am not sure this is the perfect fix but it does re-enable running the testsuite

floribon · 2016-09-05T21:50:20Z

@charlierudolph I am really sorry I did not follow the contributing guidelines. This time I ran the tests and my use case and everything works. However I studied the specs codebase but could not figure out how to add a unit test for this.

charlierudolph · 2016-09-08T20:41:38Z

@floribon let me see if I can help you through being able to write a unit test for it.

The unit test would go in
https://github.com/cucumber/cucumber-js/blob/master/spec/cucumber/support_code/library_spec.js

An example of mocking out another module's function
https://github.com/cucumber/cucumber-js/blob/master/spec/cucumber/ast/filter/scenario_at_line_rule_spec.js#L11

What else can I help with?

floribon · 2016-09-08T21:21:27Z

@charlierudolph are stacktrace outputs tested somewhere? I believe that's where one could make sure that it succeeds even if the user overrode Error.prepareStackTrace. This PR aimed to be a bug fix and not a new feature.

Now reading more about #157 I realized that I could avoid the error by using these, which is cleaner:

require('hide-stack-frames-from')('cucumber');
require('hide-stack-frames-from')('selenium-webdriver');

So feel free to reject this PR if it is not helping.

charlierudolph · 2016-09-08T21:24:31Z

They are not tested anywhere at the moment. If you wanted you could just create a feature where you override Error.prepareStackTrace and ensure it works as expected.

floribon · 2016-09-08T21:40:34Z

Pushed, confirmed that it fails on master and passes on this branch.

charlierudolph · 2016-09-08T21:56:42Z

features/custom_stack_trace.feature

+        this.When(/^I override Error.prepareStackTrace$/, function() {
+          Error.prepareStackTrace = function() {
+            return 'Custom message';
+          }


this doesn't fail for me on master. I would expect you to need to override it before this.When() gets called as that is when the stacktrace is captured.

@charlierudolph my bad, it failed for me but because of the missing ✓ which I didn't see initially. I've just pushed an update for the test which this time I made sure were giving the error on master:

(...)/node_modules/cucumber/lib/cucumber/support_code/library.js:53 var line = stackframes[1].getLineNumber();

charlierudolph · 2016-09-18T00:25:37Z

Thank you very much for your contribution!

lock · 2018-10-25T06:16:56Z

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.

hmalphettes changed the title ~~Dont crash if there is a single stackframe~~ 1.2.1 regression: dont crash if there is a single stackframe Jul 6, 2016

charlierudolph reviewed Aug 29, 2016
View reviewed changes

charlierudolph reviewed Aug 31, 2016
View reviewed changes

hmalphettes and others added 2 commits September 5, 2016 23:48

Dont crash if there is a single stackframe

f370fe0

Regression from #538 In my setup there is either 1 stackframe or 8 of them. I am not sure this is the perfect fix but it does re-enable running the testsuite

Use straightforward approach

a44b62c

Add feature test for Error.prepareStackTrace overriding

0c6c555

floribon added 2 commits September 8, 2016 23:48

Merge branch 'cucumber/master' into fix-stacktraces

b16107d

Update test

ad11745

charlierudolph reviewed Sep 8, 2016
View reviewed changes

Update test

a4764db

charlierudolph merged commit f46fa68 into cucumber:master Sep 18, 2016

lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1.2.1 regression: dont crash if there is a single stackframe #605

1.2.1 regression: dont crash if there is a single stackframe #605

hmalphettes commented Jul 6, 2016

hmalphettes commented Jul 6, 2016

charlierudolph commented Jul 6, 2016

hmalphettes commented Jul 7, 2016

floribon commented Aug 10, 2016

charlierudolph Aug 29, 2016

charlierudolph Aug 29, 2016 •

edited

Loading

floribon commented Aug 31, 2016 •

edited

Loading

charlierudolph Aug 31, 2016 •

edited

Loading

charlierudolph commented Aug 31, 2016

floribon commented Sep 5, 2016

charlierudolph commented Sep 8, 2016

floribon commented Sep 8, 2016

charlierudolph commented Sep 8, 2016

floribon commented Sep 8, 2016

charlierudolph Sep 8, 2016

floribon Sep 8, 2016

charlierudolph commented Sep 18, 2016

lock bot commented Oct 25, 2018

1.2.1 regression: dont crash if there is a single stackframe #605

1.2.1 regression: dont crash if there is a single stackframe #605

Conversation

hmalphettes commented Jul 6, 2016

hmalphettes commented Jul 6, 2016

charlierudolph commented Jul 6, 2016

hmalphettes commented Jul 7, 2016

floribon commented Aug 10, 2016

charlierudolph Aug 29, 2016

Choose a reason for hiding this comment

charlierudolph Aug 29, 2016 • edited Loading

Choose a reason for hiding this comment

floribon commented Aug 31, 2016 • edited Loading

charlierudolph Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

charlierudolph commented Aug 31, 2016

floribon commented Sep 5, 2016

charlierudolph commented Sep 8, 2016

floribon commented Sep 8, 2016

charlierudolph commented Sep 8, 2016

floribon commented Sep 8, 2016

charlierudolph Sep 8, 2016

Choose a reason for hiding this comment

floribon Sep 8, 2016

Choose a reason for hiding this comment

charlierudolph commented Sep 18, 2016

lock bot commented Oct 25, 2018

charlierudolph Aug 29, 2016 •

edited

Loading

floribon commented Aug 31, 2016 •

edited

Loading

charlierudolph Aug 31, 2016 •

edited

Loading