From 074cc7d467102bbc750622052454ed26b5bb8d30 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 15 Jul 2024 01:08:51 -0700 Subject: [PATCH] Revert "feat(trace): record trace upon browser closure (#31563)" (#31677) This reverts commit bc27ca225e69995f238192426df4ccb3f940a50d. Considered too risky. --- .../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, 3 insertions(+), 182 deletions(-) diff --git a/packages/playwright-core/src/client/browser.ts b/packages/playwright-core/src/client/browser.ts index 0d95bf24d7..be47ddeb51 100644 --- a/packages/playwright-core/src/client/browser.ts +++ b/packages/playwright-core/src/client/browser.ts @@ -50,7 +50,6 @@ 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)); } @@ -139,14 +138,10 @@ export class Browser extends ChannelOwner implements ap async close(options: { reason?: string } = {}): Promise { this._closeReason = options.reason; try { - 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); + if (this._shouldCloseConnectionOnClose) this._connection.close(); - } else { + else await this._channel.close(options); - } await this._closedPromise; } catch (e) { if (isTargetClosedError(e)) @@ -155,13 +150,6 @@ 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 8faf24c84b..4ebe32595c 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 || this._type === 'JsonPipe' ? { apiName: undefined, csi: undefined, callCookie: undefined, frames: [], stepId: undefined } : 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) { diff --git a/packages/playwright-core/src/client/clientInstrumentation.ts b/packages/playwright-core/src/client/clientInstrumentation.ts index 0ade3295c2..6d90911acd 100644 --- a/packages/playwright-core/src/client/clientInstrumentation.ts +++ b/packages/playwright-core/src/client/clientInstrumentation.ts @@ -15,7 +15,6 @@ */ import type { StackFrame } from '@protocol/channels'; -import type { Browser } from './browser'; import type { BrowserContext } from './browserContext'; import type { APIRequestContext } from './fetch'; @@ -31,7 +30,6 @@ export interface ClientInstrumentation { runAfterCreateRequestContext(context: APIRequestContext): Promise; runBeforeCloseBrowserContext(context: BrowserContext): Promise; runBeforeCloseRequestContext(context: APIRequestContext): Promise; - runBeforeCloseBrowser(browser: Browser): Promise; } export interface ClientInstrumentationListener { @@ -43,7 +41,6 @@ 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 9ff716a537..32877078e4 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -607,10 +607,7 @@ 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 c37325b485..663b9be377 100644 --- a/packages/playwright-core/src/server/browser.ts +++ b/packages/playwright-core/src/server/browser.ts @@ -55,7 +55,6 @@ export type BrowserOptions = { export abstract class Browser extends SdkObject { static Events = { - BeforeClose: 'beforeClose', Disconnected: 'disconnected', }; @@ -67,7 +66,6 @@ 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'); @@ -152,18 +150,7 @@ 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 1f23518bcb..99ce5f961f 100644 --- a/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts +++ b/packages/playwright-core/src/server/dispatchers/browserDispatcher.ts @@ -34,15 +34,9 @@ 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(); @@ -61,10 +55,6 @@ export class BrowserDispatcher extends Dispatcher { metadata.potentiallyClosesScope = true; await this._object.close(params); @@ -109,7 +99,6 @@ 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(); @@ -133,10 +122,6 @@ 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 65185925be..2e55da594b 100644 --- a/packages/playwright/src/index.ts +++ b/packages/playwright/src/index.ts @@ -537,10 +537,6 @@ 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) @@ -763,10 +759,6 @@ 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 b8cccde6ef..ed48ef2b6c 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -1120,12 +1120,10 @@ 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; @@ -1136,11 +1134,7 @@ 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, }; @@ -1464,7 +1458,6 @@ 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 28df319e18..987261c173 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -934,8 +934,6 @@ Browser: commands: - beforeCloseFinished: - close: parameters: reason: string? @@ -1013,8 +1011,6 @@ Browser: events: - beforeClose: - close: ConsoleMessage: diff --git a/tests/playwright-test/playwright.connect.spec.ts b/tests/playwright-test/playwright.connect.spec.ts index 1a0b2a5bc5..083a01ac5e 100644 --- a/tests/playwright-test/playwright.connect.spec.ts +++ b/tests/playwright-test/playwright.connect.spec.ts @@ -15,7 +15,6 @@ */ import { test, expect } from './playwright-test-fixtures'; -import { parseTrace } from '../config/utils'; test('should work with connectOptions', async ({ runInlineTest }) => { const result = await runInlineTest({ @@ -168,48 +167,3 @@ 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 3a99524f28..01d4081af7 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/tests/playwright-test/playwright.trace.spec.ts @@ -1268,71 +1268,3 @@ 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' })); -});