vitest icon indicating copy to clipboard operation
vitest copied to clipboard

Violent crash on stack-trace parsing

Open osher opened this issue 1 year ago • 2 comments

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.: image

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:

  1. use globals: true in vitest.config.js
  2. use a setup file:
Error.prototype.toJSON = function() {
  return {
    name: this.name,
    message: this.message,
    ...this,
    stack: this.stack?.split('\n'),
  }
}
  1. 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

osher avatar Aug 27 '24 08:08 osher

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

sheremet-va avatar Aug 27 '24 09:08 sheremet-va

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.

osher avatar Aug 28 '24 10:08 osher

Seems like this isn't that obscure -- happening to me fairly regularly with different packages, all with array-based stacks.

mattkindy avatar Jan 10 '25 22:01 mattkindy

happening to me fairly regularly with different packages, all with array-based stacks.

@mattkindy Can you list such packages? I don't think we are familiar with this convention, so knowing concrete examples would help if you want to push the PR.

hi-ogawa avatar Jan 11 '25 02:01 hi-ogawa

happening to me fairly regularly with different packages, all with array-based stacks.

@mattkindy Can you list such packages? I don't think we are familiar with this convention, so knowing concrete examples would help if you want to push the PR.

@hi-ogawa Apologies, as it seems like I was a bit off.

TL;DR: jsonify-error was being pulled in by a dependency and patching in the toJSON() method for Error. I added an entry to setupFiles to undo this.

/**
 * This file removes the Error.prototype.toJSON method that gets added by
 * breaking-dep via the jsonify-error package's overrideErrorMethods().
 *
 * Background:
 * - jsonify-error modifies Error.prototype to add a toJSON method for better error serialization
 * - This causes conflicts with Vitest's own error handling/reporting mechanisms
 * - When both are active, error objects end up with an array-based stack trace instead
 *   of the standard string format, breaking error handling assumptions
 *
 * This setup file ensures we get standard Node.js error behavior in our tests by
 * removing the custom toJSON method before any tests run.
 */
import 'breaking-dep';

if (Error.prototype.hasOwnProperty('toJSON')) {
  delete Error.prototype['toJSON'];
}

Take the example of the openai package. It throws an error if it's missing an environment variable for the API key. That error is just

export class OpenAIError extends Error {}

I had caught and logged that error and saw an array stack, but it seems like that was just a quirk of my logger config. console.log / console.dir on the same object gave me a string stack. So, great, right?

However, I was still seeing

TypeError: stack.split is not a function
 ❯ parseFFOrSafariStackTrace node_modules/@vitest/utils/dist/source-map.js:936:16
 ❯ parseStacktrace node_modules/@vitest/utils/dist/source-map.js:912:54
 ❯ parseErrorStacktrace node_modules/@vitest/utils/dist/source-map.js:955:21
 ❯ Object.parseErrorStacktrace node_modules/vitest/dist/chunks/index.DsZFoqi9.js:3089:16
 ❯ printError node_modules/vitest/dist/chunks/index.DsZFoqi9.js:2732:26
 ❯ Logger.printError node_modules/vitest/dist/chunks/index.DsZFoqi9.js:3077:12
 ❯ DefaultReporter.printTaskErrors node_modules/vitest/dist/chunks/index.DsZFoqi9.js:3761:23
 ❯ DefaultReporter.printErrorsSummary node_modules/vitest/dist/chunks/index.DsZFoqi9.js:3695:12
 ❯ DefaultReporter.reportSummary node_modules/vitest/dist/chunks/index.DsZFoqi9.js:3619:10
 ❯ DefaultReporter.onFinished node_modules/vitest/dist/chunks/index.DsZFoqi9.js:3423:10

I started to dig around in node_modules/@vitest/utils/dist/source-map.js, and noticed this code:

function parseErrorStacktrace(e, options = {}) {
  if (!e || isPrimitive(e)) {
    return [];
  }
  if (e.stacks) {
    return e.stacks;
  }
  const stackStr = e.stack || e.stackStr || "";
  let stackFrames = parseStacktrace(stackStr, options);
  if (options.frameFilter) {
    stackFrames = stackFrames.filter(
      (f) => options.frameFilter(e, f) !== false
    );
  }
  e.stacks = stackFrames;
  return stackFrames;
}

I logged out e here, and got the following

{
    name: 'Error',
    className: 'OpenAIError',
    message: "The OPENAI_API_KEY environment variable is missing or empty; either provide it, or instantiate the OpenAI client with an apiKey option, like new OpenAI({ apiKey: 'My API Key' }).",
    superclasses: [ 'Error', 'Object' ],
    enumerableFields: {
      stackStr: "Error: The OPENAI_API_KEY environment variable is missing or empty; either provide it, or instantiate the OpenAI client with an apiKey option, like new OpenAI({ apiKey: 'My API Key' }).\n" +
        '    at new OpenAI (file:///Users/mattkindy/code/node_modules/openai/src/index.ts:122:13)\n' +
        '    at /Users/mattkindy/code/src/server/service/openai:9:12\n' +
        '    at processTicksAndRejections (node:internal/process/task_queues:95:5)\n' +
        '    at VitestExecutor.runModule (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:399:5)\n' +
        '    at VitestExecutor.directRequest (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:381:5)\n' +
        '    at VitestExecutor.cachedRequest (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:206:14)\n' +
        '    at VitestExecutor.dependencyRequest (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:259:12)\n' +
        '    at /Users/mattkindy/code/src/server/service/tools/conversation/util.test.ts:2:31\n' +
        '    at VitestExecutor.runModule (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:399:5)\n' +
        '    at VitestExecutor.directRequest (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:381:5)\n' +
        '    at VitestExecutor.cachedRequest (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:206:14)\n' +
        '    at VitestExecutor.executeId (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:173:12)\n' +
        '    at collectTests (file:///Users/mattkindy/code/node_modules/@vitest/runner/dist/index.js:851:7)\n' +
        '    at startTests (file:///Users/mattkindy/code/node_modules/@vitest/runner/dist/index.js:1268:17)\n' +
        '    at file:///Users/mattkindy/code/node_modules/vitest/dist/chunks/runBaseTests.3qpJUEJM.js:126:11\n' +
        '    at withEnv (file:///Users/mattkindy/code/node_modules/vitest/dist/chunks/runBaseTests.3qpJUEJM.js:90:5)',
      nameStr: 'Error',
      expected: 'undefined',
      actual: 'undefined'
    },
    stack: [
      "Error: The OPENAI_API_KEY environment variable is missing or empty; either provide it, or instantiate the OpenAI client with an apiKey option, like new OpenAI({ apiKey: 'My API Key' }).",
      'at new OpenAI (file:///Users/mattkindy/code/node_modules/openai/src/index.ts:122:13)',
      'at /Users/mattkindy/code/src/server/service/openai:9:12',
      'at processTicksAndRejections (node:internal/process/task_queues:95:5)',
      'at VitestExecutor.runModule (file:///Users/mattkindy/code/node_modules/vite-node/dist/client.mjs:399:5)',
     // ...
    ]
  }

Of course, at this point, we know that const stackStr = e.stack || e.stackStr || ""; will eventually end up as an array... but how?

So in processError:

  // stack is not serialized in worker communication
  // we stringify it first
  if (err.stack) {
    err.stackStr = String(err.stack)
  }

However, in my local logging, err.stack is already a string, so this is redundant at worst. I found

return serializeValue(err);

serializeValue: which has

  if (typeof val.toJSON === 'function') {
    return serializeValue(val.toJSON(), seen)
  }

And I found that somehow this error class does in fact have a toJSON which turns the stack into an array... I tried with just throw new Error(), and that error, when .toJSON() is invoked, has the same default.

After more digging, I found that I had a dependency that was augmenting the Error class via jsonify-error, adding thetoJSONmethod. So that's the root cause at the end of the day.

mattkindy avatar Jan 11 '25 19:01 mattkindy

Closed by https://github.com/vitest-dev/vitest/pull/7995

hi-ogawa avatar May 28 '25 02:05 hi-ogawa