chore: do not set metadata.error for expect failure results (#29310)

The metadata.error change was brought back in
https://github.com/microsoft/playwright/pull/29271and it broke java port
as we could have error and result set simulteniously. This PR moves the
logic to the trace recorder instead and keeps the protocol contract
clear that either error or result is present, but not both.

packages/playwright-core/src/server/dispatchers/dispatcher.ts

@@ -346,10 +346,12 @@ export class DispatcherConnection {
     }
 
     await sdkObject?.instrumentation.onBeforeCall(sdkObject, callMetadata);
+    const response: any = { id };
     try {
       const result = await dispatcher._handleCommand(callMetadata, method, validParams);
       const validator = findValidator(dispatcher._type, method, 'Result');
-      callMetadata.result = validator(result, '', { tChannelImpl: this._tChannelImplToWire.bind(this), binary: this._isLocal ? 'buffer' : 'toBase64' });
+      response.result = validator(result, '', { tChannelImpl: this._tChannelImplToWire.bind(this), binary: this._isLocal ? 'buffer' : 'toBase64' });
+      callMetadata.result = response.result;
     } catch (e) {
       if (isTargetClosedError(e) && sdkObject) {
         const reason = closeReason(sdkObject);
@@ -363,19 +365,16 @@ export class DispatcherConnection {
           rewriteErrorMessage(e, 'Target crashed ' + e.browserLogMessage());
         }
       }
-      callMetadata.error = serializeError(e);
+      response.error = serializeError(e);
+      // The command handler could have set error in the metada, do not reset it if there was no exception.
+      callMetadata.error = response.error;
     } finally {
       callMetadata.endTime = monotonicTime();
       await sdkObject?.instrumentation.onAfterCall(sdkObject, callMetadata);
     }
 
-    const response: any = { id };
-    if (callMetadata.result)
-      response.result = callMetadata.result;
-    if (callMetadata.error) {
-      response.error = callMetadata.error;
+    if (response.error)
       response.log = callMetadata.log;
-    }
     this.onmessage(response);
   }
 }

packages/playwright-core/src/server/dispatchers/frameDispatcher.ts

@@ -262,8 +262,6 @@ export class FrameDispatcher extends Dispatcher<Frame, channels.FrameChannel, Br
     const result = await this._frame.expect(metadata, params.selector, { ...params, expectedValue });
     if (result.received !== undefined)
       result.received = serializeResult(result.received);
-    if (result.matches === params.isNot)
-      metadata.error = { error: { name: 'Expect', message: 'Expect failed' } };
     return result;
   }
 }

packages/playwright-core/src/server/frames.ts

@@ -1372,6 +1372,14 @@ export class Frame extends SdkObject {
   }
 
   async expect(metadata: CallMetadata, selector: string, options: FrameExpectParams): Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }> {
+    const result = await this._expectImpl(metadata, selector, options);
+    // Library mode special case for the expect errors which are return values, not exceptions.
+    if (result.matches === options.isNot)
+      metadata.error = { error: { name: 'Expect', message: 'Expect failed' } };
+    return result;
+  }
+
+  private async _expectImpl(metadata: CallMetadata, selector: string, options: FrameExpectParams): Promise<{ matches: boolean, received?: any, log?: string[], timedOut?: boolean }> {
     let timeout = this._page._timeoutSettings.timeout(options);
     const start = timeout > 0 ? monotonicTime() : 0;
     const lastIntermediateResult: { received?: any, isSet: boolean } = { isSet: false };

tests/config/traceViewerFixtures.ts

@@ -39,6 +39,7 @@ class TraceViewerPage {
   callLines: Locator;
   consoleLines: Locator;
   logLines: Locator;
+  errorMessages: Locator;
   consoleLineMessages: Locator;
   consoleStacks: Locator;
   stackFrames: Locator;
@@ -51,6 +52,7 @@ class TraceViewerPage {
     this.logLines = page.getByTestId('log-list').locator('.list-view-entry');
     this.consoleLines = page.locator('.console-line');
     this.consoleLineMessages = page.locator('.console-line-message');
+    this.errorMessages = page.locator('.error-message');
     this.consoleStacks = page.locator('.console-stack');
     this.stackFrames = page.getByTestId('stack-trace-list').locator('.list-view-entry');
     this.networkRequests = page.getByTestId('network-list').locator('.list-view-entry');
@@ -75,6 +77,10 @@ class TraceViewerPage {
     await this.page.click(`.snapshot-tab .tabbed-pane-tab-label:has-text("${name}")`);
   }
 
+  async showErrorsTab() {
+    await this.page.click('text="Errors"');
+  }
+
   async showConsoleTab() {
     await this.page.click('text="Console"');
   }

tests/library/trace-viewer.spec.ts

@@ -686,6 +686,21 @@ test('should highlight target element in shadow dom', async ({ page, server, run
   await expect(frameExpect.locator('h1')).toHaveCSS('background-color', 'rgba(111, 168, 220, 0.498)');
 });
 
+test('should highlight expect failure', async ({ page, server, runAndTrace }) => {
+  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright-python/issues/2258' });
+  const traceViewer = await runAndTrace(async () => {
+    try {
+      await page.goto(server.EMPTY_PAGE);
+      await expect(page).toHaveTitle('foo', { timeout: 100 });
+    } catch (e) {
+    }
+  });
+
+  await expect(traceViewer.actionTitles.getByText('expect.toHaveTitle')).toHaveCSS('color', 'rgb(176, 16, 17)');
+  await traceViewer.showErrorsTab();
+  await expect(traceViewer.errorMessages.nth(0)).toHaveText('Expect failed');
+});
+
 test('should show action source', async ({ showTraceViewer }) => {
   const traceViewer = await showTraceViewer([traceFile]);
   await traceViewer.selectAction('locator.click');