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.

Sign up for GitHub

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

chore: remove dependency from library on expectZone, straighten csi handling #34211

Merged
merged 2 commits into from
Jan 7, 2025
Merged

chore: remove dependency from library on expectZone, straighten csi handling #34211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6429c76
refactor
dgozman Jan 4, 2025
a8857dc
remove library dependency on expectZone
dgozman Jan 4, 2025
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
75 changes: 32 additions & 43 deletions packages/playwright-core/src/client/channelOwner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { EventEmitter } from './eventEmitter';
import type * as channels from '@protocol/channels';
import { maybeFindValidator, ValidationError, type ValidatorContext } from '../protocol/validator';
import { debugLogger } from '../utils/debugLogger';
import type { ExpectZone } from '../utils/stackTrace';
import { captureLibraryStackTrace, stringifyStackFrames } from '../utils/stackTrace';
import { isUnderTest } from '../utils';
import { zones } from '../utils/zones';
Expand Down Expand Up @@ -148,15 +147,18 @@ export abstract class ChannelOwner<T extends channels.Channel = channels.Channel
if (validator) {
return async (params: any) => {
return await this._wrapApiCall(async apiZone => {
const { apiName, frames, csi, callCookie, stepId } = apiZone.reported ? { apiName: undefined, csi: undefined, callCookie: undefined, frames: [], stepId: undefined } : apiZone;
apiZone.reported = true;
let currentStepId = stepId;
if (csi && apiName) {
const out: { stepId?: string } = {};
csi.onApiCallBegin(apiName, params, frames, callCookie, out);
currentStepId = out.stepId;
const validatedParams = validator(params, '', { tChannelImpl: tChannelImplToWire, binary: this._connection.rawBuffers() ? 'buffer' : 'toBase64' });
if (!apiZone.isInternal && !apiZone.reported) {
// Reporting/tracing/logging this api call for the first time.
apiZone.params = params;
apiZone.reported = true;
this._instrumentation.onApiCallBegin(apiZone);
logApiCall(this._logger, `=> ${apiZone.apiName} started`);
return await this._connection.sendMessageToServer(this, prop, validatedParams, apiZone.apiName, apiZone.frames, apiZone.stepId);
}
return await this._connection.sendMessageToServer(this, prop, validator(params, '', { tChannelImpl: tChannelImplToWire, binary: this._connection.rawBuffers() ? 'buffer' : 'toBase64' }), apiName, frames, currentStepId);
// Since this api call is either internal, or has already been reported/traced once,
// passing undefined apiName will avoid an extra unneeded tracing entry.
return await this._connection.sendMessageToServer(this, prop, validatedParams, undefined, [], undefined);
});
};
}
Expand All @@ -170,48 +172,36 @@ export abstract class ChannelOwner<T extends channels.Channel = channels.Channel

async _wrapApiCall<R>(func: (apiZone: ApiZone) => Promise<R>, isInternal?: boolean): Promise<R> {
const logger = this._logger;
const apiZone = zones.zoneData<ApiZone>('apiZone');
if (apiZone)
return await func(apiZone);

const stackTrace = captureLibraryStackTrace();
let apiName: string | undefined = stackTrace.apiName;
const frames: channels.StackFrame[] = stackTrace.frames;
const existingApiZone = zones.zoneData<ApiZone>('apiZone');
if (existingApiZone)
return await func(existingApiZone);

if (isInternal === undefined)
isInternal = this._isInternalType;
if (isInternal)
apiName = undefined;

// Enclosing zone could have provided the apiName and wallTime.
const expectZone = zones.zoneData<ExpectZone>('expectZone');
const stepId = expectZone?.stepId;
if (!isInternal && expectZone)
apiName = expectZone.title;

// If we are coming from the expectZone, there is no need to generate a new
// step for the API call, since it will be generated by the expect itself.
const csi = isInternal || expectZone ? undefined : this._instrumentation;
const callCookie: any = {};
const stackTrace = captureLibraryStackTrace();
const apiZone: ApiZone = { apiName: stackTrace.apiName, frames: stackTrace.frames, isInternal, reported: false, userData: undefined, stepId: undefined };

try {
logApiCall(logger, `=> ${apiName} started`, isInternal);
const apiZone: ApiZone = { apiName, frames, isInternal, reported: false, csi, callCookie, stepId };
const result = await zones.run('apiZone', apiZone, async () => await func(apiZone));
csi?.onApiCallEnd(callCookie);
logApiCall(logger, `<= ${apiName} succeeded`, isInternal);
if (!isInternal) {
logApiCall(logger, `<= ${apiZone.apiName} succeeded`);
this._instrumentation.onApiCallEnd(apiZone);
}
return result;
} catch (e) {
const innerError = ((process.env.PWDEBUGIMPL || isUnderTest()) && e.stack) ? '\n<inner error>\n' + e.stack : '';
if (apiName && !apiName.includes('<anonymous>'))
e.message = apiName + ': ' + e.message;
if (apiZone.apiName && !apiZone.apiName.includes('<anonymous>'))
e.message = apiZone.apiName + ': ' + e.message;
const stackFrames = '\n' + stringifyStackFrames(stackTrace.frames).join('\n') + innerError;
if (stackFrames.trim())
e.stack = e.message + stackFrames;
else
e.stack = '';
csi?.onApiCallEnd(callCookie, e);
logApiCall(logger, `<= ${apiName} failed`, isInternal);
if (!isInternal) {
apiZone.error = e;
logApiCall(logger, `<= ${apiZone.apiName} failed`);
this._instrumentation.onApiCallEnd(apiZone);
}
throw e;
}
}
Expand All @@ -232,9 +222,7 @@ export abstract class ChannelOwner<T extends channels.Channel = channels.Channel
}
}

function logApiCall(logger: Logger | undefined, message: string, isNested: boolean) {
if (isNested)
return;
function logApiCall(logger: Logger | undefined, message: string) {
if (logger && logger.isEnabled('api', 'info'))
logger.log('api', 'info', message, [], { color: 'cyan' });
debugLogger.log('api', message);
Expand All @@ -247,11 +235,12 @@ function tChannelImplToWire(names: '*' | string[], arg: any, path: string, conte
}

type ApiZone = {
apiName: string | undefined;
apiName: string;
params?: Record<string, any>;
frames: channels.StackFrame[];
isInternal: boolean;
reported: boolean;
csi: ClientInstrumentation | undefined;
callCookie: any;
userData: any;
stepId?: string;
error?: Error;
};
18 changes: 14 additions & 4 deletions packages/playwright-core/src/client/clientInstrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,22 @@ import type { StackFrame } from '@protocol/channels';
import type { BrowserContext } from './browserContext';
import type { APIRequestContext } from './fetch';

// Instrumentation can mutate the data, for example change apiName or stepId.
export interface ApiCallData {
apiName: string;
params?: Record<string, any>;
frames: StackFrame[];
userData: any;
stepId?: string;
error?: Error;
}

export interface ClientInstrumentation {
addListener(listener: ClientInstrumentationListener): void;
removeListener(listener: ClientInstrumentationListener): void;
removeAllListeners(): void;
onApiCallBegin(apiCall: string, params: Record<string, any>, frames: StackFrame[], userData: any, out: { stepId?: string }): void;
onApiCallEnd(userData: any, error?: Error): void;
onApiCallBegin(apiCall: ApiCallData): void;
onApiCallEnd(apiCal: ApiCallData): void;
onWillPause(options: { keepTestTimeout: boolean }): void;

runAfterCreateBrowserContext(context: BrowserContext): Promise<void>;
Expand All @@ -33,8 +43,8 @@ export interface ClientInstrumentation {
}

export interface ClientInstrumentationListener {
onApiCallBegin?(apiName: string, params: Record<string, any>, frames: StackFrame[], userData: any, out: { stepId?: string }): void;
onApiCallEnd?(userData: any, error?: Error): void;
onApiCallBegin?(apiCall: ApiCallData): void;
onApiCallEnd?(apiCall: ApiCallData): void;
onWillPause?(options: { keepTestTimeout: boolean }): void;

runAfterCreateBrowserContext?(context: BrowserContext): Promise<void>;
Expand Down
4 changes: 2 additions & 2 deletions packages/playwright-core/src/client/connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ export class Connection extends EventEmitter {

constructor(localUtils: LocalUtils | undefined, instrumentation: ClientInstrumentation | undefined) {
super();
this._rootObject = new Root(this);
this._localUtils = localUtils;
this._instrumentation = instrumentation || createInstrumentation();
this._localUtils = localUtils;
this._rootObject = new Root(this);
}

markAsRemote() {
Expand Down
53 changes: 28 additions & 25 deletions packages/playwright/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@ import * as fs from 'fs';
import * as path from 'path';
import type { APIRequestContext, BrowserContext, Browser, BrowserContextOptions, LaunchOptions, Page, Tracing, Video } from 'playwright-core';
import * as playwrightLibrary from 'playwright-core';
import { createGuid, debugMode, addInternalStackPrefix, isString, asLocator, jsonStringifyForceASCII } from 'playwright-core/lib/utils';
import { createGuid, debugMode, addInternalStackPrefix, isString, asLocator, jsonStringifyForceASCII, zones } from 'playwright-core/lib/utils';
import type { ExpectZone } from 'playwright-core/lib/utils';
import type { Fixtures, PlaywrightTestArgs, PlaywrightTestOptions, PlaywrightWorkerArgs, PlaywrightWorkerOptions, ScreenshotMode, TestInfo, TestType, VideoMode } from '../types/test';
import type { TestInfoImpl, TestStepInternal } 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 { ApiCallData, ClientInstrumentation, ClientInstrumentationListener } from '../../playwright-core/src/client/clientInstrumentation';
import { currentTestInfo } from './common/globals';
export { expect } from './matchers/expect';
export const _baseTest: TestType<{}, {}> = rootTestType.test;
Expand Down Expand Up @@ -258,34 +259,43 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({

const tracingGroupSteps: TestStepInternal[] = [];
const csiListener: ClientInstrumentationListener = {
onApiCallBegin: (apiName: string, params: Record<string, any>, frames: StackFrame[], userData: any, out: { stepId?: string }) => {
userData.apiName = apiName;
onApiCallBegin: (data: ApiCallData) => {
const testInfo = currentTestInfo();
if (!testInfo || apiName.includes('setTestIdAttribute') || apiName === 'tracing.groupEnd')
// Some special calls do not get into steps.
if (!testInfo || data.apiName.includes('setTestIdAttribute') || data.apiName === 'tracing.groupEnd')
return;
const expectZone = zones.zoneData<ExpectZone>('expectZone');
if (expectZone) {
// Display the internal locator._expect call under the name of the enclosing expect call,
// and connect it to the existing expect step.
data.apiName = expectZone.title;
data.stepId = expectZone.stepId;
return;
}
// In the general case, create a step for each api call and connect them through the stepId.
const step = testInfo._addStep({
location: frames[0] as any,
location: data.frames[0],
category: 'pw:api',
title: renderApiCall(apiName, params),
apiName,
params,
title: renderApiCall(data.apiName, data.params),
apiName: data.apiName,
params: data.params,
}, tracingGroupSteps[tracingGroupSteps.length - 1]);
userData.step = step;
out.stepId = step.stepId;
if (apiName === 'tracing.group')
data.userData = step;
data.stepId = step.stepId;
if (data.apiName === 'tracing.group')
tracingGroupSteps.push(step);
},
onApiCallEnd: (userData: any, error?: Error) => {
onApiCallEnd: (data: ApiCallData) => {
// "tracing.group" step will end later, when "tracing.groupEnd" finishes.
if (userData.apiName === 'tracing.group')
if (data.apiName === 'tracing.group')
return;
if (userData.apiName === 'tracing.groupEnd') {
if (data.apiName === 'tracing.groupEnd') {
const step = tracingGroupSteps.pop();
step?.complete({ error });
step?.complete({ error: data.error });
return;
}
const step = userData.step;
step?.complete({ error });
const step = data.userData;
step?.complete({ error: data.error });
},
onWillPause: ({ keepTestTimeout }) => {
if (!keepTestTimeout)
Expand Down Expand Up @@ -441,13 +451,6 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
},
});

type StackFrame = {
file: string,
line?: number,
column?: number,
function?: string,
};

type ScreenshotOption = PlaywrightWorkerOptions['screenshot'] | undefined;
type Playwright = PlaywrightWorkerArgs['playwright'];

Expand Down
Loading