Violent crash on stack-trace parsing

osher commented 2 months ago

Describe the bug

Description

I'm trying to evaluate migration to vitest from mocha

All the projects of this codebase use a module that adds Error.prototype.toJSON that represents stack as an array, (the string stack split by \n). We're getting it from packages whose purpose is to make sure that when errors are serialized when sent to ELK and it's like - they comes with all data, and it makes the stacks represented as arrays - which:

lets the user filter them before they are serialized (much like vitest: options.frameFilter does)
forces them to be displayed nicer on the web UI
forces them to be displayed nicer when dumped as yaml to console or files in local-dev mode or troubleshoot mode. (There are more benefits, but they all are related to the concrete project).

However, when I add a setup file that calls this aforementioned module - I get these violent crashes that hide the real error: e.g.:

I tried to do a quick PR https://github.com/vitest-dev/vitest/pull/6407, but this does not seem to be a one-liner fix, so I fall back to the full procedure 🤷

I really hope we can find a solution for this in the time I have left for this migration POC task, or all these projects will have to continue with mocha ...

The project looks amazing - Keep up the good work!

Reproduction

The minimalist form I could find. Please note we don't do it this way, we're using packages that do that and more (stated above).

Steps:

use globals: true in vitest.config.js

use a setup file:

Error.prototype.toJSON = function() {
return {
name: this.name,
message: this.message,
...this,
stack: this.stack?.split('\n'),
}
}

Use a test that fails

describe('stack trace parse failure', () => {
it('fails to parse', () => {
 throw new Error('stack it up')
})
})

System Info

System:
    OS: Linux 5.15 Fedora Linux 39 (Container Image)
    CPU: (12) x64 13th Gen Intel(R) Core(TM) i7-1355U
    Memory: 11.93 GB / 15.46 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 22.6.0 - ~/.volta/tools/image/node/22.6.0/bin/node
    Yarn: 4.1.1 - ~/.volta/tools/image/yarn/4.1.1/bin/yarn
    npm: 10.8.2 - ~/.volta/tools/image/npm/10.8.2/bin/npm
    pnpm: 9.9.0 - ~/.volta/bin/pnpm
  npmPackages:
    vitest: ^2.0.5 => 2.0.5

If it matters - the diagnosis is partial - it's actually a WSL on win11Pro :P

Used Package Manager

npm

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

sheremet-va commented 2 months ago

I do not think it makes sense to support stack as an array as it is a very obscure edge case. But Vitest shouldn't crash here at least. To do that, we need to ignore all non-string stack traces found on Errors.

I also do not think we should add options to bypass this for the same reason - the use case is too obscure to support. But maybe we can print non-string stacks as a property in that case:

https://github.com/vitest-dev/vitest/blob/83638eb9ad58b9addfe74121f7ef2952518aeb96/packages/vitest/src/node/error.ts#L359

osher commented 2 months ago

I can agree with that :)

The basic "contract" with a test-runner is that the true error is visible. That's a must. All the "flying colors" and formatting can be done under SLA of best-effort.

As log as the information get to the screen - that would be good enough :)

If you could give me in the PR a hint on how you expect it be done in the PR - I'll be grateful :) Also - there's the macos check that I suspect to be flaky. If there's anything I can do to mend it - LMK, I'll include it.

vitest-dev / vitest