Retain (most of) Node.js internals in stack traces #2420

bunysae · 2020-03-09T15:12:56Z

Fixes issue #2110. The node internal stack trace is shown for error originated from Node.js. If it's originated from custom code, node internal stack traces are culled out.
The code-property is shown in node v12.16.1 without any modification to ava-source-code. In older versions (i had v12.10.0) before, the code-property wasn't a enumerable property and therefore doesn't appear.

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#2110: Display internal Node.js stacktrace if no user stacktrace is present

IssueHunt has been backed by the following sponsors. Become a sponsor

novemberborn · 2020-03-16T08:36:26Z

Thanks for your PR @bunysae. I did not open my editor this weekend so I haven't had time yet to look into this, sorry.

novemberborn · 2020-03-21T16:17:27Z

I've spent some time trying to figure out what we're trying to do here. #2110 is an older issue, we recently came to the conclusion that it'd be good to show all lines, but use colors to indicate the less relevant ones. See #2412 for that discussion.

That's probably more than we need to deal with in this PR. However as you probably found the code here is rather messy.

I suspect we can remove clean-stack, and rely solely on stack-utils. We haven't kept pace with the changes in stack-utils so we need to make better use of its current features.

There's another mess in lib/serialize-error.js which ends up relying on the beautified stack trace:

ava/lib/serialize-error.js

Line 58 in b6a46bd

stack = beautifyStack(stack);

Going by your test, the output for a file like this:

const path = require('path')
const test = require('ava')

test('foo', t => {
  path.resolve({root: '..'})
  t.pass()
})

Goes from:

npx ava -v

  ✖ foo Error thrown in test

  1 test failed

  foo

  test.js:5

   4: test('foo', t => {
   5:   path.resolve({root: '..'})
   6:   t.pass()

  Error thrown in test:

  TypeError (NodeError) {
    code: 'ERR_INVALID_ARG_TYPE',
    message: 'The "path" argument must be of type string. Received type object',
  }

To:

npx ava -v

  ✖ foo Error thrown in test

  1 test failed

  foo

  internal/validators.js:112

  Error thrown in test:

  TypeError (NodeError) {
    code: 'ERR_INVALID_ARG_TYPE',
    message: 'The "path" argument must be of type string. Received type object',
  }

  validateString (internal/validators.js:112:11)
  Object.resolve (path.js:980:7)
  test.js:5:8
  processTicksAndRejections (internal/process/task_queues.js:94:5)

Note how we're no longer showing the test code in the error output. Arguably, the error did not originate in Node.js, but it originated in the test.

#2110 describes a situation where the error occurred asynchronously, outside of a test invocation.

Probably the right approach here is to simplify the error formatting and the various ways in which we munge the stack, and then see how errors end up being reported. What do you think @bunysae?

bunysae · 2020-03-22T10:45:39Z

@novemberborn
I understand your problem with this PR. When i understand the discussion in #2412 correctly, you want still cull out ava internal stack lines and give the user and node internal stack lines different colors?
I think i can do this with the chalk-lib.

novemberborn · 2020-03-22T12:14:02Z

When i understand the discussion in #2412 correctly, you want still cull out ava internal stack lines and give the user and node internal stack lines different colors?

Yes. However this is made more difficult due to mess I described above. We should try and clear that up first.

bunysae · 2020-03-24T15:04:16Z

I refactored this PR. Now the complete stack-trace is shown except:

ava internals,
ava dependencies
the node internals internal/process/task-queues and internal/modules/cjs/loader (They add noice to ava assertion errors and errors thrown due to impromper test files).
Node internals are grey. Custom-code has the default color.

I also added an option to cull out node internals. This is useful for the CI-Build as well as for developers, how want to have the current behavior.

How to you want the show the code-excerpt? In the path-example above, the code excerpt would be still omitted. Should ava always show an excerpt of the deepest layer (custom-code)?

novemberborn · 2020-03-26T07:20:10Z

@bunysae thanks for the update. I'll try and have a look at this on the weekend.

novemberborn

Now the complete stack-trace is shown except:

ava internals,

ava dependencies

the node internals internal/process/task-queues and internal/modules/cjs/loader (They add noice to ava assertion errors and errors thrown due to impromper test files).
Node internals are grey. Custom-code has the default color.

👍

I also added an option to cull out node internals. This is useful for the CI-Build as well as for developers, how want to have the current behavior.

I don't think we need that just yet. Let's see how folks get on seeing more stack trace lines.

The reporter tests can be quite tricky to get working across the Node.js versions and OS. I can help with that.

P.S. the worker processes have access to an options object, there's no need to use additional environment variables.

How to you want the show the code-excerpt? In the path-example above, the code excerpt would be still omitted. Should ava always show an excerpt of the deepest layer (custom-code)?

I think we should update serialize-error to go through the stack trace and find the first call site that came from the test file, and then show that.

lib/beautify-stack.js

lib/reporters/mini.js

test/beautify-stack.js

bunysae · 2020-04-04T09:41:14Z

My idea to handle the stack-traces of different nodes.js versions and OSes is to create different log-files for every combination. Then we wouldn't had a problem with a failing CI-build.

novemberborn · 2020-04-04T11:50:26Z

My idea to handle the stack-traces of different nodes.js versions and OSes is to create different log-files for every combination. Then we wouldn't had a problem with a failing CI-build.

Yes, sounds good. It'll be a bit harder to update the files, but that should still be easier than trying to string-replace away the differences.

bunysae · 2020-04-14T17:53:47Z

I moved the stack processing to the reporters and splited the log-files for the three major node versions.

novemberborn · 2020-04-15T08:03:29Z

@bunysae awesome! It probably won't be until the weekend that I'll have a chance to look at this though.

* Use the ignoredPackages option from stack-utils * Ignore all cjs internals * Keep ignoring some Node.js internals (there's useful stuff here: https://github.com/tapjs/stack-utils/blob/d3c7ee2a7d46744c271a36ce3acfd69465f36df5/index.js#L12-L18) * Return an array of lines * Remove dedicated tests, rely on reporter integration tests instead

Dim internal lines, leave normal lines as grey. Add a pointer so it looks a bit more like a list of lines

novemberborn

@bunysae I've made a bunch more changes but I think this is ready now. Would you mind reviewing my changes?

There's two review comments below but they're already outdated.

(Let's see if I managed to not break the tests…)

novemberborn · 2020-04-19T10:37:05Z

lib/serialize-error.js

+			// The `callSite.file` property is platform independent (with '/' as directory separator)
+			// and `testFile` is platform dependent (on win32 with '\' as directory separator).
+			if (callSite.file === path.relative(process.cwd(), testFile).split(path.sep).join('/')) {
+				result = callSite;


I like how this tries to find the first call site in the test file. However this takes the last call site, rather than the first. Is that intentional?

No that wasn't intentional. I didn't realize that.

novemberborn · 2020-04-19T10:38:41Z

lib/serialize-error.js

+	let result = null;
+	stack.split('\n').forEach(line => {
+		try {
+			const callSite = stackUtils.parseLine(line.replace('file:///', '/'));


These are meant to have come from esm but I can't reproduce that anymore, at least not for the use case of extracting the source within the test file.

file:/// is replaced to pass the test (https://github.com/bunysae/ava/blob/7c5ebac7adb4183393d02ee6e6f5b237e5f6aa1e/test-tap/serialize-error.js#L128). I can try it out, if the actual esm version still inserts file-urls. If not, i think we can omit it.

bunysae · 2020-04-19T15:15:08Z

@novemberborn Except the open-point with the esm file-url's it looks goods. Didn't find more todos in my review.

novemberborn · 2020-04-19T15:15:59Z

Except the open-point with the esm file-url's it looks goods. Didn't find more todos in my review.

I couldn't reproduce that problem with the latest esm so I think we're good!

novemberborn · 2020-04-19T15:56:10Z

I'm just asking some folks for feedback outside of GitHub. Stay tuned 😄

novemberborn mentioned this pull request Mar 21, 2020

Retain internal stack lines for errors, but color differently #2412

Closed

novemberborn requested changes Mar 28, 2020

View reviewed changes

lib/beautify-stack.js Outdated Show resolved Hide resolved

lib/reporters/mini.js Outdated Show resolved Hide resolved

test/beautify-stack.js Outdated Show resolved Hide resolved

bunysae added 6 commits April 4, 2020 15:07

Show node internal stack trace

6729fce

Moved stack processing to the reporters

8bb1dea

Bugfix: support windows

5015ee7

unit-test: create separate log-files for major node versions

0d18b24

Bugfix: support windows

4bf273f

unit-test: create separate log-files for node v13.5.0

7c5ebac

novemberborn added 8 commits April 19, 2020 14:36

Merge branch 'master' into HEAD

f8d545a

Update readme for reporter tests

84af368

Simplify source extraction

c4b7af5

Tweak stack formatting

3a56b4a

Dim internal lines, leave normal lines as grey. Add a pointer so it looks a bit more like a list of lines

Remove some reporter sanitizers now we report by Node.js version

88a22e9

Remove low-level promise stuff from stacks

bd2ccf6

Update reporter logs

7568f2a

novemberborn approved these changes Apr 19, 2020

View reviewed changes

Windows…

ee9ddeb

novemberborn changed the title ~~Show node internal stack trace~~ Retain (most of) Node.js internals in stack traces Apr 23, 2020

novemberborn merged commit 9a9351d into avajs:master Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retain (most of) Node.js internals in stack traces #2420

Retain (most of) Node.js internals in stack traces #2420

bunysae commented Mar 9, 2020 •

edited by issuehunt-oss bot

Loading

novemberborn commented Mar 16, 2020

novemberborn commented Mar 21, 2020

bunysae commented Mar 22, 2020

novemberborn commented Mar 22, 2020

bunysae commented Mar 24, 2020 •

edited

Loading

novemberborn commented Mar 26, 2020

novemberborn left a comment

bunysae commented Apr 4, 2020 via email

novemberborn commented Apr 4, 2020

bunysae commented Apr 14, 2020

novemberborn commented Apr 15, 2020

novemberborn left a comment

novemberborn Apr 19, 2020

bunysae Apr 19, 2020

novemberborn Apr 19, 2020

bunysae Apr 19, 2020 •

edited

Loading

bunysae commented Apr 19, 2020

novemberborn commented Apr 19, 2020

novemberborn commented Apr 19, 2020

Retain (most of) Node.js internals in stack traces #2420

Retain (most of) Node.js internals in stack traces #2420

Conversation

bunysae commented Mar 9, 2020 • edited by issuehunt-oss bot Loading

Referenced issues

novemberborn commented Mar 16, 2020

novemberborn commented Mar 21, 2020

bunysae commented Mar 22, 2020

novemberborn commented Mar 22, 2020

bunysae commented Mar 24, 2020 • edited Loading

novemberborn commented Mar 26, 2020

novemberborn left a comment

Choose a reason for hiding this comment

bunysae commented Apr 4, 2020 via email

novemberborn commented Apr 4, 2020

bunysae commented Apr 14, 2020

novemberborn commented Apr 15, 2020

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn Apr 19, 2020

Choose a reason for hiding this comment

bunysae Apr 19, 2020

Choose a reason for hiding this comment

novemberborn Apr 19, 2020

Choose a reason for hiding this comment

bunysae Apr 19, 2020 • edited Loading

Choose a reason for hiding this comment

bunysae commented Apr 19, 2020

novemberborn commented Apr 19, 2020

novemberborn commented Apr 19, 2020

bunysae commented Mar 9, 2020 •

edited by issuehunt-oss bot

Loading

bunysae commented Mar 24, 2020 •

edited

Loading

bunysae Apr 19, 2020 •

edited

Loading