From 62aa7208125b22ed2bed371592db859a0996d25c Mon Sep 17 00:00:00 2001 From: Vladimir Date: Sun, 30 Jun 2024 18:26:14 +0200 Subject: [PATCH] fix(browser): print correct stack trace in source files (#6003) --- packages/browser/src/node/server.ts | 43 +++++++++++++++++++ packages/runner/src/collect.ts | 2 +- packages/runner/src/types/runner.ts | 4 ++ packages/runner/src/types/tasks.ts | 1 + packages/runner/src/utils/collect.ts | 2 + packages/utils/src/source-map.ts | 5 +++ packages/vitest/src/api/setup.ts | 16 ++++--- packages/vitest/src/node/error.ts | 29 +++++-------- packages/vitest/src/node/logger.ts | 30 ++++++++++--- packages/vitest/src/node/reporters/base.ts | 17 +++++--- .../src/node/reporters/github-actions.ts | 6 ++- packages/vitest/src/node/reporters/junit.ts | 2 +- packages/vitest/src/node/reporters/tap.ts | 10 ++--- packages/vitest/src/runtime/runners/test.ts | 2 + packages/vitest/src/types/browser.ts | 5 ++- test/browser/specs/runner.test.ts | 5 ++- test/browser/src/error.ts | 9 ++++ test/browser/test/failing.test.ts | 5 +++ .../tests/__snapshots__/html.test.ts.snap | 2 + 19 files changed, 147 insertions(+), 48 deletions(-) create mode 100644 test/browser/src/error.ts diff --git a/packages/browser/src/node/server.ts b/packages/browser/src/node/server.ts index 46f3c2d98fd5..4ecc68b78dae 100644 --- a/packages/browser/src/node/server.ts +++ b/packages/browser/src/node/server.ts @@ -9,8 +9,10 @@ import type { WorkspaceProject, } from 'vitest/node' import { join, resolve } from 'pathe' +import type { ErrorWithDiff } from '@vitest/utils' import { slash } from '@vitest/utils' import type { ResolvedConfig } from 'vitest' +import { type StackTraceParserOptions, parseErrorStacktrace, parseStacktrace } from '@vitest/utils/source-map' import { BrowserServerState } from './state' import { getBrowserProvider } from './utils' import { BrowserServerCDPHandler } from './cdp' @@ -33,10 +35,31 @@ export class BrowserServer implements IBrowserServer { public vite!: Vite.ViteDevServer + private stackTraceOptions: StackTraceParserOptions + constructor( public project: WorkspaceProject, public base: string, ) { + this.stackTraceOptions = { + frameFilter: project.config.onStackTrace, + getSourceMap: (id) => { + const result = this.vite.moduleGraph.getModuleById(id)?.transformResult + return result?.map + }, + getFileName: (id) => { + const mod = this.vite.moduleGraph.getModuleById(id) + if (mod?.file) { + return mod.file + } + const modUrl = this.vite.moduleGraph.urlToModuleMap.get(id) + if (modUrl?.file) { + return modUrl.file + } + return id + }, + } + this.state = new BrowserServerState() const pkgRoot = resolve(fileURLToPath(import.meta.url), '../..') @@ -139,6 +162,26 @@ export class BrowserServer implements IBrowserServer { }) } + public parseErrorStacktrace( + e: ErrorWithDiff, + options: StackTraceParserOptions = {}, + ) { + return parseErrorStacktrace(e, { + ...this.stackTraceOptions, + ...options, + }) + } + + public parseStacktrace( + trace: string, + options: StackTraceParserOptions = {}, + ) { + return parseStacktrace(trace, { + ...this.stackTraceOptions, + ...options, + }) + } + private cdpSessions = new Map>() async ensureCDPHandler(contextId: string, sessionId: string) { diff --git a/packages/runner/src/collect.ts b/packages/runner/src/collect.ts index c114e197990b..63837a6ba1d7 100644 --- a/packages/runner/src/collect.ts +++ b/packages/runner/src/collect.ts @@ -27,7 +27,7 @@ export async function collectTests( const config = runner.config for (const filepath of paths) { - const file = createFileTask(filepath, config.root, config.name) + const file = createFileTask(filepath, config.root, config.name, runner.pool) runner.onCollectStart?.(file) diff --git a/packages/runner/src/types/runner.ts b/packages/runner/src/types/runner.ts index ae8795c2fe24..3c237fae1e9e 100644 --- a/packages/runner/src/types/runner.ts +++ b/packages/runner/src/types/runner.ts @@ -148,4 +148,8 @@ export interface VitestRunner { * Publicly available configuration. */ config: VitestRunnerConfig + /** + * The name of the current pool. Can affect how stack trace is inferred on the server side. + */ + pool?: string } diff --git a/packages/runner/src/types/tasks.ts b/packages/runner/src/types/tasks.ts index c1b9c19be7e3..bdd09f32b4fc 100644 --- a/packages/runner/src/types/tasks.ts +++ b/packages/runner/src/types/tasks.ts @@ -63,6 +63,7 @@ export interface Suite extends TaskBase { } export interface File extends Suite { + pool?: string filepath: string projectName: string | undefined collectDuration?: number diff --git a/packages/runner/src/utils/collect.ts b/packages/runner/src/utils/collect.ts index a2d5e63254bf..7173a048ae80 100644 --- a/packages/runner/src/utils/collect.ts +++ b/packages/runner/src/utils/collect.ts @@ -118,6 +118,7 @@ export function createFileTask( filepath: string, root: string, projectName: string, + pool?: string, ) { const path = relative(root, filepath) const file: File = { @@ -130,6 +131,7 @@ export function createFileTask( meta: Object.create(null), projectName, file: undefined!, + pool, } file.file = file return file diff --git a/packages/utils/src/source-map.ts b/packages/utils/src/source-map.ts index 5e26cbcea91e..99ebab691d13 100644 --- a/packages/utils/src/source-map.ts +++ b/packages/utils/src/source-map.ts @@ -14,6 +14,7 @@ export type { SourceMapInput } from '@jridgewell/trace-mapping' export interface StackTraceParserOptions { ignoreStackEntries?: (RegExp | string)[] getSourceMap?: (file: string) => unknown + getFileName?: (id: string) => string frameFilter?: (error: Error, frame: ParsedStack) => boolean | void } @@ -192,6 +193,10 @@ export function parseStacktrace( ) } return stacks.map((stack) => { + if (options.getFileName) { + stack.file = options.getFileName(stack.file) + } + const map = options.getSourceMap?.(stack.file) as | SourceMapInput | null diff --git a/packages/vitest/src/api/setup.ts b/packages/vitest/src/api/setup.ts index cf9e77851c9f..bb04c48c60de 100644 --- a/packages/vitest/src/api/setup.ts +++ b/packages/vitest/src/api/setup.ts @@ -4,7 +4,6 @@ import { createBirpc } from 'birpc' import { parse, stringify } from 'flatted' import type { WebSocket } from 'ws' import { WebSocketServer } from 'ws' -import type { StackTraceParserOptions } from '@vitest/utils/source-map' import type { ViteDevServer } from 'vite' import { API_PATH } from '../constants' import type { Vitest } from '../node' @@ -182,15 +181,18 @@ export class WebSocketReporter implements Reporter { packs.forEach(([taskId, result]) => { const project = this.ctx.getProjectByTaskId(taskId) - - const parserOptions: StackTraceParserOptions = { - getSourceMap: file => project.getBrowserSourceMapModuleById(file), - } + const task = this.ctx.state.idMap.get(taskId) + const isBrowser = task && task.file.pool === 'browser' result?.errors?.forEach((error) => { - if (!isPrimitive(error)) { - error.stacks = parseErrorStacktrace(error, parserOptions) + if (isPrimitive(error)) { + return } + + const stacks = isBrowser + ? project.browser?.parseErrorStacktrace(error) + : parseErrorStacktrace(error) + error.stacks = stacks }) }) diff --git a/packages/vitest/src/node/error.ts b/packages/vitest/src/node/error.ts index 75a8775e3b55..70f9db6f2478 100644 --- a/packages/vitest/src/node/error.ts +++ b/packages/vitest/src/node/error.ts @@ -4,13 +4,11 @@ import { Writable } from 'node:stream' import { normalize, relative } from 'pathe' import c from 'picocolors' import cliTruncate from 'cli-truncate' -import type { StackTraceParserOptions } from '@vitest/utils/source-map' import { inspect } from '@vitest/utils' import stripAnsi from 'strip-ansi' import type { ErrorWithDiff, ParsedStack } from '../types' import { lineSplitRE, - parseErrorStacktrace, positionToOffset, } from '../utils/source-map' import { F_POINTER } from '../utils/figures' @@ -18,19 +16,20 @@ import { TypeCheckError } from '../typecheck/typechecker' import { isPrimitive } from '../utils' import type { Vitest } from './core' import { divider } from './reporters/renderers/utils' +import type { ErrorOptions } from './logger' import { Logger } from './logger' import type { WorkspaceProject } from './workspace' interface PrintErrorOptions { type?: string logger: Logger - fullStack?: boolean showCodeFrame?: boolean printProperties?: boolean screenshotPaths?: string[] + parseErrorStacktrace: (error: ErrorWithDiff) => ParsedStack[] } -interface PrintErrorResult { +export interface PrintErrorResult { nearest?: ParsedStack } @@ -38,7 +37,7 @@ interface PrintErrorResult { export function capturePrintError( error: unknown, ctx: Vitest, - project: WorkspaceProject, + options: ErrorOptions, ) { let output = '' const writable = new Writable({ @@ -47,9 +46,10 @@ export function capturePrintError( callback() }, }) - const result = printError(error, project, { + const logger = new Logger(ctx, writable, writable) + const result = logger.printError(error, { showCodeFrame: false, - logger: new Logger(ctx, writable, writable), + ...options, }) return { nearest: result?.nearest, output } } @@ -59,7 +59,7 @@ export function printError( project: WorkspaceProject | undefined, options: PrintErrorOptions, ): PrintErrorResult | undefined { - const { showCodeFrame = true, fullStack = false, type, printProperties = true } = options + const { showCodeFrame = true, type, printProperties = true } = options const logger = options.logger let e = error as ErrorWithDiff @@ -84,16 +84,7 @@ export function printError( return } - const parserOptions: StackTraceParserOptions = { - // only browser stack traces require remapping - getSourceMap: file => project.getBrowserSourceMapModuleById(file), - frameFilter: project.config.onStackTrace, - } - - if (fullStack) { - parserOptions.ignoreStackEntries = [] - } - const stacks = parseErrorStacktrace(e, parserOptions) + const stacks = options.parseErrorStacktrace(e) const nearest = error instanceof TypeCheckError @@ -195,9 +186,9 @@ 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, { - fullStack, showCodeFrame: false, logger: options.logger, + parseErrorStacktrace: options.parseErrorStacktrace, }) } diff --git a/packages/vitest/src/node/logger.ts b/packages/vitest/src/node/logger.ts index 05329e2615ed..c2a5c6d7d6e3 100644 --- a/packages/vitest/src/node/logger.ts +++ b/packages/vitest/src/node/logger.ts @@ -2,22 +2,26 @@ import { Console } from 'node:console' import type { Writable } from 'node:stream' import { createLogUpdate } from 'log-update' import c from 'picocolors' -import type { ErrorWithDiff } from '../types' +import { parseErrorStacktrace } from '@vitest/utils/source-map' +import type { ErrorWithDiff, Task } from '../types' import type { TypeCheckError } from '../typecheck/typechecker' import { toArray } from '../utils' import { highlightCode } from '../utils/colors' import { divider } from './reporters/renderers/utils' import { RandomSequencer } from './sequencers/RandomSequencer' import type { Vitest } from './core' +import type { PrintErrorResult } from './error' import { printError } from './error' import type { WorkspaceProject } from './workspace' -interface ErrorOptions { +export interface ErrorOptions { type?: string fullStack?: boolean project?: WorkspaceProject verbose?: boolean screenshotPaths?: string[] + task?: Task + showCodeFrame?: boolean } const ESC = '\x1B[' @@ -89,18 +93,32 @@ export class Logger { this.console.log(`${CURSOR_TO_START}${ERASE_DOWN}${log}`) } - printError(err: unknown, options: ErrorOptions = {}) { + printError(err: unknown, options: ErrorOptions = {}): PrintErrorResult | undefined { const { fullStack = false, type } = options const project = options.project ?? this.ctx.getCoreWorkspaceProject() ?? this.ctx.projects[0] - printError(err, project, { - fullStack, + return printError(err, project, { type, - showCodeFrame: true, + 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, + }) + }, }) } diff --git a/packages/vitest/src/node/reporters/base.ts b/packages/vitest/src/node/reporters/base.ts index 8096983958ae..c2d4eb6241d5 100644 --- a/packages/vitest/src/node/reporters/base.ts +++ b/packages/vitest/src/node/reporters/base.ts @@ -309,13 +309,15 @@ export abstract class BaseReporter implements Reporter { if (log.browser) { write('\n') } + const project = log.taskId ? this.ctx.getProjectByTaskId(log.taskId) : this.ctx.getCoreWorkspaceProject() - const stack = parseStacktrace(log.origin, { - getSourceMap: file => project.getBrowserSourceMapModuleById(file), - frameFilter: project.config.onStackTrace, - }) + + const stack = log.browser + ? (project.browser?.parseStacktrace(log.origin) || []) + : parseStacktrace(log.origin) + const highlight = task ? stack.find(i => i.file === task.file.filepath) : null @@ -605,7 +607,12 @@ export abstract class BaseReporter implements Reporter { } const screenshots = tasks.filter(t => t.meta?.failScreenshotPath).map(t => t.meta?.failScreenshotPath as string) const project = this.ctx.getProjectByTaskId(tasks[0].id) - this.ctx.logger.printError(error, { project, verbose: this.verbose, screenshotPaths: screenshots }) + this.ctx.logger.printError(error, { + project, + verbose: this.verbose, + screenshotPaths: screenshots, + task: tasks[0], + }) errorDivider() } } diff --git a/packages/vitest/src/node/reporters/github-actions.ts b/packages/vitest/src/node/reporters/github-actions.ts index 2c0fc55d4d7c..3fbe2a188ba3 100644 --- a/packages/vitest/src/node/reporters/github-actions.ts +++ b/packages/vitest/src/node/reporters/github-actions.ts @@ -18,6 +18,7 @@ export class GithubActionsReporter implements Reporter { project: WorkspaceProject title: string error: unknown + file?: File }>() for (const error of errors) { projectErrors.push({ @@ -40,14 +41,15 @@ export class GithubActionsReporter implements Reporter { project, title, error, + file, }) } } } // format errors via `printError` - for (const { project, title, error } of projectErrors) { - const result = capturePrintError(error, this.ctx, project) + for (const { project, title, error, file } of projectErrors) { + const result = capturePrintError(error, this.ctx, { project, task: file }) const stack = result?.nearest if (!stack) { continue diff --git a/packages/vitest/src/node/reporters/junit.ts b/packages/vitest/src/node/reporters/junit.ts index d39849d159ee..5b5c4a2dcf1e 100644 --- a/packages/vitest/src/node/reporters/junit.ts +++ b/packages/vitest/src/node/reporters/junit.ts @@ -230,7 +230,7 @@ export class JUnitReporter implements Reporter { const result = capturePrintError( error, this.ctx, - this.ctx.getProjectByTaskId(task.id), + { project: this.ctx.getProjectByTaskId(task.id), task }, ) await this.baseLog( escapeXML(stripAnsi(result.output.trim())), diff --git a/packages/vitest/src/node/reporters/tap.ts b/packages/vitest/src/node/reporters/tap.ts index 27b7debce628..e9ab48e4fa3e 100644 --- a/packages/vitest/src/node/reporters/tap.ts +++ b/packages/vitest/src/node/reporters/tap.ts @@ -86,11 +86,11 @@ export class TapReporter implements Reporter { this.logger.indent() task.result.errors.forEach((error) => { - const stacks = parseErrorStacktrace(error, { - getSourceMap: file => - project.getBrowserSourceMapModuleById(file), - frameFilter: this.ctx.config.onStackTrace, - }) + const stacks = task.file.pool === 'browser' + ? (project.browser?.parseErrorStacktrace(error) || []) + : parseErrorStacktrace(error, { + frameFilter: this.ctx.config.onStackTrace, + }) const stack = stacks[0] this.logger.log('---') diff --git a/packages/vitest/src/runtime/runners/test.ts b/packages/vitest/src/runtime/runners/test.ts index ee99ba884df5..c286ad1f4fbe 100644 --- a/packages/vitest/src/runtime/runners/test.ts +++ b/packages/vitest/src/runtime/runners/test.ts @@ -28,6 +28,8 @@ export class VitestTestRunner implements VitestRunner { private assertionsErrors = new WeakMap, Error>() + public pool = this.workerState.ctx.pool + constructor(public config: ResolvedConfig) {} importFile(filepath: string, source: VitestRunnerImportSource): unknown { diff --git a/packages/vitest/src/types/browser.ts b/packages/vitest/src/types/browser.ts index 454ce12fa1ff..1b747077454a 100644 --- a/packages/vitest/src/types/browser.ts +++ b/packages/vitest/src/types/browser.ts @@ -1,6 +1,7 @@ -import type { Awaitable } from '@vitest/utils' +import type { Awaitable, ErrorWithDiff, ParsedStack } from '@vitest/utils' import type { ViteDevServer } from 'vite' import type { CancelReason } from '@vitest/runner' +import type { StackTraceParserOptions } from '@vitest/utils/source-map' import type { WorkspaceProject } from '../node/workspace' import type { ApiConfig } from './config' @@ -190,6 +191,8 @@ export interface BrowserServer { provider: BrowserProvider close: () => Promise initBrowserProvider: () => Promise + parseStacktrace: (stack: string) => ParsedStack[] + parseErrorStacktrace: (error: ErrorWithDiff, options?: StackTraceParserOptions) => ParsedStack[] } export interface BrowserCommand { diff --git a/test/browser/specs/runner.test.ts b/test/browser/specs/runner.test.ts index eb6c62686a1d..5065cc3498bd 100644 --- a/test/browser/specs/runner.test.ts +++ b/test/browser/specs/runner.test.ts @@ -105,7 +105,10 @@ error with a stack test(`stack trace points to correct file in every browser`, () => { // dependeing on the browser it references either `.toBe()` or `expect()` - expect(stderr).toMatch(/test\/failing.test.ts:4:(12|17)/) + expect(stderr).toMatch(/test\/failing.test.ts:5:(12|17)/) + + // column is 18 in safari, 8 in others + expect(stderr).toMatch(/throwError src\/error.ts:8:(18|8)/) }) test('popup apis should log a warning', () => { diff --git a/test/browser/src/error.ts b/test/browser/src/error.ts new file mode 100644 index 000000000000..457df59afbfc --- /dev/null +++ b/test/browser/src/error.ts @@ -0,0 +1,9 @@ +interface _SomeType { + _unused: string +} + +// this should affect the line number + +export function throwError(_opts?: _SomeType) { + throw new Error('error') +} diff --git a/test/browser/test/failing.test.ts b/test/browser/test/failing.test.ts index de541b1bf3ac..14cb207fe897 100644 --- a/test/browser/test/failing.test.ts +++ b/test/browser/test/failing.test.ts @@ -1,5 +1,10 @@ import { expect, it } from 'vitest' +import { throwError } from '../src/error' it('correctly fails and prints a diff', () => { expect(1).toBe(2) }) + +it('correctly print error in another file', () => { + throwError() +}) diff --git a/test/reporters/tests/__snapshots__/html.test.ts.snap b/test/reporters/tests/__snapshots__/html.test.ts.snap index 6774077b8d97..06c502e054b9 100644 --- a/test/reporters/tests/__snapshots__/html.test.ts.snap +++ b/test/reporters/tests/__snapshots__/html.test.ts.snap @@ -13,6 +13,7 @@ exports[`html reporter > resolves to "failing" status for test file "json-fail" "meta": {}, "mode": "run", "name": "json-fail.test.ts", + "pool": "forks", "prepareDuration": 0, "result": { "duration": 0, @@ -126,6 +127,7 @@ exports[`html reporter > resolves to "passing" status for test file "all-passing "meta": {}, "mode": "run", "name": "all-passing-or-skipped.test.ts", + "pool": "forks", "prepareDuration": 0, "result": { "duration": 0,