From 11a4b3f7f55b0681a69140c1f255c34746adaa89 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 9 Oct 2023 17:04:16 -0700 Subject: [PATCH] chore: remove parsed stack trace (#27496) --- .../src/client/channelOwner.ts | 27 ++++++++++-------- .../src/client/clientInstrumentation.ts | 6 ++-- .../playwright-core/src/client/connection.ts | 21 ++++++++------ .../playwright-core/src/utils/stackTrace.ts | 28 +++++++++++-------- packages/playwright/src/index.ts | 24 ++-------------- .../playwright/src/matchers/matcherHint.ts | 2 +- packages/playwright/src/util.ts | 13 +-------- packages/playwright/src/worker/testInfo.ts | 4 +-- tests/playwright-test/golden.spec.ts | 2 +- tests/playwright-test/playwright.spec.ts | 9 ++++-- .../to-have-screenshot.spec.ts | 2 +- 11 files changed, 63 insertions(+), 75 deletions(-) diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index eac9d27400..b81d757156 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -18,8 +18,8 @@ import { EventEmitter } from 'events'; import type * as channels from '@protocol/channels'; import { maybeFindValidator, ValidationError, type ValidatorContext } from '../protocol/validator'; import { debugLogger } from '../common/debugLogger'; -import type { ExpectZone, ParsedStackTrace } from '../utils/stackTrace'; -import { captureRawStack, captureLibraryStackTrace } from '../utils/stackTrace'; +import type { ExpectZone } from '../utils/stackTrace'; +import { captureRawStack, captureLibraryStackTrace, stringifyStackFrames } from '../utils/stackTrace'; import { isUnderTest } from '../utils'; import { zones } from '../utils/zones'; import type { ClientInstrumentation } from './clientInstrumentation'; @@ -143,11 +143,11 @@ export abstract class ChannelOwner { return this._wrapApiCall(apiZone => { - const { stackTrace, csi, callCookie, wallTime } = apiZone.reported ? { csi: undefined, callCookie: undefined, stackTrace: null, wallTime: undefined } : apiZone; + const { apiName, frames, csi, callCookie, wallTime } = apiZone.reported ? { apiName: undefined, csi: undefined, callCookie: undefined, frames: [], wallTime: undefined } : apiZone; apiZone.reported = true; - if (csi && stackTrace && stackTrace.apiName) - csi.onApiCallBegin(stackTrace.apiName, params, stackTrace, wallTime, callCookie); - return this._connection.sendMessageToServer(this, prop, validator(params, '', { tChannelImpl: tChannelImplToWire, binary: this._connection.isRemote() ? 'toBase64' : 'buffer' }), stackTrace, wallTime); + if (csi && apiName) + csi.onApiCallBegin(apiName, params, frames, wallTime, callCookie); + return this._connection.sendMessageToServer(this, prop, validator(params, '', { tChannelImpl: tChannelImplToWire, binary: this._connection.isRemote() ? 'toBase64' : 'buffer' }), apiName, frames, wallTime); }); }; } @@ -167,23 +167,25 @@ export abstract class ChannelOwner('expectZone', stack); const wallTime = expectZone ? expectZone.wallTime : Date.now(); if (!isInternal && expectZone) - stackTrace.apiName = expectZone.title; + apiName = expectZone.title; const csi = isInternal ? undefined : this._instrumentation; const callCookie: any = {}; - const { apiName, frameTexts } = stackTrace; try { logApiCall(logger, `=> ${apiName} started`, isInternal); - const apiZone = { stackTrace, isInternal, reported: false, csi, callCookie, wallTime }; + const apiZone: ApiZone = { apiName, frames, isInternal, reported: false, csi, callCookie, wallTime }; const result = await zones.run>('apiZone', apiZone, async () => { return await func(apiZone); }); @@ -194,7 +196,7 @@ export abstract class ChannelOwner\n' + e.stack : ''; if (apiName && !apiName.includes('')) e.message = apiName + ': ' + e.message; - const stackFrames = '\n' + frameTexts.join('\n') + innerError; + const stackFrames = '\n' + stringifyStackFrames(stackTrace.frames).join('\n') + innerError; if (stackFrames.trim()) e.stack = e.message + stackFrames; else @@ -236,7 +238,8 @@ function tChannelImplToWire(names: '*' | string[], arg: any, path: string, conte } type ApiZone = { - stackTrace: ParsedStackTrace; + apiName: string | undefined; + frames: channels.StackFrame[]; isInternal: boolean; reported: boolean; csi: ClientInstrumentation | undefined; diff --git a/packages/playwright-core/src/client/clientInstrumentation.ts b/packages/playwright-core/src/client/clientInstrumentation.ts index db563583d9..2b5dd00970 100644 --- a/packages/playwright-core/src/client/clientInstrumentation.ts +++ b/packages/playwright-core/src/client/clientInstrumentation.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import type { ParsedStackTrace } from '../utils/stackTrace'; +import type { StackFrame } from '@protocol/channels'; import type { BrowserContext } from './browserContext'; import type { APIRequestContext } from './fetch'; @@ -22,7 +22,7 @@ export interface ClientInstrumentation { addListener(listener: ClientInstrumentationListener): void; removeListener(listener: ClientInstrumentationListener): void; removeAllListeners(): void; - onApiCallBegin(apiCall: string, params: Record, stackTrace: ParsedStackTrace | null, wallTime: number, userData: any): void; + onApiCallBegin(apiCall: string, params: Record, frames: StackFrame[], wallTime: number, userData: any): void; onApiCallEnd(userData: any, error?: Error): void; onDidCreateBrowserContext(context: BrowserContext): Promise; onDidCreateRequestContext(context: APIRequestContext): Promise; @@ -32,7 +32,7 @@ export interface ClientInstrumentation { } export interface ClientInstrumentationListener { - onApiCallBegin?(apiCall: string, params: Record, stackTrace: ParsedStackTrace | null, wallTime: number, userData: any): void; + onApiCallBegin?(apiName: string, params: Record, frames: StackFrame[], wallTime: number, userData: any): void; onApiCallEnd?(userData: any, error?: Error): void; onDidCreateBrowserContext?(context: BrowserContext): Promise; onDidCreateRequestContext?(context: APIRequestContext): Promise; diff --git a/packages/playwright-core/src/client/connection.ts b/packages/playwright-core/src/client/connection.ts index add0a0b42f..37f41856ac 100644 --- a/packages/playwright-core/src/client/connection.ts +++ b/packages/playwright-core/src/client/connection.ts @@ -35,7 +35,7 @@ import { WritableStream } from './writableStream'; import { debugLogger } from '../common/debugLogger'; import { SelectorsOwner } from './selectors'; import { Android, AndroidSocket, AndroidDevice } from './android'; -import { captureLibraryStackTrace, type ParsedStackTrace } from '../utils/stackTrace'; +import { captureLibraryStackText, stringifyStackFrames } from '../utils/stackTrace'; import { Artifact } from './artifact'; import { EventEmitter } from 'events'; import { JsonPipe } from './jsonPipe'; @@ -65,7 +65,7 @@ export class Connection extends EventEmitter { readonly _objects = new Map(); onmessage = (message: object): void => {}; private _lastId = 0; - private _callbacks = new Map void, reject: (a: Error) => void, stackTrace: ParsedStackTrace | null, type: string, method: string }>(); + private _callbacks = new Map void, reject: (a: Error) => void, apiName: string | undefined, frames: channels.StackFrame[], type: string, method: string }>(); private _rootObject: Root; private _closedErrorMessage: string | undefined; private _isRemote = false; @@ -98,8 +98,14 @@ export class Connection extends EventEmitter { return await this._rootObject.initialize(); } - pendingProtocolCalls(): ParsedStackTrace[] { - return Array.from(this._callbacks.values()).map(callback => callback.stackTrace).filter(Boolean) as ParsedStackTrace[]; + pendingProtocolCalls(): String { + const lines: string[] = []; + for (const call of this._callbacks.values()) { + if (!call.apiName) + continue; + lines.push(` - ${call.apiName}\n${stringifyStackFrames(call.frames)}\n`); + } + return lines.length ? 'Pending operations:\n' + lines.join('\n') : ''; } getObjectWithKnownName(guid: string): any { @@ -113,13 +119,12 @@ export class Connection extends EventEmitter { this._tracingCount--; } - async sendMessageToServer(object: ChannelOwner, method: string, params: any, stackTrace: ParsedStackTrace | null, wallTime: number | undefined): Promise { + async sendMessageToServer(object: ChannelOwner, method: string, params: any, apiName: string | undefined, frames: channels.StackFrame[], wallTime: number | undefined): Promise { if (this._closedErrorMessage) throw new Error(this._closedErrorMessage); if (object._wasCollected) throw new Error('The object has been collected to prevent unbounded heap growth.'); - const { apiName, frames } = stackTrace || { apiName: '', frames: [] }; const guid = object._guid; const type = object._type; const id = ++this._lastId; @@ -133,7 +138,7 @@ export class Connection extends EventEmitter { if (this._tracingCount && frames && type !== 'LocalUtils') this._localUtils?._channel.addStackToTracingNoReply({ callData: { stack: frames, id } }).catch(() => {}); this.onmessage({ ...message, metadata }); - return await new Promise((resolve, reject) => this._callbacks.set(id, { resolve, reject, stackTrace, type, method })); + return await new Promise((resolve, reject) => this._callbacks.set(id, { resolve, reject, apiName, frames, type, method })); } dispatch(message: object) { @@ -186,7 +191,7 @@ export class Connection extends EventEmitter { } close(errorMessage: string = 'Connection closed') { - const stack = captureLibraryStackTrace().frameTexts.join('\n'); + const stack = captureLibraryStackText(); if (stack) errorMessage += '\n ==== Closed by ====\n' + stack + '\n'; this._closedErrorMessage = errorMessage; diff --git a/packages/playwright-core/src/utils/stackTrace.ts b/packages/playwright-core/src/utils/stackTrace.ts index a8b9cfae28..81cbf8e98d 100644 --- a/packages/playwright-core/src/utils/stackTrace.ts +++ b/packages/playwright-core/src/utils/stackTrace.ts @@ -36,13 +36,6 @@ const internalStackPrefixes = [ ]; export const addInternalStackPrefix = (prefix: string) => internalStackPrefixes.push(prefix); -export type ParsedStackTrace = { - allFrames: StackFrame[]; - frames: StackFrame[]; - frameTexts: string[]; - apiName: string | undefined; -}; - export type RawStack = string[]; export function captureRawStack(): RawStack { @@ -54,7 +47,7 @@ export function captureRawStack(): RawStack { return stack.split('\n'); } -export function captureLibraryStackTrace(rawStack?: RawStack): ParsedStackTrace { +export function captureLibraryStackTrace(rawStack?: RawStack): { frames: StackFrame[], apiName: string } { const stack = rawStack || captureRawStack(); const isTesting = isUnderTest(); @@ -79,7 +72,6 @@ export function captureLibraryStackTrace(rawStack?: RawStack): ParsedStackTrace }).filter(Boolean) as ParsedFrame[]; let apiName = ''; - const allFrames = parsedFrames; // Deepest transition between non-client code calling into client // code is the api entry. @@ -110,13 +102,27 @@ export function captureLibraryStackTrace(rawStack?: RawStack): ParsedStackTrace }); return { - allFrames: allFrames.map(p => p.frame), frames: parsedFrames.map(p => p.frame), - frameTexts: parsedFrames.map(p => p.frameText), apiName }; } +export function stringifyStackFrames(frames: StackFrame[]): string[] { + const stackLines: string[] = []; + for (const frame of frames) { + if (frame.function) + stackLines.push(` at ${frame.function} (${frame.file}:${frame.line}:${frame.column})`); + else + stackLines.push(` at ${frame.file}:${frame.line}:${frame.column}`); + } + return stackLines; +} + +export function captureLibraryStackText() { + const parsed = captureLibraryStackTrace(); + return stringifyStackFrames(parsed.frames).join('\n'); +} + export function splitErrorMessage(message: string): { name: string, message: string } { const separationIdx = message.indexOf(':'); return { diff --git a/packages/playwright/src/index.ts b/packages/playwright/src/index.ts index 2a02f52315..4046594ceb 100644 --- a/packages/playwright/src/index.ts +++ b/packages/playwright/src/index.ts @@ -24,7 +24,6 @@ import type { TestInfoImpl } from './worker/testInfo'; import { rootTestType } from './common/testType'; import type { ContextReuseMode } from './common/config'; import type { ClientInstrumentation, ClientInstrumentationListener } from '../../playwright-core/src/client/clientInstrumentation'; -import type { ParsedStackTrace } from '../../playwright-core/src/utils/stackTrace'; import { currentTestInfo } from './common/globals'; import { mergeTraceFiles } from './worker/testTracing'; export { expect } from './matchers/expect'; @@ -253,12 +252,12 @@ const playwrightFixtures: Fixtures = ({ const artifactsRecorder = new ArtifactsRecorder(playwright, _artifactsDir, trace, screenshot); await artifactsRecorder.willStartTest(testInfo as TestInfoImpl); const csiListener: ClientInstrumentationListener = { - onApiCallBegin: (apiName: string, params: Record, stackTrace: ParsedStackTrace | null, wallTime: number, userData: any) => { + onApiCallBegin: (apiName: string, params: Record, frames: StackFrame[], wallTime: number, userData: any) => { const testInfo = currentTestInfo(); if (!testInfo || apiName.startsWith('expect.') || apiName.includes('setTestIdAttribute')) return { userObject: null }; const step = testInfo._addStep({ - location: stackTrace?.frames[0] as any, + location: frames[0] as any, category: 'pw:api', title: renderApiCall(apiName, params), apiName, @@ -331,8 +330,7 @@ const playwrightFixtures: Fixtures = ({ return context; }); - const prependToError = testInfoImpl._didTimeout ? - formatPendingCalls((browser as any)._connection.pendingProtocolCalls()) : ''; + const prependToError = testInfoImpl._didTimeout ? (browser as any)._connection.pendingProtocolCalls() : ''; let counter = 0; await Promise.all([...contexts.keys()].map(async context => { @@ -403,22 +401,6 @@ const playwrightFixtures: Fixtures = ({ }, }); - -function formatPendingCalls(calls: ParsedStackTrace[]) { - calls = calls.filter(call => !!call.apiName); - if (!calls.length) - return ''; - return 'Pending operations:\n' + calls.map(call => { - const frame = call.frames && call.frames[0] ? ' at ' + formatStackFrame(call.frames[0]) : ''; - return ` - ${call.apiName}${frame}\n`; - }).join(''); -} - -function formatStackFrame(frame: StackFrame) { - const file = path.relative(process.cwd(), frame.file) || path.basename(frame.file); - return `${file}:${frame.line || 1}:${frame.column || 1}`; -} - function hookType(testInfo: TestInfoImpl): 'beforeAll' | 'afterAll' | undefined { const type = testInfo._timeoutManager.currentRunnableType(); if (type === 'beforeAll' || type === 'afterAll') diff --git a/packages/playwright/src/matchers/matcherHint.ts b/packages/playwright/src/matchers/matcherHint.ts index 298567bfe0..8a34553744 100644 --- a/packages/playwright/src/matchers/matcherHint.ts +++ b/packages/playwright/src/matchers/matcherHint.ts @@ -17,8 +17,8 @@ import { colors } from 'playwright-core/lib/utilsBundle'; import type { ExpectMatcherContext } from './expect'; import type { Locator } from 'playwright-core'; -import { stringifyStackFrames } from '../util'; import type { StackFrame } from '@protocol/channels'; +import { stringifyStackFrames } from 'playwright-core/lib/utils'; export function matcherHint(state: ExpectMatcherContext, locator: Locator | undefined, matcherName: string, expression: any, actual: any, matcherOptions: any, timeout?: number) { let header = state.utils.matcherHint(matcherName, expression, actual, matcherOptions).replace(/ \/\/ deep equality/, '') + '\n\n'; diff --git a/packages/playwright/src/util.ts b/packages/playwright/src/util.ts index 0a74094887..756d2e3b18 100644 --- a/packages/playwright/src/util.ts +++ b/packages/playwright/src/util.ts @@ -23,7 +23,7 @@ import url from 'url'; import { colors, debug, minimatch, parseStackTraceLine } from 'playwright-core/lib/utilsBundle'; import type { TestInfoError } from './../types/test'; import type { Location } from './../types/testReporter'; -import { calculateSha1, isRegExp, isString, sanitizeForFilePath } from 'playwright-core/lib/utils'; +import { calculateSha1, isRegExp, isString, sanitizeForFilePath, stringifyStackFrames } from 'playwright-core/lib/utils'; import type { RawStack } from 'playwright-core/lib/utils'; const PLAYWRIGHT_TEST_PATH = path.join(__dirname, '..'); @@ -61,17 +61,6 @@ export function filteredStackTrace(rawStack: RawStack): StackFrame[] { return frames; } -export function stringifyStackFrames(frames: StackFrame[]): string[] { - const stackLines: string[] = []; - for (const frame of frames) { - if (frame.function) - stackLines.push(` at ${frame.function} (${frame.file}:${frame.line}:${frame.column})`); - else - stackLines.push(` at ${frame.file}:${frame.line}:${frame.column}`); - } - return stackLines; -} - export function serializeError(error: Error | any): TestInfoError { if (error instanceof Error) return filterStackTrace(error); diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index bd148f1bb3..922a6db2e8 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -16,7 +16,7 @@ import fs from 'fs'; import path from 'path'; -import { MaxTime, captureRawStack, monotonicTime, zones, sanitizeForFilePath } from 'playwright-core/lib/utils'; +import { MaxTime, captureRawStack, monotonicTime, zones, sanitizeForFilePath, stringifyStackFrames } from 'playwright-core/lib/utils'; import type { TestInfoError, TestInfo, TestStatus, FullProject, FullConfig } from '../../types/test'; import type { AttachmentPayload, StepBeginPayload, StepEndPayload, WorkerInitParams } from '../common/ipc'; import type { TestCase } from '../common/test'; @@ -24,7 +24,7 @@ import { TimeoutManager } from './timeoutManager'; import type { RunnableType, TimeSlot } from './timeoutManager'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; import type { Location } from '../../types/testReporter'; -import { filteredStackTrace, getContainedPath, normalizeAndSaveAttachment, serializeError, stringifyStackFrames, trimLongString } from '../util'; +import { filteredStackTrace, getContainedPath, normalizeAndSaveAttachment, serializeError, trimLongString } from '../util'; import { TestTracing } from './testTracing'; import type { Attachment } from './testTracing'; diff --git a/tests/playwright-test/golden.spec.ts b/tests/playwright-test/golden.spec.ts index 643fcaa738..ae094c27fb 100644 --- a/tests/playwright-test/golden.spec.ts +++ b/tests/playwright-test/golden.spec.ts @@ -227,7 +227,7 @@ test('should write missing expectations locally twice and continue', async ({ ru expect(result.output).toContain('Here we are!'); - const stackLines = result.output.split('\n').filter(line => line.includes(' at ')).filter(line => !line.includes(testInfo.outputPath())); + const stackLines = result.output.split('\n').filter(line => line.includes(' at ')).filter(line => !line.includes('a.spec.js')); expect(result.output).toContain('a.spec.js:4'); expect(stackLines.length).toBe(0); }); diff --git a/tests/playwright-test/playwright.spec.ts b/tests/playwright-test/playwright.spec.ts index 3e259018cc..e276374713 100644 --- a/tests/playwright-test/playwright.spec.ts +++ b/tests/playwright-test/playwright.spec.ts @@ -340,8 +340,10 @@ test('should report error and pending operations on timeout', async ({ runInline expect(result.passed).toBe(0); expect(result.failed).toBe(1); expect(result.output).toContain('Pending operations:'); - expect(result.output).toContain('- locator.click at a.test.ts:6:37'); - expect(result.output).toContain('- locator.textContent at a.test.ts:7:42'); + expect(result.output).toContain('- locator.click'); + expect(result.output).toContain('a.test.ts:6:37'); + expect(result.output).toContain('- locator.textContent'); + expect(result.output).toContain('a.test.ts:7:42'); expect(result.output).toContain('waiting for'); expect(result.output).toContain(`7 | page.getByText('More missing').textContent(),`); }); @@ -410,7 +412,8 @@ test('should not report waitForEventInfo as pending', async ({ runInlineTest }) expect(result.passed).toBe(0); expect(result.failed).toBe(1); expect(result.output).toContain('Pending operations:'); - expect(result.output).toContain('- page.click at a.test.ts:6:20'); + expect(result.output).toContain('- page.click'); + expect(result.output).toContain('a.test.ts:6:20'); expect(result.output).not.toContain('- page.waitForLoadState'); }); diff --git a/tests/playwright-test/to-have-screenshot.spec.ts b/tests/playwright-test/to-have-screenshot.spec.ts index ca7b20f9a0..20cac36936 100644 --- a/tests/playwright-test/to-have-screenshot.spec.ts +++ b/tests/playwright-test/to-have-screenshot.spec.ts @@ -598,7 +598,7 @@ test('should write missing expectations locally twice and attach them', async ({ expect(result.output).toContain('Here we are!'); - const stackLines = result.output.split('\n').filter(line => line.includes(' at ')).filter(line => !line.includes(testInfo.outputPath())); + const stackLines = result.output.split('\n').filter(line => line.includes(' at ')).filter(line => !line.includes('a.spec.js')); expect(result.output).toContain('a.spec.js:5'); expect(stackLines.length).toBe(0);