diff --git a/packages/isomorphic/trace/traceLoader.ts b/packages/isomorphic/trace/traceLoader.ts index 6803a3977a569..f2665fcba4de2 100644 --- a/packages/isomorphic/trace/traceLoader.ts +++ b/packages/isomorphic/trace/traceLoader.ts @@ -14,8 +14,6 @@ * limitations under the License. */ -import { parseClientSideCallMetadata } from './traceUtils'; - import { SnapshotStorage } from './snapshotStorage'; import { TraceModernizer } from './traceModernizer'; @@ -72,6 +70,11 @@ export class TraceLoader { modernizer.appendTrace(network); unzipProgress?.(++done, total); + const stacks = await this._backend.readText(prefix + '.stacks'); + if (stacks) + modernizer.appendStacks(stacks); + unzipProgress?.(++done, total); + contextEntry.actions = modernizer.actions().sort((a1, a2) => a1.startTime - a2.startTime); if (!backend.isLive()) { @@ -88,14 +91,6 @@ export class TraceLoader { } } - const stacks = await this._backend.readText(prefix + '.stacks'); - if (stacks) { - const callMetadata = parseClientSideCallMetadata(JSON.parse(stacks)); - for (const action of contextEntry.actions) - action.stack = action.stack || callMetadata.get(action.callId); - } - unzipProgress?.(++done, total); - for (const resource of contextEntry.resources) { if (resource.request.postData?._file) this._resourceToContentType.set(resource.request.postData._file, stripEncodingFromContentType(resource.request.postData.mimeType)); diff --git a/packages/isomorphic/trace/traceModel.ts b/packages/isomorphic/trace/traceModel.ts index 851a13c056f88..74938e4f1bd66 100644 --- a/packages/isomorphic/trace/traceModel.ts +++ b/packages/isomorphic/trace/traceModel.ts @@ -288,8 +288,6 @@ function mergeActionsAndUpdateTiming(contexts: ContextEntry[]) { return result; } -let lastTmpStepId = 0; - function mergeActionsAndUpdateTimingSameTrace(contexts: ContextEntry[]): ActionEntry[] { const map = new Map(); @@ -311,37 +309,31 @@ function mergeActionsAndUpdateTimingSameTrace(contexts: ContextEntry[]): ActionE } for (const context of libraryContexts) { - for (const action of context.actions) { - // Never merge stepless events. - map.set(action.stepId || `tmp-step@${++lastTmpStepId}`, { ...action }); - } + for (const action of context.actions) + map.set(action.callId, { ...action }); } - const nonPrimaryIdToPrimaryId = new Map(); for (const context of testRunnerContexts) { for (const action of context.actions) { - const existing = action.stepId && map.get(action.stepId); - if (existing) { - nonPrimaryIdToPrimaryId.set(action.callId, existing.callId); - if (action.error) - existing.error = action.error; - if (action.attachments) - existing.attachments = action.attachments; - if (action.annotations) - existing.annotations = action.annotations; - if (action.parentId) - existing.parentId = nonPrimaryIdToPrimaryId.get(action.parentId) ?? action.parentId; - if (action.group) - existing.group = action.group; - // For the events that are present in the test runner context, always take - // their time from the test runner context to preserve client side order. - existing.startTime = action.startTime; - existing.endTime = action.endTime; + const existing = map.get(action.callId); + if (!existing) { + map.set(action.callId, { ...action }); continue; } + if (action.error) + existing.error = action.error; + if (action.attachments) + existing.attachments = action.attachments; + if (action.annotations) + existing.annotations = action.annotations; if (action.parentId) - action.parentId = nonPrimaryIdToPrimaryId.get(action.parentId) ?? action.parentId; - map.set(action.stepId || `tmp-step@${++lastTmpStepId}`, { ...action }); + existing.parentId = action.parentId; + if (action.group) + existing.group = action.group; + // For the events that are present in the test runner context, always take + // their time from the test runner context to preserve client side order. + existing.startTime = action.startTime; + existing.endTime = action.endTime; } } return [...map.values()]; diff --git a/packages/isomorphic/trace/traceModernizer.ts b/packages/isomorphic/trace/traceModernizer.ts index b165702e75c17..2fcd5284f9198 100644 --- a/packages/isomorphic/trace/traceModernizer.ts +++ b/packages/isomorphic/trace/traceModernizer.ts @@ -14,6 +14,9 @@ * limitations under the License. */ +import { defaultCallId, parseClientSideCallMetadata } from './traceUtils'; + +import type { SerializedStack } from './traceUtils'; import type * as trace from './trace'; import type * as traceV3 from './versions/traceV3'; import type * as traceV4 from './versions/traceV4'; @@ -46,6 +49,7 @@ export class TraceModernizer { private _consoleObjects = new Map(); private _apiRequestRef: string | undefined; private _snapshotPhases = new Map(); + private _legacyCallIdToStepId = new Map(); constructor(contextEntry: ContextEntry, snapshotStorage: SnapshotStorage) { this._contextEntry = contextEntry; @@ -57,6 +61,18 @@ export class TraceModernizer { this._appendEvent(line); } + appendStacks(stacks: string) { + const data = JSON.parse(stacks); + const normalized: SerializedStack[] = data.stacks.map(([id, ...rest]: any) => { + // Transform legacy numeric call ids into string ids. + const callId = typeof id === 'number' ? defaultCallId(id) : id; + return [this._legacyCallIdToStepId.get(callId) ?? callId, ...rest]; + }); + const callMetadata = parseClientSideCallMetadata({ files: data.files, stacks: normalized }); + for (const action of this._actionMap.values()) + action.stack = action.stack || callMetadata.get(action.callId); + } + actions(): ActionEntry[] { return [...this._actionMap.values()]; } @@ -469,6 +485,21 @@ export class TraceModernizer { _modernize_8_to_9(events: traceV8.TraceEvent[]): trace.TraceEvent[] { for (const event of events) { + // The library and the test runner used to mint their own id for the same call and reconcile + // them through a `stepId` side-channel. Now they share a single id - adopt the step id as the + // call id, remembering the mapping for the ids that `appendStacks` will see. + if (event.type === 'before' || event.type === 'action') { + if (event.stepId && event.stepId !== event.callId) + this._legacyCallIdToStepId.set(event.callId, event.stepId); + delete event.stepId; + if (event.parentId) + event.parentId = this._legacyCallIdToStepId.get(event.parentId) ?? event.parentId; + } + if (event.type === 'before' || event.type === 'input' || event.type === 'after' || event.type === 'action' || event.type === 'log') + event.callId = this._legacyCallIdToStepId.get(event.callId) ?? event.callId; + if (event.type === 'frame-snapshot') + event.snapshot.callId = this._legacyCallIdToStepId.get(event.snapshot.callId) ?? event.snapshot.callId; + // Actions used to point at their snapshots by name, now snapshots know their own phase. if (event.type === 'before' || event.type === 'input' || event.type === 'after' || event.type === 'action') { const action = event as traceV8.ActionTraceEvent; diff --git a/packages/isomorphic/trace/traceUtils.ts b/packages/isomorphic/trace/traceUtils.ts index ab138b4a515ae..39f850100b46c 100644 --- a/packages/isomorphic/trace/traceUtils.ts +++ b/packages/isomorphic/trace/traceUtils.ts @@ -18,19 +18,23 @@ import type { StackFrame } from './trace'; import type { ClientSideCallMetadata } from '@protocol/structs'; export type SerializedStackFrame = [number, number, number, string]; -export type SerializedStack = [number, SerializedStackFrame[]]; +export type SerializedStack = [string, SerializedStackFrame[]]; export type SerializedClientSideCallMetadata = { files: string[]; stacks: SerializedStack[]; }; +export function defaultCallId(ordinal: number): string { + return `call@${ordinal}`; +} + export function parseClientSideCallMetadata(data: SerializedClientSideCallMetadata): Map { const result = new Map(); const { files, stacks } = data; for (const s of stacks) { const [id, ff] = s; - result.set(`call@${id}`, ff.map(f => ({ file: files[f[0]], line: f[1], column: f[2], function: f[3] }))); + result.set(id, ff.map(f => ({ file: files[f[0]], line: f[1], column: f[2], function: f[3] }))); } return result; } diff --git a/packages/isomorphic/trace/versions/traceV9.ts b/packages/isomorphic/trace/versions/traceV9.ts index 58db207ef1a00..7ea87d5252f70 100644 --- a/packages/isomorphic/trace/versions/traceV9.ts +++ b/packages/isomorphic/trace/versions/traceV9.ts @@ -148,7 +148,6 @@ export type BeforeActionTraceEvent = { class: string; method: string; params: Record; - stepId?: string; stack?: StackFrame[]; parentId?: string; group?: string; diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index e8c139bbb047c..079c4e36c45a2 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -196,7 +196,7 @@ export abstract class ChannelOwner await func(apiZone)); @@ -254,6 +254,6 @@ type ApiZone = { internal?: boolean; reported: boolean; userData: any; - stepId?: string; + callId?: string; error?: Error; }; diff --git a/packages/playwright-core/src/client/clientInstrumentation.ts b/packages/playwright-core/src/client/clientInstrumentation.ts index 25e05257ace72..0552acad53e32 100644 --- a/packages/playwright-core/src/client/clientInstrumentation.ts +++ b/packages/playwright-core/src/client/clientInstrumentation.ts @@ -20,12 +20,12 @@ import type { StackFrame } from './channels'; import type { Page } from './page'; import type { BrowserContextOptions } from './types'; -// Instrumentation can mutate the data, for example change the stepId. +// Instrumentation can mutate the data, for example assign the callId. export interface ApiCallData { title?: string; frames: StackFrame[]; userData: any; - stepId?: string; + callId?: string; error?: Error; } diff --git a/packages/playwright-core/src/client/connection.ts b/packages/playwright-core/src/client/connection.ts index 58780fea8d1c0..08c8242abe300 100644 --- a/packages/playwright-core/src/client/connection.ts +++ b/packages/playwright-core/src/client/connection.ts @@ -20,6 +20,7 @@ import { isUnderTest } from '@utils/debug'; import { debugLogger } from '@utils/debugLogger'; import { emptyZone } from '@utils/zones'; import { ValidationError, findValidator, maybeFindValidator } from '@protocol/validator'; +import { defaultCallId } from '@isomorphic/trace/traceUtils'; import { EventEmitter } from './eventEmitter'; import { Android, AndroidDevice, AndroidSocket } from './android'; import { Artifact } from './artifact'; @@ -73,8 +74,8 @@ export type ChannelOwnerFactory = (parent: ChannelOwner, type: string, guid: str 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, signal: AbortSignal | undefined, title: string | undefined, type: string, method: string }>(); + private _lastOrdinal = 0; + private _callbacks = new Map void, reject: (a: Error) => void, signal: AbortSignal | undefined, title: string | undefined, type: string, method: string }>(); private _rootObject: Root; private _closedError: Error | undefined; private _isRemote = false; @@ -175,7 +176,7 @@ export class Connection extends EventEmitter { this._tracingCount--; } - async sendMessageToServer(object: ChannelOwner, method: string, params: any, options: { apiName?: string, title?: string, internal?: boolean, frames?: channels.StackFrame[], stepId?: string, signal?: AbortSignal, timeout: number }): Promise { + async sendMessageToServer(object: ChannelOwner, method: string, params: any, options: { apiName?: string, title?: string, internal?: boolean, frames?: channels.StackFrame[], callId?: string, signal?: AbortSignal, timeout: number }): Promise { // Fire-and-forget: server intentionally never replies to __waitInfo__, // so silently drop it after the connection is closed or the object was collected. if (method === '__waitInfo__' && (this._closedError || object._wasCollected)) @@ -191,14 +192,14 @@ export class Connection extends EventEmitter { const guid = object._guid; const type = object._type; - const id = ++this._lastId; + const id = options.callId ?? defaultCallId(++this._lastOrdinal); const message = { id, guid, method, params }; if (debugLogger.isEnabled('channel')) { // Do not include metadata in debug logs to avoid noise. debugLogger.log('channel', 'SEND> ' + JSON.stringify(message)); } const location = options.frames?.[0] ? { file: options.frames[0].file, line: options.frames[0].line, column: options.frames[0].column } : undefined; - const metadata: channels.Metadata = { title: options.title, location, internal: options.internal, stepId: options.stepId, timeout: options.timeout }; + const metadata: channels.Metadata = { title: options.title, location, internal: options.internal, timeout: options.timeout }; if (this._tracingCount && options.frames && type !== 'LocalUtils') this._localUtils?.addStackToTracingNoReply({ callData: { stack: options.frames ?? [], id } }).catch(() => {}); // We need to exit zones before calling into the server, otherwise diff --git a/packages/playwright-core/src/server/dispatchers/dispatcher.ts b/packages/playwright-core/src/server/dispatchers/dispatcher.ts index 1cc3b578e0225..bb2e18e418cc0 100644 --- a/packages/playwright-core/src/server/dispatchers/dispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/dispatcher.ts @@ -297,7 +297,7 @@ export class DispatcherConnection { } async dispatch(message: object) { - const { id, guid, method, params, metadata } = message as any; + const { id, guid, method, params, metadata } = message as { id: string, guid: string, method: string, params: any, metadata: any }; const dispatcher = this._dispatcherByGuid.get(guid); if (method === '__waitInfo__') { // Fire-and-forget: silently drop if the target is gone. @@ -306,7 +306,7 @@ export class DispatcherConnection { return; } if (method === '__abort__') { - const entry = this._activeProgressControllers.get(`call@${params.id}`); + const entry = this._activeProgressControllers.get(params.id); if (!entry) return; entry.abortError = new AbortError(params.reason); @@ -342,11 +342,10 @@ export class DispatcherConnection { const sdkObject = dispatcher._object; const callMetadata: CallMetadata = { - id: `call@${id}`, + id, location: validMetadata.location, title: validMetadata.title, internal: validMetadata.internal, - stepId: validMetadata.stepId, objectId: sdkObject.guid, startTime: monotonicTime(), endTime: 0, @@ -358,7 +357,7 @@ export class DispatcherConnection { }; const abortControllerEntry: { controller?: ProgressController, abortError?: Error } = {}; - this._activeProgressControllers.set(callMetadata.id, abortControllerEntry); + this._activeProgressControllers.set(id, abortControllerEntry); const swapProgressController = () => { const controller = dispatcher.createProgressController(callMetadata, abortControllerEntry.abortError); abortControllerEntry.controller = controller; @@ -403,7 +402,7 @@ export class DispatcherConnection { await afterController.run(progress => sdkObject.instrumentation.onAfterCall(progress, sdkObject), 3000).catch(() => {}); if (metainfo?.slowMo) await this._doSlowMo(sdkObject); - this._activeProgressControllers.delete(callMetadata.id); + this._activeProgressControllers.delete(id); } if (response.error) @@ -417,7 +416,7 @@ export class DispatcherConnection { await new Promise(f => setTimeout(f, slowMo)); } - private async _dispatchWaitInfo(id: number, dispatcher: DispatcherScope, params: any, metadata: any) { + private async _dispatchWaitInfo(id: string, dispatcher: DispatcherScope, params: any, metadata: any) { // Fire-and-forget notification: never reply, never throw to the caller. let info: channels.WaitInfo; let validMetadata: channels.Metadata; @@ -432,11 +431,10 @@ export class DispatcherConnection { const sdkObject = dispatcher._object; if (info.phase === 'before') { const callMetadata: CallMetadata = { - id: `call@${id}`, + id, location: validMetadata.location, title: validMetadata.title, internal: validMetadata.internal, - stepId: validMetadata.stepId, objectId: sdkObject.guid, startTime: monotonicTime(), endTime: 0, diff --git a/packages/playwright-core/src/server/instrumentation.ts b/packages/playwright-core/src/server/instrumentation.ts index 20187cca4db81..a2de6a9806fbf 100644 --- a/packages/playwright-core/src/server/instrumentation.ts +++ b/packages/playwright-core/src/server/instrumentation.ts @@ -96,8 +96,6 @@ export type CallMetadata = { // Client is making an internal call that should not show up in // the inspector or trace. internal?: boolean; - // Test runner step id. - stepId?: string; location?: { file: string, line?: number, column?: number }; log: string[]; error?: SerializedError; diff --git a/packages/playwright-core/src/server/trace/recorder/tracing.ts b/packages/playwright-core/src/server/trace/recorder/tracing.ts index 4c9b282782c24..21a125271fff5 100644 --- a/packages/playwright-core/src/server/trace/recorder/tracing.ts +++ b/packages/playwright-core/src/server/trace/recorder/tracing.ts @@ -263,7 +263,6 @@ export class Tracing extends SdkObject implements InstrumentationListener, Snaps class: 'Tracing', method: 'tracingGroup', params: { }, - stepId: metadata.stepId, stack: stackFrames, }; if (this._currentGroupId()) @@ -782,7 +781,6 @@ function createBeforeActionTraceEvent(metadata: CallMetadata, parentId?: string) class: metadata.type, method: metadata.method, params: metadata.timeout ? { ...metadata.params, timeout: metadata.timeout } : metadata.params, - stepId: metadata.stepId, }; if (parentId) event.parentId = parentId; diff --git a/packages/playwright/src/index.ts b/packages/playwright/src/index.ts index fc75232654e17..ec8cd3c25b332 100644 --- a/packages/playwright/src/index.ts +++ b/packages/playwright/src/index.ts @@ -106,11 +106,11 @@ const utilityFixtures: Fixtures = { const zone = currentZone().data('stepZone'); const isExpectCall = (channel.type === 'Frame' && channel.method === 'expect') || (channel.type === 'Page' && channel.method === 'expectScreenshot'); if (zone && zone.category === 'expect' && isExpectCall) { - data.stepId = zone.stepId; + data.callId = zone.stepId; return; } - // In the general case, create a step for each api call and connect them through the stepId. + // In the general case, create a step for each api call, use it as a callId. const params = renderParamsForCall({ type: channel.type, method: channel.method, params: channel.params }); const step = testInfo._addStep({ location: data.frames[0], @@ -120,7 +120,7 @@ const utilityFixtures: Fixtures = { params, group: getActionGroup({ type: channel.type, method: channel.method }), }, tracingGroupSteps[tracingGroupSteps.length - 1]); - data.stepId = step.stepId; + data.callId = step.stepId; if (channel.type === 'Tracing' && channel.method === 'tracingGroup') { // The step will end later, when the corresponding "tracing.groupEnd" call finishes. tracingGroupSteps.push(step); diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts index 1197dccc64c09..964523195ca45 100644 --- a/packages/playwright/src/worker/testInfo.ts +++ b/packages/playwright/src/worker/testInfo.ts @@ -82,6 +82,9 @@ export const emtpyTestInfoCallbacks: TestInfoCallbacks = { onTestPaused: () => Promise.reject(new Error('TestInfoImpl not initialized')), }; +// Keep step ids globally unique, to avoid cross-test callId collisions on the same playwright instance. +let lastStepId = 0; + export class TestInfoImpl implements TestInfo { private _callbacks: TestInfoCallbacks; private _snapshotNames: SnapshotNames = { lastAnonymousSnapshotIndex: 0, lastNamedSnapshotIndex: {} }; @@ -93,7 +96,6 @@ export class TestInfoImpl implements TestInfo { readonly _uniqueSymbol; private _interruptedPromise = new ManualPromise(); - _lastStepId = 0; private readonly _requireFile: string; readonly _projectInternal: commonConfig.FullProjectInternal; readonly _configInternal: FullConfigInternal; @@ -284,7 +286,7 @@ export class TestInfoImpl implements TestInfo { } _addStep(data: Readonly, parentStep?: TestStepInternal): TestStepInternal { - const stepId = `${data.category}@${++this._lastStepId}`; + const stepId = `${data.category}@${++lastStepId}`; if (data.category === 'hook' || data.category === 'fixture') { // Predefined steps form a fixed hierarchy - use the current one as parent. diff --git a/packages/playwright/src/worker/testTracing.ts b/packages/playwright/src/worker/testTracing.ts index 0583e936b2eee..fdad3e6b203b0 100644 --- a/packages/playwright/src/worker/testTracing.ts +++ b/packages/playwright/src/worker/testTracing.ts @@ -277,7 +277,6 @@ export class TestTracing { this._appendTraceEvent({ type: 'before', callId: options.stepId, - stepId: options.stepId, parentId: options.parentId, startTime: monotonicTime(), class: 'Test', diff --git a/packages/protocol/spec/core.yml b/packages/protocol/spec/core.yml index f29b82f56d5a3..50472aedd3a32 100644 --- a/packages/protocol/spec/core.yml +++ b/packages/protocol/spec/core.yml @@ -23,14 +23,12 @@ Metadata: column: int? title: string? internal: boolean? - # Test runner step id. - stepId: string? timeout: float? ClientSideCallMetadata: type: object properties: - id: int + id: string stack: type: array? items: StackFrame diff --git a/packages/protocol/src/structs.d.ts b/packages/protocol/src/structs.d.ts index 9c637e7398574..c40e34fc11a04 100644 --- a/packages/protocol/src/structs.d.ts +++ b/packages/protocol/src/structs.d.ts @@ -91,12 +91,11 @@ export type Metadata = { }, title?: string, internal?: boolean, - stepId?: string, timeout?: number, }; export type ClientSideCallMetadata = { - id: number, + id: string, stack?: StackFrame[], }; diff --git a/packages/protocol/src/validator.ts b/packages/protocol/src/validator.ts index 565a6a18cf956..fbbfee0dc09b9 100644 --- a/packages/protocol/src/validator.ts +++ b/packages/protocol/src/validator.ts @@ -1081,11 +1081,10 @@ scheme.Metadata = tObject({ })), title: tOptional(tString), internal: tOptional(tBoolean), - stepId: tOptional(tString), timeout: tOptional(tFloat), }); scheme.ClientSideCallMetadata = tObject({ - id: tInt, + id: tString, stack: tOptional(tArray(tType('StackFrame'))), }); scheme.SDKLanguage = tEnum(['javascript', 'python', 'java', 'csharp']);