Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix capturePrintError logger duplicate event handlers #7197

Merged
merged 9 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 55 additions & 21 deletions packages/vitest/src/node/error.ts
Original file line number Diff line number Diff line change
@@ -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'
Expand All @@ -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
}

Expand All @@ -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,
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a minor refactoring of this mostly useless utility

logger.error(`\n${e.diff}\n`)
}

// if the error provide the frame
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
Expand All @@ -274,7 +314,7 @@ function handleImportOutsideModuleError(stack: string, logger: Logger) {
}

function printModuleWarningForPackage(
logger: Logger,
logger: ErrorLogger,
path: string,
name: string,
) {
Expand Down Expand Up @@ -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. `
Expand All @@ -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)
Expand All @@ -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,
Expand Down
31 changes: 2 additions & 29 deletions packages/vitest/src/node/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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) {
Expand Down
39 changes: 13 additions & 26 deletions test/core/test/diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
5 changes: 5 additions & 0 deletions test/reporters/fixtures/many-errors/basic.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { test } from "vitest"

test.for([...Array(20)].map((_, j) => j))('%i', (i) => {
throw new Error(`error-${i}`)
})
12 changes: 11 additions & 1 deletion test/reporters/tests/junit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down Expand Up @@ -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')
})
Loading