From bc27ca225e69995f238192426df4ccb3f940a50d Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Sat, 6 Jul 2024 11:34:34 -0700 Subject: [PATCH] feat(trace): record trace upon browser closure (#31563) Retaining traces in the following scenarios: - browser crash; - manual `browser.close()`; - implicit `browser.close()` from the `browser` fixture upon test end. This does not affect the library, where `browser.close()` will not retain the trace and will close the browser as fast as possible. References #31541, #31535, #31537. --- .../playwright-core/src/client/browser.ts | 16 ++++- .../src/client/channelOwner.ts | 2 +- .../src/client/clientInstrumentation.ts | 3 + .../playwright-core/src/protocol/validator.ts | 3 + .../playwright-core/src/server/browser.ts | 13 ++++ .../server/dispatchers/browserDispatcher.ts | 15 ++++ packages/playwright/src/index.ts | 8 +++ packages/protocol/src/channels.ts | 7 ++ packages/protocol/src/protocol.yml | 4 ++ .../playwright.connect.spec.ts | 46 +++++++++++++ .../playwright-test/playwright.trace.spec.ts | 68 +++++++++++++++++++ 11 files changed, 182 insertions(+), 3 deletions(-) diff --git a/packages/playwright-core/src/client/browser.ts b/packages/playwright-core/src/client/browser.ts index be47ddeb51dd1..0d95bf24d753e 100644 --- a/packages/playwright-core/src/client/browser.ts +++ b/packages/playwright-core/src/client/browser.ts @@ -50,6 +50,7 @@ export class Browser extends ChannelOwner implements ap super(parent, type, guid, initializer); this._name = initializer.name; this._channel.on('close', () => this._didClose()); + this._channel.on('beforeClose', () => this._beforeClose()); this._closedPromise = new Promise(f => this.once(Events.Browser.Disconnected, f)); } @@ -138,10 +139,14 @@ export class Browser extends ChannelOwner implements ap async close(options: { reason?: string } = {}): Promise { this._closeReason = options.reason; try { - if (this._shouldCloseConnectionOnClose) + if (this._shouldCloseConnectionOnClose) { + // We cannot run beforeClose when remote disconnects, because there is no physical connection anymore. + // However, we can run it for an explicit browser.close() call. + await this._instrumentation.runBeforeCloseBrowser(this); this._connection.close(); - else + } else { await this._channel.close(options); + } await this._closedPromise; } catch (e) { if (isTargetClosedError(e)) @@ -150,6 +155,13 @@ export class Browser extends ChannelOwner implements ap } } + async _beforeClose() { + await this._wrapApiCall(async () => { + await this._instrumentation.runBeforeCloseBrowser(this); + await this._channel.beforeCloseFinished().catch(() => {}); + }, true); + } + _didClose() { this._isConnected = false; this.emit(Events.Browser.Disconnected, this); diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index 4ebe32595c6bb..8faf24c84bec3 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -143,7 +143,7 @@ export abstract class ChannelOwner { return await this._wrapApiCall(async apiZone => { - const { apiName, frames, csi, callCookie, stepId } = apiZone.reported ? { apiName: undefined, csi: undefined, callCookie: undefined, frames: [], stepId: undefined } : apiZone; + const { apiName, frames, csi, callCookie, stepId } = apiZone.reported || this._type === 'JsonPipe' ? { apiName: undefined, csi: undefined, callCookie: undefined, frames: [], stepId: undefined } : apiZone; apiZone.reported = true; let currentStepId = stepId; if (csi && apiName) { diff --git a/packages/playwright-core/src/client/clientInstrumentation.ts b/packages/playwright-core/src/client/clientInstrumentation.ts index 6d90911acdd15..0ade3295c2384 100644 --- a/packages/playwright-core/src/client/clientInstrumentation.ts +++ b/packages/playwright-core/src/client/clientInstrumentation.ts @@ -15,6 +15,7 @@ */ import type { StackFrame } from '@protocol/channels'; +import type { Browser } from './browser'; import type { BrowserContext } from './browserContext'; import type { APIRequestContext } from './fetch'; @@ -30,6 +31,7 @@ export interface ClientInstrumentation { runAfterCreateRequestContext(context: APIRequestContext): Promise; runBeforeCloseBrowserContext(context: BrowserContext): Promise; runBeforeCloseRequestContext(context: APIRequestContext): Promise; + runBeforeCloseBrowser(browser: Browser): Promise; } export interface ClientInstrumentationListener { @@ -41,6 +43,7 @@ export interface ClientInstrumentationListener { runAfterCreateRequestContext?(context: APIRequestContext): Promise; runBeforeCloseBrowserContext?(context: BrowserContext): Promise; runBeforeCloseRequestContext?(context: APIRequestContext): Promise; + runBeforeCloseBrowser?(browser: Browser): Promise; } export function createInstrumentation(): ClientInstrumentation { diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index 63679d199328a..3f52c2f623b6e 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -589,7 +589,10 @@ scheme.BrowserInitializer = tObject({ version: tString, name: tString, }); +scheme.BrowserBeforeCloseEvent = tOptional(tObject({})); scheme.BrowserCloseEvent = tOptional(tObject({})); +scheme.BrowserBeforeCloseFinishedParams = tOptional(tObject({})); +scheme.BrowserBeforeCloseFinishedResult = tOptional(tObject({})); scheme.BrowserCloseParams = tObject({ reason: tOptional(tString), }); diff --git a/packages/playwright-core/src/server/browser.ts b/packages/playwright-core/src/server/browser.ts index 15d8e3b792ae4..f8098572841d9 100644 --- a/packages/playwright-core/src/server/browser.ts +++ b/packages/playwright-core/src/server/browser.ts @@ -55,6 +55,7 @@ export type BrowserOptions = { export abstract class Browser extends SdkObject { static Events = { + BeforeClose: 'beforeClose', Disconnected: 'disconnected', }; @@ -66,6 +67,7 @@ export abstract class Browser extends SdkObject { private _contextForReuse: { context: BrowserContext, hash: string } | undefined; _closeReason: string | undefined; _isCollocatedWithServer: boolean = true; + private _needsBeforeCloseEvent = false; constructor(parent: SdkObject, options: BrowserOptions) { super(parent, 'browser'); @@ -142,7 +144,18 @@ export abstract class Browser extends SdkObject { return video?.artifact; } + setNeedsBeforeCloseEvent(needsBeforeCloseEvent: boolean) { + this._needsBeforeCloseEvent = needsBeforeCloseEvent; + } + _didClose() { + if (this._needsBeforeCloseEvent) + this.emit(Browser.Events.BeforeClose); + else + this.beforeCloseFinished(); + } + + beforeCloseFinished() { for (const context of this.contexts()) context._browserClosed(); if (this._defaultContext) diff --git a/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts b/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts index 99ce5f961f570..1f23518bcb137 100644 --- a/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts @@ -34,9 +34,15 @@ export class BrowserDispatcher extends Dispatcher this._dispatchEvent('beforeClose')); this.addObjectListener(Browser.Events.Disconnected, () => this._didClose()); } + override _onDispose() { + this._object.setNeedsBeforeCloseEvent(false); + } + _didClose() { this._dispatchEvent('close'); this._dispose(); @@ -55,6 +61,10 @@ export class BrowserDispatcher extends Dispatcher { metadata.potentiallyClosesScope = true; await this._object.close(params); @@ -99,6 +109,7 @@ export class ConnectedBrowserDispatcher extends Dispatcher this.emit('beforeClose')); // When we have a remotely-connected browser, each client gets a fresh Selector instance, // so that two clients do not interfere between each other. this.selectors = new Selectors(); @@ -122,6 +133,10 @@ export class ConnectedBrowserDispatcher extends Dispatcher { // Client should not send us Browser.close. } diff --git a/packages/playwright/src/index.ts b/packages/playwright/src/index.ts index 79184cc1354a7..01ae4650978f3 100644 --- a/packages/playwright/src/index.ts +++ b/packages/playwright/src/index.ts @@ -511,6 +511,10 @@ class ArtifactsRecorder { await this._stopTracing(tracing); } + async willCloseBrowser(browser: Browser) { + await Promise.all(browser.contexts().map(context => this._stopTracing(context.tracing))); + } + async didFinishTestFunction() { const captureScreenshots = this._screenshotMode === 'on' || (this._screenshotMode === 'only-on-failure' && this._testInfo._isFailure()); if (captureScreenshots) @@ -733,6 +737,10 @@ class InstrumentationConnector implements TestLifecycleInstrumentation, ClientIn async runBeforeCloseRequestContext(context: APIRequestContext) { await this._artifactsRecorder?.willCloseRequestContext(context); } + + async runBeforeCloseBrowser(browser: Browser) { + await this._artifactsRecorder?.willCloseBrowser(browser); + } } const connector = new InstrumentationConnector(); diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index a1ae5a13e0f52..e54e89ddcf8c6 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -1084,10 +1084,12 @@ export type BrowserInitializer = { name: string, }; export interface BrowserEventTarget { + on(event: 'beforeClose', callback: (params: BrowserBeforeCloseEvent) => void): this; on(event: 'close', callback: (params: BrowserCloseEvent) => void): this; } export interface BrowserChannel extends BrowserEventTarget, Channel { _type_Browser: boolean; + beforeCloseFinished(params?: BrowserBeforeCloseFinishedParams, metadata?: CallMetadata): Promise; close(params: BrowserCloseParams, metadata?: CallMetadata): Promise; killForTests(params?: BrowserKillForTestsParams, metadata?: CallMetadata): Promise; defaultUserAgentForTest(params?: BrowserDefaultUserAgentForTestParams, metadata?: CallMetadata): Promise; @@ -1098,7 +1100,11 @@ export interface BrowserChannel extends BrowserEventTarget, Channel { startTracing(params: BrowserStartTracingParams, metadata?: CallMetadata): Promise; stopTracing(params?: BrowserStopTracingParams, metadata?: CallMetadata): Promise; } +export type BrowserBeforeCloseEvent = {}; export type BrowserCloseEvent = {}; +export type BrowserBeforeCloseFinishedParams = {}; +export type BrowserBeforeCloseFinishedOptions = {}; +export type BrowserBeforeCloseFinishedResult = void; export type BrowserCloseParams = { reason?: string, }; @@ -1386,6 +1392,7 @@ export type BrowserStopTracingResult = { }; export interface BrowserEvents { + 'beforeClose': BrowserBeforeCloseEvent; 'close': BrowserCloseEvent; } diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index 54f356102b1df..6b754c3619192 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -904,6 +904,8 @@ Browser: commands: + beforeCloseFinished: + close: parameters: reason: string? @@ -981,6 +983,8 @@ Browser: events: + beforeClose: + close: ConsoleMessage: diff --git a/tests/playwright-test/playwright.connect.spec.ts b/tests/playwright-test/playwright.connect.spec.ts index 083a01ac5e87a..1a0b2a5bc5d4e 100644 --- a/tests/playwright-test/playwright.connect.spec.ts +++ b/tests/playwright-test/playwright.connect.spec.ts @@ -15,6 +15,7 @@ */ import { test, expect } from './playwright-test-fixtures'; +import { parseTrace } from '../config/utils'; test('should work with connectOptions', async ({ runInlineTest }) => { const result = await runInlineTest({ @@ -167,3 +168,48 @@ test('should print debug log when failed to connect', async ({ runInlineTest }) expect(result.output).toContain('b-debug-log-string'); expect(result.results[0].attachments).toEqual([]); }); + +test('should save trace when remote browser is closed', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'playwright.config.js': ` + module.exports = { + globalSetup: './global-setup', + use: { + trace: 'on', + connectOptions: { wsEndpoint: process.env.CONNECT_WS_ENDPOINT }, + }, + }; + `, + 'global-setup.ts': ` + import { chromium } from '@playwright/test'; + module.exports = async () => { + const server = await chromium.launchServer(); + process.env.CONNECT_WS_ENDPOINT = server.wsEndpoint(); + return () => server.close(); + }; + `, + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('pass', async ({ browser }) => { + const page = await browser.newPage(); + await page.setContent(''); + await browser.close(); + }); + `, + }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(1); + + const tracePath = test.info().outputPath('test-results', 'a-pass', 'trace.zip'); + const trace = await parseTrace(tracePath); + expect(trace.actionTree).toEqual([ + 'Before Hooks', + ' fixture: browser', + ' browserType.connect', + 'browser.newPage', + 'page.setContent', + 'After Hooks', + ]); + // Check console events to make sure that library trace is recorded. + expect(trace.events).toContainEqual(expect.objectContaining({ type: 'console', text: 'from the page' })); +}); diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index 13af444edffb1..22107f74b248d 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/tests/playwright-test/playwright.trace.spec.ts @@ -1235,3 +1235,71 @@ test('should take a screenshot-on-failure in workerStorageState', async ({ runIn expect(result.failed).toBe(1); expect(fs.existsSync(test.info().outputPath('test-results', 'a-fail', 'test-failed-1.png'))).toBeTruthy(); }); + +test('should record trace upon implicit browser.close in a failed test', async ({ runInlineTest }) => { + test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/31541' }); + + const result = await runInlineTest({ + 'a.spec.ts': ` + import { test, expect } from '@playwright/test'; + test('fail', async ({ browser }) => { + const page = await browser.newPage(); + await page.setContent(''); + expect(1).toBe(2); + }); + `, + }, { trace: 'on' }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + + const tracePath = test.info().outputPath('test-results', 'a-fail', 'trace.zip'); + const trace = await parseTrace(tracePath); + expect(trace.actionTree).toEqual([ + 'Before Hooks', + ' fixture: browser', + ' browserType.launch', + 'browser.newPage', + 'page.setContent', + 'expect.toBe', + 'After Hooks', + 'Worker Cleanup', + ' fixture: browser', + ]); + // Check console events to make sure that library trace is recorded. + expect(trace.events).toContainEqual(expect.objectContaining({ type: 'console', text: 'from the page' })); +}); + +test('should record trace upon browser crash', async ({ runInlineTest }) => { + test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/31537' }); + + const result = await runInlineTest({ + 'a.spec.ts': ` + import { test, expect } from '@playwright/test'; + test('fail', async ({ browser }) => { + const page = await browser.newPage(); + await page.setContent(''); + await (browser as any)._channel.killForTests(); + await page.goto('data:text/html,
will not load
'); + }); + `, + }, { trace: 'on' }); + expect(result.exitCode).toBe(1); + expect(result.failed).toBe(1); + + const tracePath = test.info().outputPath('test-results', 'a-fail', 'trace.zip'); + const trace = await parseTrace(tracePath); + expect(trace.actionTree).toEqual([ + 'Before Hooks', + ' fixture: browser', + ' browserType.launch', + 'browser.newPage', + 'page.setContent', + 'proxy.killForTests', + 'page.goto', + 'After Hooks', + 'Worker Cleanup', + ' fixture: browser', + ]); + // Check console events to make sure that library trace is recorded. + expect(trace.events).toContainEqual(expect.objectContaining({ type: 'console', text: 'from the page' })); +});