remove library dependency on expectZone

microsoft · Jan 5, 2025 · a8857dc · a8857dc
1 parent 6429c76
commit a8857dc
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 36 deletions.
diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts
@@ -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';
@@ -150,12 +149,15 @@ export abstract class ChannelOwner<T extends channels.Channel = channels.Channel
               return await this._wrapApiCall(async apiZone => {
                 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`);
-                  apiZone.csi?.onApiCallBegin(apiZone);
                   return await this._connection.sendMessageToServer(this, prop, validatedParams, apiZone.apiName, apiZone.frames, apiZone.stepId);
                 }
+                // 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);
               });
             };
@@ -174,42 +176,32 @@ export abstract class ChannelOwner<T extends channels.Channel = channels.Channel
     if (existingApiZone)
       return await func(existingApiZone);
 
-    const stackTrace = captureLibraryStackTrace();
-    let apiName: string | undefined = stackTrace.apiName;
-    const frames: channels.StackFrame[] = stackTrace.frames;
-
     if (isInternal === undefined)
       isInternal = this._isInternalType;
-
-    // Enclosing zone could have provided the apiName and wallTime.
-    const expectZone = zones.zoneData<ExpectZone>('expectZone');
-    const stepId = expectZone?.stepId;
-    if (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 apiZone: ApiZone = { apiName, frames, isInternal, reported: false, csi, userData: undefined, stepId };
+    const stackTrace = captureLibraryStackTrace();
+    const apiZone: ApiZone = { apiName: stackTrace.apiName, frames: stackTrace.frames, isInternal, reported: false, userData: undefined, stepId: undefined };
 
     try {
       const result = await zones.run('apiZone', apiZone, async () => await func(apiZone));
-      csi?.onApiCallEnd(apiZone);
-      if (!isInternal)
-        logApiCall(logger, `<= ${apiName} succeeded`);
+      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(apiZone);
-      if (!isInternal)
-        logApiCall(logger, `<= ${apiName} failed`);
+      if (!isInternal) {
+        apiZone.error = e;
+        logApiCall(logger, `<= ${apiZone.apiName} failed`);
+        this._instrumentation.onApiCallEnd(apiZone);
+      }
       throw e;
     }
   }
@@ -248,7 +240,6 @@ type ApiZone = {
   frames: channels.StackFrame[];
   isInternal: boolean;
   reported: boolean;
-  csi: ClientInstrumentation | undefined;
   userData: any;
   stepId?: string;
   error?: Error;

diff --git a/packages/playwright-core/src/client/connection.ts b/packages/playwright-core/src/client/connection.ts
@@ -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() {

diff --git a/packages/playwright/src/index.ts b/packages/playwright/src/index.ts
@@ -18,7 +18,8 @@ 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';
@@ -260,8 +261,18 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
     const csiListener: ClientInstrumentationListener = {
       onApiCallBegin: (data: ApiCallData) => {
         const testInfo = currentTestInfo();
+        // 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: data.frames[0],
           category: 'pw:api',
@@ -440,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'];