fix: capturePrintError logger duplicate event handlers (#7197)

vitest-dev · Jan 9, 2025 · e89c369 · e89c369
1 parent 57b671d
commit e89c369
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 77 deletions.
diff --git a/packages/vitest/src/node/error.ts b/packages/vitest/src/node/error.ts
@@ -1,8 +1,9 @@
+/* eslint-disable prefer-template */
 import type { ErrorWithDiff, ParsedStack } from '@vitest/utils'
 import type { Vitest } from './core'
-import type { ErrorOptions } from './logger'
+import type { ErrorOptions, Logger } from './logger'
 import type { TestProject } from './project'
-/* eslint-disable prefer-template */
+import { Console } from 'node:console'
 import { existsSync, readFileSync } from 'node:fs'
 import { Writable } from 'node:stream'
 import { stripVTControlCharacters } from 'node:util'
@@ -12,22 +13,24 @@ import c from 'tinyrainbow'
 import { TypeCheckError } from '../typecheck/typechecker'
 import {
   lineSplitRE,
+  parseErrorStacktrace,
   positionToOffset,
 } from '../utils/source-map'
-import { Logger } from './logger'
 import { F_POINTER } from './reporters/renderers/figures'
 import { divider, truncateString } from './reporters/renderers/utils'
 
+type ErrorLogger = Pick<Logger, 'error' | 'highlight'>
+
 interface PrintErrorOptions {
+  logger: ErrorLogger
   type?: string
-  logger: Logger
   showCodeFrame?: boolean
   printProperties?: boolean
   screenshotPaths?: string[]
   parseErrorStacktrace: (error: ErrorWithDiff) => ParsedStack[]
 }
 
-export interface PrintErrorResult {
+interface PrintErrorResult {
   nearest?: ParsedStack
 }
 
@@ -44,15 +47,52 @@ export function capturePrintError(
       callback()
     },
   })
-  const logger = new Logger(ctx, writable, writable)
-  const result = logger.printError(error, {
+  const console = new Console(writable)
+  const logger: ErrorLogger = {
+    error: console.error.bind(console),
+    highlight: ctx.logger.highlight.bind(ctx.logger),
+  }
+  const result = printError(error, ctx, logger, {
     showCodeFrame: false,
     ...options,
   })
   return { nearest: result?.nearest, output }
 }
 
 export function printError(
+  error: unknown,
+  ctx: Vitest,
+  logger: ErrorLogger,
+  options: ErrorOptions,
+) {
+  const project = options.project
+    ?? ctx.coreWorkspaceProject
+    ?? ctx.projects[0]
+  return printErrorInner(error, project, {
+    logger,
+    type: options.type,
+    showCodeFrame: options.showCodeFrame,
+    screenshotPaths: options.screenshotPaths,
+    printProperties: options.verbose,
+    parseErrorStacktrace(error) {
+      // browser stack trace needs to be processed differently,
+      // so there is a separate method for that
+      if (options.task?.file.pool === 'browser' && project.browser) {
+        return project.browser.parseErrorStacktrace(error, {
+          ignoreStackEntries: options.fullStack ? [] : undefined,
+        })
+      }
+
+      // node.js stack trace already has correct source map locations
+      return parseErrorStacktrace(error, {
+        frameFilter: project.config.onStackTrace,
+        ignoreStackEntries: options.fullStack ? [] : undefined,
+      })
+    },
+  })
+}
+
+function printErrorInner(
   error: unknown,
   project: TestProject | undefined,
   options: PrintErrorOptions,
@@ -129,7 +169,7 @@ export function printError(
 
   // E.g. AssertionError from assert does not set showDiff but has both actual and expected properties
   if (e.diff) {
-    displayDiff(e.diff, logger.console)
+    logger.error(`\n${e.diff}\n`)
   }
 
   // if the error provide the frame
@@ -193,7 +233,7 @@ export function printError(
 
   if (typeof e.cause === 'object' && e.cause && 'name' in e.cause) {
     (e.cause as any).name = `Caused by: ${(e.cause as any).name}`
-    printError(e.cause, project, {
+    printErrorInner(e.cause, project, {
       showCodeFrame: false,
       logger: options.logger,
       parseErrorStacktrace: options.parseErrorStacktrace,
@@ -251,7 +291,7 @@ const esmErrors = [
   'Unexpected token \'export\'',
 ]
 
-function handleImportOutsideModuleError(stack: string, logger: Logger) {
+function handleImportOutsideModuleError(stack: string, logger: ErrorLogger) {
   if (!esmErrors.some(e => stack.includes(e))) {
     return
   }
@@ -274,7 +314,7 @@ function handleImportOutsideModuleError(stack: string, logger: Logger) {
 }
 
 function printModuleWarningForPackage(
-  logger: Logger,
+  logger: ErrorLogger,
   path: string,
   name: string,
 ) {
@@ -305,7 +345,7 @@ function printModuleWarningForPackage(
   )
 }
 
-function printModuleWarningForSourceCode(logger: Logger, path: string) {
+function printModuleWarningForSourceCode(logger: ErrorLogger, path: string) {
   logger.error(
     c.yellow(
       `Module ${path} seems to be an ES Module but shipped in a CommonJS package. `
@@ -314,13 +354,7 @@ function printModuleWarningForSourceCode(logger: Logger, path: string) {
   )
 }
 
-export function displayDiff(diff: string | undefined, console: Console) {
-  if (diff) {
-    console.error(`\n${diff}\n`)
-  }
-}
-
-function printErrorMessage(error: ErrorWithDiff, logger: Logger) {
+function printErrorMessage(error: ErrorWithDiff, logger: ErrorLogger) {
   const errorName = error.name || error.nameStr || 'Unknown Error'
   if (!error.message) {
     logger.error(error)
@@ -335,8 +369,8 @@ function printErrorMessage(error: ErrorWithDiff, logger: Logger) {
   }
 }
 
-export function printStack(
-  logger: Logger,
+function printStack(
+  logger: ErrorLogger,
   project: TestProject,
   stack: ParsedStack[],
   highlight: ParsedStack | undefined,

diff --git a/packages/vitest/src/node/logger.ts b/packages/vitest/src/node/logger.ts
@@ -3,11 +3,9 @@ import type { ErrorWithDiff } from '@vitest/utils'
 import type { Writable } from 'node:stream'
 import type { TypeCheckError } from '../typecheck/typechecker'
 import type { Vitest } from './core'
-import type { PrintErrorResult } from './error'
 import type { TestProject } from './project'
 import { Console } from 'node:console'
 import { toArray } from '@vitest/utils'
-import { parseErrorStacktrace } from '@vitest/utils/source-map'
 import c from 'tinyrainbow'
 import { highlightCode } from '../utils/colors'
 import { printError } from './error'
@@ -106,33 +104,8 @@ export class Logger {
     this.console.log(`${CURSOR_TO_START}${ERASE_DOWN}${log}`)
   }
 
-  printError(err: unknown, options: ErrorOptions = {}): PrintErrorResult | undefined {
-    const { fullStack = false, type } = options
-    const project = options.project
-      ?? this.ctx.coreWorkspaceProject
-      ?? this.ctx.projects[0]
-    return printError(err, project, {
-      type,
-      showCodeFrame: options.showCodeFrame ?? true,
-      logger: this,
-      printProperties: options.verbose,
-      screenshotPaths: options.screenshotPaths,
-      parseErrorStacktrace: (error) => {
-        // browser stack trace needs to be processed differently,
-        // so there is a separate method for that
-        if (options.task?.file.pool === 'browser' && project.browser) {
-          return project.browser.parseErrorStacktrace(error, {
-            ignoreStackEntries: fullStack ? [] : undefined,
-          })
-        }
-
-        // node.js stack trace already has correct source map locations
-        return parseErrorStacktrace(error, {
-          frameFilter: project.config.onStackTrace,
-          ignoreStackEntries: fullStack ? [] : undefined,
-        })
-      },
-    })
+  printError(err: unknown, options: ErrorOptions = {}) {
+    printError(err, this.ctx, this, options)
   }
 
   clearHighlightCache(filename?: string) {

diff --git a/test/core/test/diff.test.ts b/test/core/test/diff.test.ts
@@ -2,15 +2,16 @@ import type { DiffOptions } from '@vitest/utils/diff'
 import { stripVTControlCharacters } from 'node:util'
 import { processError } from '@vitest/runner'
 import { diff, diffStringsUnified, printDiffOrStringify } from '@vitest/utils/diff'
-import { expect, test, vi } from 'vitest'
-import { displayDiff } from '../../../packages/vitest/src/node/error'
+import { expect, test } from 'vitest'
+
+function wrapDiff(diff?: string) {
+  return diff && stripVTControlCharacters(`\n${diff}\n`)
+}
 
 test('displays string diff', () => {
   const stringA = 'Hello AWorld'
   const stringB = 'Hello BWorld'
-  const console = { log: vi.fn(), error: vi.fn() }
-  displayDiff(printDiffOrStringify(stringA, stringB), console as any)
-  expect(stripVTControlCharacters(console.error.mock.calls[0][0])).toMatchInlineSnapshot(`
+  expect(wrapDiff(printDiffOrStringify(stringA, stringB))).toMatchInlineSnapshot(`
     "
     Expected: "Hello BWorld"
     Received: "Hello AWorld"
@@ -21,9 +22,7 @@ test('displays string diff', () => {
 test('displays object diff', () => {
   const objectA = { a: 1, b: 2 }
   const objectB = { a: 1, b: 3 }
-  const console = { log: vi.fn(), error: vi.fn() }
-  displayDiff(diff(objectA, objectB), console as any)
-  expect(stripVTControlCharacters(console.error.mock.calls[0][0])).toMatchInlineSnapshot(`
+  expect(wrapDiff(diff(objectA, objectB))).toMatchInlineSnapshot(`
     "
     - Expected
     + Received
@@ -40,9 +39,7 @@ test('displays object diff', () => {
 test('display truncated object diff', () => {
   const objectA = { a: 1, b: 2, c: 3, d: 4, e: 5 }
   const objectB = { a: 1, b: 3, c: 4, d: 5, e: 6 }
-  const console = { log: vi.fn(), error: vi.fn() }
-  displayDiff(diff(objectA, objectB, { truncateThreshold: 4 }), console as any)
-  expect(stripVTControlCharacters(console.error.mock.calls[0][0])).toMatchInlineSnapshot(`
+  expect(wrapDiff(diff(objectA, objectB, { truncateThreshold: 4 }))).toMatchInlineSnapshot(`
     "
     - Expected
     + Received
@@ -61,9 +58,7 @@ test('display truncated object diff', () => {
 test('display one line string diff', () => {
   const string1 = 'string1'
   const string2 = 'string2'
-  const console = { log: vi.fn(), error: vi.fn() }
-  displayDiff(diff(string1, string2), console as any)
-  expect(stripVTControlCharacters(console.error.mock.calls[0][0])).toMatchInlineSnapshot(`
+  expect(wrapDiff(diff(string1, string2))).toMatchInlineSnapshot(`
     "
     - Expected
     + Received
@@ -77,9 +72,7 @@ test('display one line string diff', () => {
 test('display one line string diff should not be affected by truncateThreshold', () => {
   const string1 = 'string1'
   const string2 = 'string2'
-  const console = { log: vi.fn(), error: vi.fn() }
-  displayDiff(diff(string1, string2, { truncateThreshold: 3 }), console as any)
-  expect(stripVTControlCharacters(console.error.mock.calls[0][0])).toMatchInlineSnapshot(`
+  expect(wrapDiff(diff(string1, string2, { truncateThreshold: 3 }))).toMatchInlineSnapshot(`
     "
     - Expected
     + Received
@@ -93,9 +86,7 @@ test('display one line string diff should not be affected by truncateThreshold',
 test('display multiline string diff', () => {
   const string1 = 'string1\nstring2\nstring3'
   const string2 = 'string2\nstring2\nstring1'
-  const console = { log: vi.fn(), error: vi.fn() }
-  displayDiff(diff(string1, string2), console as any)
-  expect(stripVTControlCharacters(console.error.mock.calls[0][0])).toMatchInlineSnapshot(`
+  expect(wrapDiff(diff(string1, string2))).toMatchInlineSnapshot(`
     "
     - Expected
     + Received
@@ -112,9 +103,7 @@ test('display multiline string diff', () => {
 test('display truncated multiline string diff', () => {
   const string1 = 'string1\nstring2\nstring3'
   const string2 = 'string2\nstring2\nstring1'
-  const console = { log: vi.fn(), error: vi.fn() }
-  displayDiff(diff(string1, string2, { truncateThreshold: 2 }), console as any)
-  expect(stripVTControlCharacters(console.error.mock.calls[0][0])).toMatchInlineSnapshot(`
+  expect(wrapDiff(diff(string1, string2, { truncateThreshold: 2 }))).toMatchInlineSnapshot(`
     "
     - Expected
     + Received
@@ -130,9 +119,7 @@ test('display truncated multiline string diff', () => {
 test('display truncated multiple items array diff', () => {
   const array1 = Array.from({ length: 45000 }).fill('foo')
   const array2 = Array.from({ length: 45000 }).fill('bar')
-  const console = { log: vi.fn(), error: vi.fn() }
-  displayDiff(diff(array1, array2, { truncateThreshold: 3 }), console as any)
-  expect(stripVTControlCharacters(console.error.mock.calls[0][0])).toMatchInlineSnapshot(`
+  expect(wrapDiff(diff(array1, array2, { truncateThreshold: 3 }))).toMatchInlineSnapshot(`
     "
     - Expected
     + Received

diff --git a/test/reporters/fixtures/many-errors/basic.test.ts b/test/reporters/fixtures/many-errors/basic.test.ts
@@ -0,0 +1,5 @@
+import { test } from "vitest"
+
+test.for([...Array(20)].map((_, j) => j))('%i', (i) => {
+  throw new Error(`error-${i}`)
+})
diff --git a/test/reporters/tests/junit.test.ts b/test/reporters/tests/junit.test.ts
@@ -2,7 +2,7 @@ import type { File, Suite, Task, TaskResult } from 'vitest'
 import { resolve } from 'pathe'
 import { expect, test } from 'vitest'
 import { getDuration } from '../../../packages/vitest/src/node/reporters/junit'
-import { runVitest } from '../../test-utils'
+import { runVitest, runVitestCli } from '../../test-utils'
 
 const root = resolve(__dirname, '../fixtures')
 
@@ -169,3 +169,13 @@ test.each([true, false])('addFileAttribute %s', async (t) => {
   })
   expect(stabilizeReport(stdout)).matchSnapshot()
 })
+
+test('many errors without warning', async () => {
+  const { stderr } = await runVitestCli(
+    'run',
+    '--reporter=junit',
+    '--root',
+    resolve(import.meta.dirname, '../fixtures/many-errors'),
+  )
+  expect(stderr).not.toContain('MaxListenersExceededWarning')
+})