fix: violent crash in stack-trace parsing

[osher]

Description

I'm using a module that adds Error.prototype.toJSON that represents stack as an array, split by \n.
The purpose is that error stack traces look pretty when spat as JSON and visualized in kibana and it's like.
I'm also use tooling that refine the stack trace more or less like options.frameFilter you got in vitest.
(There are more benefits, but they are related to the concrete project).

I'm trying to migrate to vitest from mocha, however, when I add a setup file that calls this aforementioned module - I get very violent crashes.

e.g.:

Free your mind. Saying that a variable is of type string does not guarantee that it will be a string in runtime... 🤷

The proposed fix puts it back to be a string so your normal flow can continue with no interference :)

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
• Ideally, include a test that fails without this PR but passes with it.
• Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

• Run the tests with pnpm test:ci.

Documentation

• If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

• Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

[netlify]

✅ Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	2b20e2f
🔍 Latest deploy log	https://app.netlify.com/sites/vitest-dev/deploys/670fc492a681e90008dcebf6
😎 Deploy Preview	https://deploy-preview-6407--vitest-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[sheremet-va]

Free your mind. Saying that a variable is of type string does not guarantee that it will be a string in runtime... 🤷

By that logic in can be anything - a function, a number or an array of errors. I'd say the best way to protect the function here is to add a type guard like typeof x === 'string'

[osher]

@sheremet-va - thanks for your reply

If I wanted to support any type and give special treatment to Array and function, I would do something like:

  export function parseStacktrace(
    stack: string | Array<string> | Function,
    options: StackTraceParserOptions = {},
  ): ParsedStack[] {
    const { ignoreStackEntries = stackIgnorePatterns } = options
+   if (typeof stack === 'function') stack = stack();
+   if (Array.isArray(stack)) stack = stack.join('\n');
+   if (typeof stack !== 'string') stack = String(stack);
    let stacks = !CHROME_IE_STACK_REGEXP.test(stack)
      ? parseFFOrSafariStackTrace(stack)
      : parseV8Stacktrace(stack)

But I failed a build and resorted to new var stackStr.
I normally work in pure JS, and I don't know how to convince tc to let it be - I can use a point in the right direction :)

---

I need help to understand what happened on node-20, macos-14 - I only see "exit with code 1" 🤔
is it a flaky one?

---

But if we're here already - there's another problem with the logic:

the regex if is naive. It assumes one of two formats FF_IE/V8.

In one of my attempts, I got this when I tried to return "my" Array as is - which is a filtered array of strings:

This result hinted it does not expect an Array of strings, but an array of parsed stackFrames.

Anyway. The correction should consider 3 cases - FF_IE/V8/unsupported.

To minimize impact on the view/output layer - I can propose that the unsupported case will return an array of one element that displaying it will result in the raw unparsed string propagated to the screen, to give the user ...something.

I'm a little reluctant to include such a change in this PR.
This PR aims to fix the violent crash - and it does...

I can propose to start another after this more urgent fix landed.

If we're going for a 2nd PR - I believe I should add tests with it.
However - I could not find the tests for util - some directions would help.
And again - I'll need help with TS...
But we can take that discussion to the other PR :)

[sheremet-va]

If I wanted to support any type and give special treatment to Array and function, I would do something like:

I was saying that we should not support anything other than stack string. If it's not a string, don't process it.

[osher]

First - wow. you're a fast one.
I updated my comment above to be clearer... saw you already replied after I finished

please check it...

Anyway -

If we don't process anything non-string that would leave the user with no details on screen
wouldn't that be a regression of SLA?

[sheremet-va]

But that would leave the user with no details on screen wouldn't that be a regression of SLA?

No, that wouldn't be a regression because it doesn't work already. Assuming what users mean if stack is for some reason not a string is out of scope of Vitest.

[osher]

Would you please consider the case for a moment?

You run a test, you get a fail and you don't get a stack trace. No failed assertion, no stack trace, no reason on screen.
Would you trust the test runner?

I suggest that in that case - a violent crash - as bad as it is - is better, because it gave me a line number I could debug from mu node_modules.

Or am I getting it wrong? Is there a fallback to display with node:util.inspect errors the code could not normalize?

[sheremet-va]

Would you please consider the case for a moment?

You run a test, you get a fail and you don't get a stack trace. No failed assertion, no stack trace, no reason on screen. Would you trust the test runner?

I suggest that in that case - a violent crash - as bad as it is - is better, because it gave me a line number I could debug from mu node_modules.

Or am I getting it wrong? Is there a fallback to display with node:util.inspect errors the code could not normalize?

Don't patch built-ins if you want to see the stack trace. It's that simple.

[osher]

Don't patch built-ins if you want to see the stack trace. It's that simple.

That's what the project requirements are...
This is not even the project per-se, it's using a library for that.

So what you're saying I can't use vitest with this project, or any project that wants such logic?

[sheremet-va]

Don't patch built-ins if you want to see the stack trace. It's that simple.

That's what the project requirements are... This is not even the project per-se, it's using a library for that.

So what you're saying I can't use vitest with this project, or any project that wants such logic?

You can use it, you just won't see the stack trace.

[osher]

You can use it, you just won't see the stack trace

And not know what the test failed for?
Sir, I'm sorry, this is a basic expectation from a test-runner and would render vitest not usable for such projects :(

If you intend to add a simple string-guard,
Would you please consider any kind of fallback that will let the user have their raw error / raw stack trace?
In this case you regress only the frame-filtering feature, not the entire stack trace.

Edited:

• please also note that the proposed change puts you back on your normal track.

[osher]

🤔 I can propose one more thing - an options.errorNormalizer: (err:Error) => Error or an options.stackNormalizer: (any) => string each will involve more moving parts and result in a much bigger change... 🤷

Im not telling you what to do - you do you, and thanks for all the effort :)

I just really do hope we can find a solution for this in the time I have to evaluate the migration from mocha to vitest

[osher]

hmm. I suspect a flaky one. It passed before I merged changes from the upstream...
🟢 CI / Build&Test: node-20, macos-14 (pull_request)

I also do not understand the error, but would love to treat it if I could

[osher]

is there a slack or a discord I can get help in?

I started with editing one line on the github web editor, I now have a cloned the repo locally, ran pnpm i and everything, and the setup won't run out of the box... 🤔

maybe that's something I did. back to main.

ok. so I ran pnpm run test:ci
and I got

please help...

[sheremet-va]

Yeah, sorry, the contribution guide is very outdated. There is a discord where you can ask questions: https://chat.vitest.dev/