Skip to content

Fix stack trace if there are anonymous functions in stack trace#24

Merged
steren merged 2 commits intoGoogleCloudPlatform:masterfrom
Jastrzebowski:feature-21-anonymous-in-stack-trace
Nov 17, 2017
Merged

Fix stack trace if there are anonymous functions in stack trace#24
steren merged 2 commits intoGoogleCloudPlatform:masterfrom
Jastrzebowski:feature-21-anonymous-in-stack-trace

Conversation

@Jastrzebowski
Copy link
Copy Markdown
Contributor

  • Add <anonymous> in stack trace if there is no functionName
  • Add command using scripts in package.json
  • Add tests
  • Remove unused code in test.html
  • Update version in package lock
  • Update readme to include test instructions

Problem with issue #21 is due to errors thrown in anonymous scope, V8 stack trace format expects <anonymous> in this case. I've added a simple fallback for this case and some tests to try it (I'm not 100% happy with anonymous one but I have no idea how to throw an error in the global anonymous scope from inside the tests). Still, I think we should review the whole V8 format and check if we cover every case and definitely write tests to cover all the cases.

As for other changes, I've added scripts commands in package.json so no global Gulp required anymore — Readme updated too.

* Add <anonymous> in stack trace if there is no functionName
* Add command using scripts in package.json
* Add tests
* Remove unused code in test.html
* Update version in package lock
* Update readme to include test instructions
@steren
Copy link
Copy Markdown
Collaborator

steren commented Nov 15, 2017

Thanks a lot, I will take a closer look within a day.

Copy link
Copy Markdown
Collaborator

@steren steren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. One small comment on the test.

Comment thread test/test.js Outdated
}
});

it('should extract and send <anonymous> in stack traces', function (done) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the test name to
"should set in stack traces when frame is anonymous"

Comment thread README.md
* Run `gulp` to test your changes.
* Run `gulp dist` generates the minified version.
* Run `npm test` or `yarn run test` to test your changes.
* Run `npm run dist` or `yarn run dist` generates the minified version.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're welcome, still, find NPM scripts an underappreciated feature :) perfect for simple commands.

Comment thread test/test.js
throwError(message)
} catch(e) {
errorHandler.report(e, function() {
expectRequestWithMessage('<anonymous>');
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, it implicitly relies on the internal implementation of Mocha (Mocha uses anonymous functions in its execution)
Could you instead use an anonymous function in this test file? So that we can clearly understand why the stack will contain

@steren
Copy link
Copy Markdown
Collaborator

steren commented Nov 16, 2017

Due to the fact that the sent stacktraces might contain more frames (with ), which might impact the error grouping algorithm, when this change is merged, I will increment the major version of the library.

* Update test message
* Use explicite anonymous function in anonymous test case
@steren steren merged commit 8fab071 into GoogleCloudPlatform:master Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants