From 79e379fc11ee7f07f6538f0f0aaffe6175fa5033 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Fri, 2 Feb 2024 16:41:08 -0800 Subject: [PATCH] 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. --- .../src/server/dispatchers/dispatcher.ts | 15 +++++++-------- .../src/server/dispatchers/frameDispatcher.ts | 2 -- packages/playwright-core/src/server/frames.ts | 8 ++++++++ tests/config/traceViewerFixtures.ts | 6 ++++++ tests/library/trace-viewer.spec.ts | 15 +++++++++++++++ 5 files changed, 36 insertions(+), 10 deletions(-) diff --git a/packages/playwright-core/src/server/dispatchers/dispatcher.ts b/packages/playwright-core/src/server/dispatchers/dispatcher.ts index 6fffb9d83b..9881a9eeea 100644 --- a/packages/playwright-core/src/server/dispatchers/dispatcher.ts +++ b/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); } } diff --git a/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts b/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts index 6782926e7b..b335f27ea0 100644 --- a/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/frameDispatcher.ts @@ -262,8 +262,6 @@ export class FrameDispatcher extends Dispatcher { + 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 }; diff --git a/tests/config/traceViewerFixtures.ts b/tests/config/traceViewerFixtures.ts index 6c43f1a4d3..995233e4d6 100644 --- a/tests/config/traceViewerFixtures.ts +++ b/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"'); } diff --git a/tests/library/trace-viewer.spec.ts b/tests/library/trace-viewer.spec.ts index ff8f049921..3bd3c11288 100644 --- a/tests/library/trace-viewer.spec.ts +++ b/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');