Skip to content

Include anonymous functions (whose name node cannot infer) in stack traces #1313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

neoeno
Copy link

@neoeno neoeno commented Mar 19, 2017

Issue

Functions whose name node cannot infer do not get included in stack traces.

I came across this issue when working with curried functions.

Reproduction

This error in a code using an anonymous function:

const fn = () => () => doesnotexist
fn()()

Produces a stacktrace line without a function name:

ReferenceError: doesnotexist is not defined
    at /Users/caden/Code/libs/ava/repro.js:1:86
    at Object.<anonymous> (/Users/caden/Code/libs/ava/repro.js:2:5)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)
    at Module.runMain (module.js:590:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)

The regex in extract-stack.js:2 expects the function name & brackets to be there:

const stackLineRegex = /^.+ \(.+:[0-9]+:[0-9]+\)$/;

So that line gets swallowed. Which means we lose context for our error message. This is a shame.

Fix

This commit fixes the issue by adding another regex to extract-stack.js which will match anonymous functions.

The regex may be a little broad — I couldn't think of any obvious instances where it would fail but undoubtedly some contrived examples exist.

I have included a unit test and an integration test. A few different areas in the codebase amend stacktraces, so I thought it was best to do so.

Finally

Thanks for all your great work on Ava!

@novemberborn
Copy link
Member

@neoeno could you share an example of how this changes AVA's output, and what is missing in the current output? The stack trace you shared comes from Node.js itself but I can't quite see how it impacts AVA's output.

@neoeno
Copy link
Author

neoeno commented Mar 20, 2017

Of course @novemberborn — sorry I didn't do this already!

Here's a repo with reproduction, output both before and after this PR: https://github.com/neoeno/ava-pr-1313-example

For posterity, here's the code:

import test from 'ava';

const fn = () => () => doesnotexist;

test("some test", t => {
  fn()();
});

Here's the current output (as of 3da8603):

$ ava test.js

  1 failed

  some test
  /Users/kay/Code/libs/tester/test.js:6

   5: test("some test", t => {
   6:   fn()();
   7: });

  Error:

    [ReferenceError: doesnotexist is not defined]

  Error thrown in test

And here's the output after this PR:

$ ava test.js

  1 failed

  some test
  /Users/kay/Code/libs/tester/test.js:3

   2:
   3: const fn = () => () => doesnotexist;
   4:

  Error:

    [ReferenceError: doesnotexist is not defined]

  Error thrown in test

  test.js:3:24
  Test.t [as fn] (test.js:6:3)

@neoeno
Copy link
Author

neoeno commented Mar 20, 2017

I'll fix that formatting error too 😨

@neoeno neoeno force-pushed the include-anonymous-functions-in-stacktraces branch from 15a464d to 5cfc60a Compare March 20, 2017 10:22
@novemberborn
Copy link
Member

@vadimdemedes how did you arrive at the regular expression? Looks like extract-stack was introduced in #1154.


module.exports = stack => {
return stack
.split('\n')
.filter(line => stackLineRegex.test(line))
.filter(line => namedFunctionStackLineRegex.test(line) || anonymousFunctionStackLineRegex.test(line))
Copy link
Member

Choose a reason for hiding this comment

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

I think you should combine this into a singular regular expression.

Copy link
Author

Choose a reason for hiding this comment

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

As you prefer!

Some functions are more anonymous than others. Node does a pretty
good job of naming functions like this:

```js
const fn = () => { throw "whoops!" } // <-- gets named `fn` in stacktraces
```

But in some cases a function genuinely has no name, e.g.

```js
const fn = () => () => { throw "whoops!" } // <-- inner function has no name!
```

This means we get stacktraces like this:

```
whoops!
  path/to/file.js:1:1
  Test.t (test.js:1:1)
```

Before this commit, the regex in `extract-stack.js` was not anticipating
stacktraces of this form, and would simply filter out the anonymous functions.
This made my life a little more awkward, as I could only see the failing test
line and the error, not the code that was actually erroring.

This commit adds another regex to `extract-stack.js` which matches on anonymous
function stacktrace lines too.
@neoeno neoeno force-pushed the include-anonymous-functions-in-stacktraces branch from 5cfc60a to 4826582 Compare March 20, 2017 12:28
@vadimdemedes
Copy link
Contributor

@novemberborn I think I tried to make a regex that'd be as simple as possible and match "standard" stack lines. The point of that was to get only stack from err.stack.

@sindresorhus
Copy link
Member

I think the regex could still be looser. I decided to extract it into a separate module as it can be useful for other projects too: https://github.com/sindresorhus/extract-stack

@neoeno Can you update to use that module and remove lib/extract-stack.js and its tests?

@neoeno
Copy link
Author

neoeno commented Mar 26, 2017

I'll give it a go — though afaict it won't be as simple as replacing extract-stack.js with the new module (which is great by the way!).

This is because at the moment extractStack accepts a pre-processed stack-trace, of of this form:

error message
  Test.t (test.js:1:1)

No ats — which from reading the code I think the extract-stack module relies upon?

Here's where my knowledge gets a bit hazy, but it looks like it passes through clean-stack elsewhere — removing the ats as well as some other stuff — via paths like this: main.js:37 -> serialize-error.js:52 -> beautify-stack:27 -> clean-stack. There may be one or two more...

So the solution would seem to be to do all this stacky work in one place and delegate to extract-stack its due share — but I've no idea how much work that will be. Might be simple anyway, so I'll give it a shot and get back to you.

@novemberborn
Copy link
Member

My hunch is that we should extract the stack when serializing (filter out unnecessary bits early), and beautify it in the mini and verbose reporters. Though I'm not even sure what beautify-stack does that extract-stack doesn't.

@bchapman
Copy link

Hit the same issue. I'm using an async wrapper function inside of a test, and any code inside loses it's context in the traceback. This would be super helpful to get the error context back.

@patrickrand
Copy link

patrickrand commented Apr 13, 2017

+1

@novemberborn
Copy link
Member

@neoeno will you have a chance to get back to this? If not, perhaps @bchapman, @zabawaba99 or @patrickrand want to give it a go? 😄

@neoeno
Copy link
Author

neoeno commented Apr 14, 2017

Though this is still on my radar I suspect I won't get to it imminently. As such, if anyone is keen to take it on I should certainly give way!

@zabawaba99
Copy link

I don't think I'll be able to get to this immediately either but I will post if I'm able to start looking into it!

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.

7 participants