From 3f8f2a0fdd43d42cf420e7aefd7399cf54c9e4f0 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Tue, 21 Feb 2023 19:24:17 -0800 Subject: [PATCH] chore: do not use library stack capturer in test runner (#21075) --- .../src/client/channelOwner.ts | 4 +- .../playwright-core/src/client/connection.ts | 4 +- .../playwright-core/src/utils/stackTrace.ts | 24 +++-------- packages/playwright-core/src/utilsBundle.ts | 27 +++++++++--- .../playwright-test/src/matchers/expect.ts | 13 +++--- .../playwright-test/src/matchers/matchers.ts | 4 +- .../playwright-test/src/matchers/toEqual.ts | 8 +--- .../playwright-test/src/reporters/base.ts | 8 ++-- packages/playwright-test/src/util.ts | 43 +++++++++++++------ .../playwright-test/src/worker/workerMain.ts | 3 +- tests/page/page-mouse.spec.ts | 1 - 11 files changed, 79 insertions(+), 60 deletions(-) diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index aa3dac12c3..6c512c3dcf 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -19,7 +19,7 @@ import type * as channels from '@protocol/channels'; import { maybeFindValidator, ValidationError, type ValidatorContext } from '../protocol/validator'; import { debugLogger } from '../common/debugLogger'; import type { ParsedStackTrace } from '../utils/stackTrace'; -import { captureRawStack, captureStackTrace } from '../utils/stackTrace'; +import { captureRawStack, captureLibraryStackTrace } from '../utils/stackTrace'; import { isUnderTest } from '../utils'; import { zones } from '../utils/zones'; import type { ClientInstrumentation } from './clientInstrumentation'; @@ -161,7 +161,7 @@ export abstract class ChannelOwner { - const { frame, fileName } = parseStackTraceLine(line); - if (!frame || !frame.file || !fileName) + const frame = parseStackTraceLine(line); + if (!frame || !frame.fileName) return null; - if (!process.env.PWDEBUGIMPL && isInternalFileName(frame.file, frame.function)) + if (!process.env.PWDEBUGIMPL && isTesting && frame.fileName.includes(COVERAGE_PATH)) return null; - if (!process.env.PWDEBUGIMPL && isTesting && fileName.includes(COVERAGE_PATH)) - return null; - const isPlaywrightLibrary = fileName.startsWith(CORE_DIR); + const isPlaywrightLibrary = frame.fileName.startsWith(CORE_DIR); const parsed: ParsedFrame = { frame: { - file: fileName, + file: frame.fileName, line: frame.line, column: frame.column, function: frame.function, diff --git a/packages/playwright-core/src/utilsBundle.ts b/packages/playwright-core/src/utilsBundle.ts index 718ca796f7..cfc3301bfd 100644 --- a/packages/playwright-core/src/utilsBundle.ts +++ b/packages/playwright-core/src/utilsBundle.ts @@ -38,20 +38,37 @@ export type { Command } from '../bundles/utils/node_modules/commander'; export type { WebSocket, WebSocketServer, RawData as WebSocketRawData, EventEmitter as WebSocketEventEmitter } from '../bundles/utils/node_modules/@types/ws'; const StackUtils: typeof import('../bundles/utils/node_modules/@types/stack-utils') = require('./utilsBundleImpl').StackUtils; -const stackUtils = new StackUtils(); +const stackUtils = new StackUtils({ internals: StackUtils.nodeInternals() }); +const nodeInternals = StackUtils.nodeInternals(); +const nodeMajorVersion = +process.versions.node.split('.')[0]; -export function parseStackTraceLine(line: string): { frame: import('../bundles/utils/node_modules/@types/stack-utils').StackLineData | null, fileName: string | null } { +export type StackFrameData = { + line?: number | undefined; + column?: number | undefined; + fileName?: string | undefined; + fileOrUrl?: string | undefined; + function?: string | undefined; +}; + +export function parseStackTraceLine(line: string): StackFrameData | null { + if (!process.env.PWDEBUGIMPL && nodeMajorVersion < 16 && nodeInternals.some(internal => internal.test(line))) + return null; const frame = stackUtils.parseLine(line); if (!frame) - return { frame: null, fileName: null }; - let fileName = null; + return null; + if (!process.env.PWDEBUGIMPL && (frame.file?.startsWith('internal') || frame.file?.startsWith('node:'))) + return null; + let fileName: string | undefined = undefined; if (frame.file) { // ESM files return file:// URLs, see here: https://github.com/tapjs/stack-utils/issues/60 fileName = frame.file.startsWith('file://') ? url.fileURLToPath(frame.file) : path.resolve(process.cwd(), frame.file); } return { - frame, + line: frame.line, + column: frame.column, fileName, + fileOrUrl: frame.file, + function: frame.function, }; } diff --git a/packages/playwright-test/src/matchers/expect.ts b/packages/playwright-test/src/matchers/expect.ts index a1175a20e3..bd2ed6456a 100644 --- a/packages/playwright-test/src/matchers/expect.ts +++ b/packages/playwright-test/src/matchers/expect.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { captureStackTrace, pollAgainstTimeout } from 'playwright-core/lib/utils'; +import { pollAgainstTimeout } from 'playwright-core/lib/utils'; import path from 'path'; import { toBeChecked, @@ -44,7 +44,7 @@ import { import { toMatchSnapshot, toHaveScreenshot } from './toMatchSnapshot'; import type { Expect } from '../common/types'; import { currentTestInfo, currentExpectTimeout } from '../common/globals'; -import { serializeError, trimLongString } from '../util'; +import { filteredStackTrace, serializeError, stringifyStackFrames, trimLongString } from '../util'; import { expect as expectLibrary, INVERTED_COLOR, @@ -196,13 +196,12 @@ class ExpectMetaInfoProxyHandler { if (!testInfo) return matcher.call(target, ...args); - const stackTrace = captureStackTrace(); - const stackLines = stackTrace.frameTexts; - const frame = stackTrace.frames[0]; + const stackFrames = filteredStackTrace(new Error()); + const frame = stackFrames[0]; const customMessage = this._info.message || ''; const defaultTitle = `expect${this._info.isPoll ? '.poll' : ''}${this._info.isSoft ? '.soft' : ''}${this._info.isNot ? '.not' : ''}.${matcherName}`; const step = testInfo._addStep({ - location: frame && frame.file ? { file: path.resolve(process.cwd(), frame.file), line: frame.line || 0, column: frame.column || 0 } : undefined, + location: frame && frame.fileName ? { file: path.resolve(process.cwd(), frame.fileName), line: frame.line || 0, column: frame.column || 0 } : undefined, category: 'expect', title: trimLongString(customMessage || defaultTitle, 1024), canHaveChildren: true, @@ -230,7 +229,7 @@ class ExpectMetaInfoProxyHandler { ...messageLines, ].join('\n'); jestError.message = newMessage; - jestError.stack = jestError.name + ': ' + newMessage + '\n' + stackLines.join('\n'); + jestError.stack = jestError.name + ': ' + newMessage + '\n' + stringifyStackFrames(stackFrames).join('\n'); } const serializerError = serializeError(jestError); diff --git a/packages/playwright-test/src/matchers/matchers.ts b/packages/playwright-test/src/matchers/matchers.ts index 6228fd332e..bd62c79b11 100644 --- a/packages/playwright-test/src/matchers/matchers.ts +++ b/packages/playwright-test/src/matchers/matchers.ts @@ -135,7 +135,7 @@ export function toContainText( options: { timeout?: number, useInnerText?: boolean, ignoreCase?: boolean } = {}, ) { if (Array.isArray(expected)) { - return toEqual.call(this, 'toContainText', locator, 'Locator', async (isNot, timeout, customStackTrace) => { + return toEqual.call(this, 'toContainText', locator, 'Locator', async (isNot, timeout) => { const expectedText = toExpectedTextValues(expected, { matchSubstring: true, normalizeWhiteSpace: true, ignoreCase: options.ignoreCase }); return await locator._expect('to.contain.text.array', { expectedText, isNot, useInnerText: options.useInnerText, timeout }); }, expected, { ...options, contains: true }); @@ -264,7 +264,7 @@ export function toHaveValues( expected: (string | RegExp)[], options?: { timeout?: number }, ) { - return toEqual.call(this, 'toHaveValues', locator, 'Locator', async (isNot, timeout, customStackTrace) => { + return toEqual.call(this, 'toHaveValues', locator, 'Locator', async (isNot, timeout) => { const expectedText = toExpectedTextValues(expected); return await locator._expect('to.have.values', { expectedText, isNot, timeout }); }, expected, options); diff --git a/packages/playwright-test/src/matchers/toEqual.ts b/packages/playwright-test/src/matchers/toEqual.ts index 300dc20b58..216c1d6c56 100644 --- a/packages/playwright-test/src/matchers/toEqual.ts +++ b/packages/playwright-test/src/matchers/toEqual.ts @@ -17,8 +17,6 @@ import type { Expect } from '../common/types'; import { expectTypes } from '../util'; import { callLogText } from '../util'; -import type { ParsedStackTrace } from 'playwright-core/lib/utils'; -import { captureStackTrace } from 'playwright-core/lib/utils'; import { matcherHint } from './matcherHint'; import { currentExpectTimeout } from '../common/globals'; @@ -34,7 +32,7 @@ export async function toEqual( matcherName: string, receiver: any, receiverType: string, - query: (isNot: boolean, timeout: number, customStackTrace: ParsedStackTrace) => Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }>, + query: (isNot: boolean, timeout: number) => Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }>, expected: T, options: { timeout?: number, contains?: boolean } = {}, ) { @@ -48,9 +46,7 @@ export async function toEqual( const timeout = currentExpectTimeout(options); - const customStackTrace = captureStackTrace(); - customStackTrace.apiName = 'expect.' + matcherName; - const { matches: pass, received, log, timedOut } = await query(this.isNot, timeout, customStackTrace); + const { matches: pass, received, log, timedOut } = await query(this.isNot, timeout); const message = pass ? () => diff --git a/packages/playwright-test/src/reporters/base.ts b/packages/playwright-test/src/reporters/base.ts index 7f7be29ed4..dcf999be52 100644 --- a/packages/playwright-test/src/reporters/base.ts +++ b/packages/playwright-test/src/reporters/base.ts @@ -441,12 +441,12 @@ export function prepareErrorStack(stack: string): { const stackLines = lines.slice(firstStackLine); let location: Location | undefined; for (const line of stackLines) { - const { frame: parsed, fileName: resolvedFile } = parseStackTraceLine(line); - if (!parsed || !resolvedFile) + const frame = parseStackTraceLine(line); + if (!frame || !frame.fileName) continue; - if (belongsToNodeModules(resolvedFile)) + if (belongsToNodeModules(frame.fileName)) continue; - location = { file: resolvedFile, column: parsed.column || 0, line: parsed.line || 0 }; + location = { file: frame.fileName, column: frame.column || 0, line: frame.line || 0 }; break; } return { message, stackLines, location }; diff --git a/packages/playwright-test/src/util.ts b/packages/playwright-test/src/util.ts index 8294375567..6017140c12 100644 --- a/packages/playwright-test/src/util.ts +++ b/packages/playwright-test/src/util.ts @@ -16,33 +16,52 @@ import fs from 'fs'; import { mime } from 'playwright-core/lib/utilsBundle'; +import type { StackFrameData } from 'playwright-core/lib/utilsBundle'; import util from 'util'; import path from 'path'; import url from 'url'; -import { colors, debug, minimatch } from 'playwright-core/lib/utilsBundle'; +import { colors, debug, minimatch, parseStackTraceLine } from 'playwright-core/lib/utilsBundle'; import type { TestInfoError, Location } from './common/types'; -import { calculateSha1, captureStackTrace, isRegExp, isString } from 'playwright-core/lib/utils'; -import type { ParsedStackTrace } from 'playwright-core/lib/utils'; - -export type { ParsedStackTrace }; +import { calculateSha1, isRegExp, isString } from 'playwright-core/lib/utils'; const PLAYWRIGHT_TEST_PATH = path.join(__dirname, '..'); +const PLAYWRIGHT_CORE_PATH = path.dirname(require.resolve('playwright-core/package.json')); export function filterStackTrace(e: Error) { if (process.env.PWDEBUGIMPL) return; - - const stack = captureStackTrace(e.stack); - const stackLines = stack.frames.filter(f => !f.file.startsWith(PLAYWRIGHT_TEST_PATH)).map(f => { - if (f.function) - return ` at ${f.function} (${f.file}:${f.line}:${f.column})`; - return ` at ${f.file}:${f.line}:${f.column}`; - }); + const stackLines = stringifyStackFrames(filteredStackTrace(e)); const message = e.message; e.stack = `${e.name}: ${e.message}\n${stackLines.join('\n')}`; e.message = message; } +export function filteredStackTrace(e: Error): StackFrameData[] { + const frames: StackFrameData[] = []; + for (const line of e.stack?.split('\n') || []) { + const frame = parseStackTraceLine(line); + if (!frame || !frame.fileName) + continue; + if (!process.env.PWDEBUGIMPL && frame.fileName.startsWith(PLAYWRIGHT_TEST_PATH)) + continue; + if (!process.env.PWDEBUGIMPL && frame.fileName.startsWith(PLAYWRIGHT_CORE_PATH)) + continue; + frames.push(frame); + } + return frames; +} + +export function stringifyStackFrames(frames: StackFrameData[]): string[] { + const stackLines: string[] = []; + for (const frame of frames) { + if (frame.function) + stackLines.push(` at ${frame.function} (${frame.fileName}:${frame.line}:${frame.column})`); + else + stackLines.push(` at ${frame.fileName}:${frame.line}:${frame.column}`); + } + return stackLines; +} + export function serializeError(error: Error | any): TestInfoError { if (error instanceof Error) { filterStackTrace(error); diff --git a/packages/playwright-test/src/worker/workerMain.ts b/packages/playwright-test/src/worker/workerMain.ts index 507a65057a..7d2bff0474 100644 --- a/packages/playwright-test/src/worker/workerMain.ts +++ b/packages/playwright-test/src/worker/workerMain.ts @@ -233,7 +233,8 @@ export class WorkerMain extends ProcessRunner { // In theory, we should run above code without any errors. // However, in the case we screwed up, or loadTestFile failed in the worker // but not in the runner, let's do a fatal error. - this.unhandledError(e); + this._fatalErrors.push(serializeError(e)); + this._stop(); } finally { const donePayload: DonePayload = { fatalErrors: this._fatalErrors, diff --git a/tests/page/page-mouse.spec.ts b/tests/page/page-mouse.spec.ts index a05db54c65..d8624c546e 100644 --- a/tests/page/page-mouse.spec.ts +++ b/tests/page/page-mouse.spec.ts @@ -278,7 +278,6 @@ it('should dispatch mouse move after context menu was opened', async ({ page, br const angle = 2 * Math.PI * i / N; const x = CX + Math.round(radius * Math.cos(angle)); const y = CY + Math.round(radius * Math.sin(angle)); - console.log(x, y); await page.mouse.move(x, y); } }