From 762e954599e1e45caebc21921d26c58b715176a9 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Mon, 16 Sep 2024 15:43:42 +0200 Subject: [PATCH 01/13] devops: update GitHub Actions (#32634) Fixes: image For future reference: `pip install github-actions-cli && github-actions-cli update-actions --update` --- .github/workflows/infra.yml | 4 ++-- .github/workflows/tests_others.yml | 2 +- .github/workflows/tests_service.yml | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/infra.yml b/.github/workflows/infra.yml index 3580ecc97a..f33c8535f0 100644 --- a/.github/workflows/infra.yml +++ b/.github/workflows/infra.yml @@ -44,10 +44,10 @@ jobs: - uses: actions/setup-node@v4 with: node-version: 18 - - uses: actions/setup-python@v4 + - uses: actions/setup-python@v5 with: python-version: '3.11' - - uses: actions/setup-dotnet@v3 + - uses: actions/setup-dotnet@v4 with: dotnet-version: 8.0.x - run: npm ci diff --git a/.github/workflows/tests_others.yml b/.github/workflows/tests_others.yml index 783f3fe2ff..434c5aa9d3 100644 --- a/.github/workflows/tests_others.yml +++ b/.github/workflows/tests_others.yml @@ -66,7 +66,7 @@ jobs: contents: read # This is required for actions/checkout to succeed steps: - uses: actions/checkout@v4 - - uses: actions/setup-dotnet@v3 + - uses: actions/setup-dotnet@v4 with: dotnet-version: '8.0.x' - run: dotnet build diff --git a/.github/workflows/tests_service.yml b/.github/workflows/tests_service.yml index 2788b0cf08..2d68740006 100644 --- a/.github/workflows/tests_service.yml +++ b/.github/workflows/tests_service.yml @@ -34,7 +34,7 @@ jobs: PLAYWRIGHT_SERVICE_RUN_ID: ${{ github.run_id }}-${{ github.run_attempt }}-${{ github.sha }} - name: Upload blob report to GitHub if: ${{ !cancelled() }} - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: all-blob-reports path: blob-report From 71c43693acf35a9f49c9282ecae4c61557a7e160 Mon Sep 17 00:00:00 2001 From: Anthony Roberts Date: Tue, 17 Sep 2024 00:57:11 +1000 Subject: [PATCH 02/13] feat(reporter): add copy button for annotations (#31790) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a copy-to-clipboard button for each annotation so that text can be copied easily. This re-uses the existing `CopyToClipboard` component and adds a `small` variant that can be used inline. The icon size and colour have been chosen to avoid being overwhelming when used inline. Related to #30141 I opted not to introduce the hover behaviour from #30749 as it's less discoverable, but can understand why that might be favourable. Certainly open to suggestions 😄 Screenshot 2024-07-22 at 3 23 53 PM --- .../html-reporter/src/copyToClipboard.css | 18 +++++++++++- .../html-reporter/src/copyToClipboard.tsx | 29 ++++++++++++++++--- .../html-reporter/src/testCaseView.spec.tsx | 13 +++++++++ packages/html-reporter/src/testCaseView.tsx | 3 +- 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/packages/html-reporter/src/copyToClipboard.css b/packages/html-reporter/src/copyToClipboard.css index 5790b626c0..818100fff7 100644 --- a/packages/html-reporter/src/copyToClipboard.css +++ b/packages/html-reporter/src/copyToClipboard.css @@ -20,15 +20,31 @@ width: 24px; border: none; outline: none; - color: var(--color-fg-default); + color: var(--color-fg-muted); background: transparent; padding: 4px; cursor: pointer; display: inline-flex; align-items: center; + justify-content: center; border-radius: 4px; } +.copy-icon svg { + margin: 0; +} + .copy-icon:not(:disabled):hover { background-color: var(--color-border-default); } + +.copy-button-container { + visibility: hidden; + display: inline-flex; + margin-left: 8px; + vertical-align: bottom; +} + +.copy-value-container:hover .copy-button-container { + visibility: visible; +} diff --git a/packages/html-reporter/src/copyToClipboard.tsx b/packages/html-reporter/src/copyToClipboard.tsx index a24015a671..17b1dfbf95 100644 --- a/packages/html-reporter/src/copyToClipboard.tsx +++ b/packages/html-reporter/src/copyToClipboard.tsx @@ -18,9 +18,14 @@ import * as React from 'react'; import * as icons from './icons'; import './copyToClipboard.css'; -export const CopyToClipboard: React.FunctionComponent<{ - value: string, -}> = ({ value }) => { +type CopyToClipboardProps = { + value: string; +}; + +/** + * A copy to clipboard button. + */ +export const CopyToClipboard: React.FunctionComponent = ({ value }) => { type IconType = 'copy' | 'check' | 'cross'; const [icon, setIcon] = React.useState('copy'); const handleCopy = React.useCallback(() => { @@ -34,5 +39,21 @@ export const CopyToClipboard: React.FunctionComponent<{ }); }, [value]); const iconElement = icon === 'check' ? icons.check() : icon === 'cross' ? icons.cross() : icons.copy(); - return ; + return ; +}; + +type CopyToClipboardContainerProps = CopyToClipboardProps & { + children: React.ReactNode +}; + +/** + * Container for displaying a copy to clipboard button alongside children. + */ +export const CopyToClipboardContainer: React.FunctionComponent = ({ children, value }) => { + return + {children} + + + + ; }; diff --git a/packages/html-reporter/src/testCaseView.spec.tsx b/packages/html-reporter/src/testCaseView.spec.tsx index afe06cebcb..72552f5184 100644 --- a/packages/html-reporter/src/testCaseView.spec.tsx +++ b/packages/html-reporter/src/testCaseView.spec.tsx @@ -76,6 +76,19 @@ test('should render test case', async ({ mount }) => { await expect(component.getByText('My test')).toBeVisible(); }); +test('should render copy buttons for annotations', async ({ mount, page, context }) => { + await context.grantPermissions(['clipboard-read', 'clipboard-write']); + + const component = await mount(); + await expect(component.getByText('Annotation text', { exact: false }).first()).toBeVisible(); + component.getByText('Annotation text', { exact: false }).first().hover(); + await expect(component.getByLabel('Copy to clipboard').first()).toBeVisible(); + await component.getByLabel('Copy to clipboard').first().click(); + const handle = await page.evaluateHandle(() => navigator.clipboard.readText()); + const clipboardContent = await handle.jsonValue(); + expect(clipboardContent).toBe('Annotation text'); +}); + const annotationLinkRenderingTestCase: TestCase = { testId: 'testid', title: 'My test', diff --git a/packages/html-reporter/src/testCaseView.tsx b/packages/html-reporter/src/testCaseView.tsx index 5bed3c8309..1fe4f42ed3 100644 --- a/packages/html-reporter/src/testCaseView.tsx +++ b/packages/html-reporter/src/testCaseView.tsx @@ -26,6 +26,7 @@ import { TestResultView } from './testResultView'; import { linkifyText } from '@web/renderUtils'; import { hashStringToInt, msToString } from './utils'; import { clsx } from '@web/uiUtils'; +import { CopyToClipboardContainer } from './copyToClipboard'; export const TestCaseView: React.FC<{ projectNames: string[], @@ -73,7 +74,7 @@ function TestCaseAnnotationView({ annotation: { type, description } }: { annotat return (
{type} - {description && : {linkifyText(description)}} + {description && : {linkifyText(description)}}
); } From feac957475a5743c38512f429b2bee58120710a8 Mon Sep 17 00:00:00 2001 From: Playwright Service <89237858+playwrightmachine@users.noreply.github.com> Date: Mon, 16 Sep 2024 08:18:01 -0700 Subject: [PATCH 03/13] feat(webkit): roll to r2077 (#32636) Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- packages/playwright-core/browsers.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/playwright-core/browsers.json b/packages/playwright-core/browsers.json index de51a1643c..dc91235659 100644 --- a/packages/playwright-core/browsers.json +++ b/packages/playwright-core/browsers.json @@ -27,7 +27,7 @@ }, { "name": "webkit", - "revision": "2075", + "revision": "2077", "installByDefault": true, "revisionOverrides": { "mac10.14": "1446", From b335b00a8622db4d2599741f2f013c83af8cb926 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Mon, 16 Sep 2024 17:30:14 +0200 Subject: [PATCH 04/13] docs: add reference to locator strictness if or resolves to multiple elements (#32633) --- docs/src/api/class-locator.md | 4 +++- packages/playwright-core/types/types.d.ts | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/src/api/class-locator.md b/docs/src/api/class-locator.md index 4df0035098..88658b5494 100644 --- a/docs/src/api/class-locator.md +++ b/docs/src/api/class-locator.md @@ -1654,7 +1654,9 @@ var banana = await page.GetByRole(AriaRole.Listitem).Nth(2); - alias-python: or_ - returns: <[Locator]> -Creates a locator that matches either of the two locators. +Creates a locator matching all elements that match one or both of the two locators. + +Note that when both locators match something, the resulting locator will have multiple matches and violate [locator strictness](../locators.md#strictness) guidelines. **Usage** diff --git a/packages/playwright-core/types/types.d.ts b/packages/playwright-core/types/types.d.ts index e88e1b473b..67bb51b192 100644 --- a/packages/playwright-core/types/types.d.ts +++ b/packages/playwright-core/types/types.d.ts @@ -13060,7 +13060,10 @@ export interface Locator { nth(index: number): Locator; /** - * Creates a locator that matches either of the two locators. + * Creates a locator matching all elements that match one or both of the two locators. + * + * Note that when both locators match something, the resulting locator will have multiple matches and violate + * [locator strictness](https://playwright.dev/docs/locators#strictness) guidelines. * * **Usage** * From 21d162c945e81661278b08f508c0af9fcc08e78c Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Mon, 16 Sep 2024 17:57:33 +0200 Subject: [PATCH 05/13] feat(client-certificates): add support for proxies (#32611) Fixes https://github.com/microsoft/playwright/issues/32370 --- docs/src/api/params.md | 4 - .../playwright-core/src/server/browser.ts | 9 +- .../src/server/browserContext.ts | 11 +- .../playwright-core/src/server/browserType.ts | 17 +-- packages/playwright-core/src/server/fetch.ts | 38 +++---- .../socksClientCertificatesInterceptor.ts | 14 ++- packages/playwright-core/types/types.d.ts | 8 -- packages/playwright/types/test.d.ts | 2 - tests/config/proxy.ts | 51 ++++++++- tests/library/client-certificates.spec.ts | 101 ++++++++++++++++-- tests/library/proxy.spec.ts | 44 ++------ 11 files changed, 196 insertions(+), 103 deletions(-) diff --git a/docs/src/api/params.md b/docs/src/api/params.md index 608855ab2a..de930ee97e 100644 --- a/docs/src/api/params.md +++ b/docs/src/api/params.md @@ -558,10 +558,6 @@ TLS Client Authentication allows the server to request a client certificate and An array of client certificates to be used. Each certificate object must have either both `certPath` and `keyPath`, a single `pfxPath`, or their corresponding direct value equivalents (`cert` and `key`, or `pfx`). Optionally, `passphrase` property should be provided if the certificate is encrypted. The `origin` property should be provided with an exact match to the request origin that the certificate is valid for. -:::note -Using Client Certificates in combination with Proxy Servers is not supported. -::: - :::note When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it work by replacing `localhost` with `local.playwright`. ::: diff --git a/packages/playwright-core/src/server/browser.ts b/packages/playwright-core/src/server/browser.ts index 04a23e7eac..252443bc44 100644 --- a/packages/playwright-core/src/server/browser.ts +++ b/packages/playwright-core/src/server/browser.ts @@ -16,7 +16,7 @@ import type * as types from './types'; import type * as channels from '@protocol/channels'; -import { BrowserContext, createClientCertificatesProxyIfNeeded, validateBrowserContextOptions } from './browserContext'; +import { BrowserContext, validateBrowserContextOptions } from './browserContext'; import { Page } from './page'; import { Download } from './download'; import type { ProxySettings } from './types'; @@ -25,6 +25,7 @@ import type { RecentLogsCollector } from '../utils/debugLogger'; import type { CallMetadata } from './instrumentation'; import { SdkObject } from './instrumentation'; import { Artifact } from './artifact'; +import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor'; export interface BrowserProcess { onclose?: ((exitCode: number | null, signal: string | null) => void); @@ -82,8 +83,10 @@ export abstract class Browser extends SdkObject { async newContext(metadata: CallMetadata, options: types.BrowserContextOptions): Promise { validateBrowserContextOptions(options, this.options); - const clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(options, this.options); - if (clientCertificatesProxy) { + let clientCertificatesProxy: ClientCertificatesProxy | undefined; + if (options.clientCertificates?.length) { + clientCertificatesProxy = new ClientCertificatesProxy(options); + options = { ...options }; options.proxyOverride = await clientCertificatesProxy.listen(); options.internalIgnoreHTTPSErrors = true; } diff --git a/packages/playwright-core/src/server/browserContext.ts b/packages/playwright-core/src/server/browserContext.ts index e1166f3e97..499356ca49 100644 --- a/packages/playwright-core/src/server/browserContext.ts +++ b/packages/playwright-core/src/server/browserContext.ts @@ -42,7 +42,7 @@ import * as consoleApiSource from '../generated/consoleApiSource'; import { BrowserContextAPIRequestContext } from './fetch'; import type { Artifact } from './artifact'; import { Clock } from './clock'; -import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor'; +import type { ClientCertificatesProxy } from './socksClientCertificatesInterceptor'; import { RecorderApp } from './recorder/recorderApp'; export abstract class BrowserContext extends SdkObject { @@ -659,15 +659,6 @@ export function assertBrowserContextIsNotOwned(context: BrowserContext) { } } -export async function createClientCertificatesProxyIfNeeded(options: types.BrowserContextOptions, browserOptions?: BrowserOptions) { - if (!options.clientCertificates?.length) - return; - if ((options.proxy?.server && options.proxy?.server !== 'per-context') || (browserOptions?.proxy?.server && browserOptions?.proxy?.server !== 'http://per-context')) - throw new Error('Cannot specify both proxy and clientCertificates'); - verifyClientCertificates(options.clientCertificates); - return new ClientCertificatesProxy(options); -} - export function validateBrowserContextOptions(options: types.BrowserContextOptions, browserOptions: BrowserOptions) { if (options.noDefaultViewport && options.deviceScaleFactor !== undefined) throw new Error(`"deviceScaleFactor" option is not supported with null "viewport"`); diff --git a/packages/playwright-core/src/server/browserType.ts b/packages/playwright-core/src/server/browserType.ts index 286ec27c29..abc15a20f4 100644 --- a/packages/playwright-core/src/server/browserType.ts +++ b/packages/playwright-core/src/server/browserType.ts @@ -18,7 +18,7 @@ import fs from 'fs'; import * as os from 'os'; import path from 'path'; import type { BrowserContext } from './browserContext'; -import { createClientCertificatesProxyIfNeeded, normalizeProxySettings, validateBrowserContextOptions } from './browserContext'; +import { normalizeProxySettings, validateBrowserContextOptions } from './browserContext'; import type { BrowserName } from './registry'; import { registry } from './registry'; import type { ConnectionTransport } from './transport'; @@ -39,6 +39,7 @@ import { RecentLogsCollector } from '../utils/debugLogger'; import type { CallMetadata } from './instrumentation'; import { SdkObject } from './instrumentation'; import { type ProtocolError, isProtocolError } from './protocolError'; +import { ClientCertificatesProxy } from './socksClientCertificatesInterceptor'; export const kNoXServerRunningError = 'Looks like you launched a headed browser without having a XServer running.\n' + 'Set either \'headless: true\' or use \'xvfb-run \' before running Playwright.\n\n<3 Playwright Team'; @@ -92,19 +93,23 @@ export abstract class BrowserType extends SdkObject { return browser; } - async launchPersistentContext(metadata: CallMetadata, userDataDir: string, persistentContextOptions: channels.BrowserTypeLaunchPersistentContextOptions & { useWebSocket?: boolean }): Promise { - const launchOptions = this._validateLaunchOptions(persistentContextOptions); + async launchPersistentContext(metadata: CallMetadata, userDataDir: string, options: channels.BrowserTypeLaunchPersistentContextOptions & { useWebSocket?: boolean, internalIgnoreHTTPSErrors?: boolean }): Promise { + const launchOptions = this._validateLaunchOptions(options); if (this._useBidi) launchOptions.useWebSocket = true; const controller = new ProgressController(metadata, this); controller.setLogName('browser'); const browser = await controller.run(async progress => { // Note: Any initial TLS requests will fail since we rely on the Page/Frames initialize which sets ignoreHTTPSErrors. - const clientCertificatesProxy = await createClientCertificatesProxyIfNeeded(persistentContextOptions); - if (clientCertificatesProxy) + let clientCertificatesProxy: ClientCertificatesProxy | undefined; + if (options.clientCertificates?.length) { + clientCertificatesProxy = new ClientCertificatesProxy(options); launchOptions.proxyOverride = await clientCertificatesProxy?.listen(); + options = { ...options }; + options.internalIgnoreHTTPSErrors = true; + } progress.cleanupWhenAborted(() => clientCertificatesProxy?.close()); - const browser = await this._innerLaunchWithRetries(progress, launchOptions, persistentContextOptions, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); }); + const browser = await this._innerLaunchWithRetries(progress, launchOptions, options, helper.debugProtocolLogger(), userDataDir).catch(e => { throw this._rewriteStartupLog(e); }); browser._defaultContext!._clientCertificatesProxy = clientCertificatesProxy; return browser; }, TimeoutSettings.launchTimeout(launchOptions)); diff --git a/packages/playwright-core/src/server/fetch.ts b/packages/playwright-core/src/server/fetch.ts index 01c6c397ab..fc4e2c027d 100644 --- a/packages/playwright-core/src/server/fetch.ts +++ b/packages/playwright-core/src/server/fetch.ts @@ -168,23 +168,11 @@ export abstract class APIRequestContext extends SdkObject { const method = params.method?.toUpperCase() || 'GET'; const proxy = defaults.proxy; let agent; - // When `clientCertificates` is present, we set the `proxy` property to our own socks proxy - // for the browser to use. However, we don't need it here, because we already respect - // `clientCertificates` when fetching from Node.js. - if (proxy && !defaults.clientCertificates?.length && proxy.server !== 'per-context' && !shouldBypassProxy(requestUrl, proxy.bypass)) { - const proxyOpts = url.parse(proxy.server); - if (proxyOpts.protocol?.startsWith('socks')) { - agent = new SocksProxyAgent({ - host: proxyOpts.hostname, - port: proxyOpts.port || undefined, - }); - } else { - if (proxy.username) - proxyOpts.auth = `${proxy.username}:${proxy.password || ''}`; - // TODO: We should use HttpProxyAgent conditional on proxyOpts.protocol instead of always using CONNECT method. - agent = new HttpsProxyAgent(proxyOpts); - } - } + // We skip 'per-context' in order to not break existing users. 'per-context' was previously used to + // workaround an upstream Chromium bug. Can be removed in the future. + if (proxy && proxy.server !== 'per-context' && !shouldBypassProxy(requestUrl, proxy.bypass)) + agent = createProxyAgent(proxy); + const timeout = defaults.timeoutSettings.timeout(params); const deadline = timeout && (monotonicTime() + timeout); @@ -577,8 +565,6 @@ export class GlobalAPIRequestContext extends APIRequestContext { if (!/^\w+:\/\//.test(url)) url = 'http://' + url; proxy.server = url; - if (options.clientCertificates) - throw new Error('Cannot specify both proxy and clientCertificates'); } if (options.storageState) { this._origins = options.storageState.origins; @@ -629,6 +615,20 @@ export class GlobalAPIRequestContext extends APIRequestContext { } } +export function createProxyAgent(proxy: types.ProxySettings) { + const proxyOpts = url.parse(proxy.server); + if (proxyOpts.protocol?.startsWith('socks')) { + return new SocksProxyAgent({ + host: proxyOpts.hostname, + port: proxyOpts.port || undefined, + }); + } + if (proxy.username) + proxyOpts.auth = `${proxy.username}:${proxy.password || ''}`; + // TODO: We should use HttpProxyAgent conditional on proxyOpts.protocol instead of always using CONNECT method. + return new HttpsProxyAgent(proxyOpts); +} + function toHeadersArray(rawHeaders: string[]): types.HeadersArray { const result: types.HeadersArray = []; for (let i = 0; i < rawHeaders.length; i += 2) diff --git a/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts b/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts index 6d4b334dba..08aaa4f25a 100644 --- a/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts +++ b/packages/playwright-core/src/server/socksClientCertificatesInterceptor.ts @@ -25,6 +25,9 @@ import type { SocksSocketClosedPayload, SocksSocketDataPayload, SocksSocketReque import { SocksProxy } from '../common/socksProxy'; import type * as types from './types'; import { debugLogger } from '../utils/debugLogger'; +import { createProxyAgent } from './fetch'; +import { EventEmitter } from 'events'; +import { verifyClientCertificates } from './browserContext'; let dummyServerTlsOptions: tls.TlsOptions | undefined = undefined; function loadDummyServerCertsIfNeeded() { @@ -94,7 +97,11 @@ class SocksProxyConnection { } async connect() { - this.target = await createSocket(rewriteToLocalhostIfNeeded(this.host), this.port); + if (this.socksProxy.proxyAgentFromOptions) + this.target = await this.socksProxy.proxyAgentFromOptions.callback(new EventEmitter() as any, { host: rewriteToLocalhostIfNeeded(this.host), port: this.port, secureEndpoint: false }); + else + this.target = await createSocket(rewriteToLocalhostIfNeeded(this.host), this.port); + this.target.once('close', this._targetCloseEventListener); this.target.once('error', error => this.socksProxy._socksProxy.sendSocketError({ uid: this.uid, error: error.message })); if (this._closed) { @@ -233,12 +240,15 @@ export class ClientCertificatesProxy { ignoreHTTPSErrors: boolean | undefined; secureContextMap: Map = new Map(); alpnCache: ALPNCache; + proxyAgentFromOptions: ReturnType | undefined; constructor( - contextOptions: Pick + contextOptions: Pick ) { + verifyClientCertificates(contextOptions.clientCertificates); this.alpnCache = new ALPNCache(); this.ignoreHTTPSErrors = contextOptions.ignoreHTTPSErrors; + this.proxyAgentFromOptions = contextOptions.proxy ? createProxyAgent(contextOptions.proxy) : undefined; this._initSecureContexts(contextOptions.clientCertificates); this._socksProxy = new SocksProxy(); this._socksProxy.setPattern('*'); diff --git a/packages/playwright-core/types/types.d.ts b/packages/playwright-core/types/types.d.ts index 67bb51b192..f770fc70dc 100644 --- a/packages/playwright-core/types/types.d.ts +++ b/packages/playwright-core/types/types.d.ts @@ -9202,8 +9202,6 @@ export interface Browser { * `passphrase` property should be provided if the certificate is encrypted. The `origin` property should be provided * with an exact match to the request origin that the certificate is valid for. * - * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. - * * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it * work by replacing `localhost` with `local.playwright`. */ @@ -13924,8 +13922,6 @@ export interface BrowserType { * `passphrase` property should be provided if the certificate is encrypted. The `origin` property should be provided * with an exact match to the request origin that the certificate is valid for. * - * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. - * * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it * work by replacing `localhost` with `local.playwright`. */ @@ -16350,8 +16346,6 @@ export interface APIRequest { * `passphrase` property should be provided if the certificate is encrypted. The `origin` property should be provided * with an exact match to the request origin that the certificate is valid for. * - * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. - * * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it * work by replacing `localhost` with `local.playwright`. */ @@ -20712,8 +20706,6 @@ export interface BrowserContextOptions { * `passphrase` property should be provided if the certificate is encrypted. The `origin` property should be provided * with an exact match to the request origin that the certificate is valid for. * - * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. - * * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it * work by replacing `localhost` with `local.playwright`. */ diff --git a/packages/playwright/types/test.d.ts b/packages/playwright/types/test.d.ts index 73cc415a87..e17a43843c 100644 --- a/packages/playwright/types/test.d.ts +++ b/packages/playwright/types/test.d.ts @@ -5211,8 +5211,6 @@ export interface PlaywrightTestOptions { * `passphrase` property should be provided if the certificate is encrypted. The `origin` property should be provided * with an exact match to the request origin that the certificate is valid for. * - * **NOTE** Using Client Certificates in combination with Proxy Servers is not supported. - * * **NOTE** When using WebKit on macOS, accessing `localhost` will not pick up client certificates. You can make it * work by replacing `localhost` with `local.playwright`. * diff --git a/tests/config/proxy.ts b/tests/config/proxy.ts index 08546c9283..dc2d51b3ec 100644 --- a/tests/config/proxy.ts +++ b/tests/config/proxy.ts @@ -15,9 +15,11 @@ */ import type { IncomingMessage } from 'http'; -import type { Socket } from 'net'; import type { ProxyServer } from '../third_party/proxy'; import { createProxy } from '../third_party/proxy'; +import net from 'net'; +import type { SocksSocketClosedPayload, SocksSocketDataPayload, SocksSocketRequestedPayload } from '../../packages/playwright-core/src/common/socksProxy'; +import { SocksProxy } from '../../packages/playwright-core/lib/common/socksProxy'; export class TestProxy { readonly PORT: number; @@ -27,7 +29,7 @@ export class TestProxy { requestUrls: string[] = []; private readonly _server: ProxyServer; - private readonly _sockets = new Set(); + private readonly _sockets = new Set(); private _handlers: { event: string, handler: (...args: any[]) => void }[] = []; static async create(port: number): Promise { @@ -90,7 +92,7 @@ export class TestProxy { this._server.prependListener(event, handler); } - private _onSocket(socket: Socket) { + private _onSocket(socket: net.Socket) { this._sockets.add(socket); // ECONNRESET and HPE_INVALID_EOF_STATE are legit errors given // that tab closing aborts outgoing connections to the server. @@ -100,5 +102,46 @@ export class TestProxy { }); socket.once('close', () => this._sockets.delete(socket)); } - +} + +export async function setupSocksForwardingServer({ + port, forwardPort, allowedTargetPort +}: { + port: number, forwardPort: number, allowedTargetPort: number +}) { + const connectHosts = []; + const connections = new Map(); + const socksProxy = new SocksProxy(); + socksProxy.setPattern('*'); + socksProxy.addListener(SocksProxy.Events.SocksRequested, async (payload: SocksSocketRequestedPayload) => { + if (!['127.0.0.1', 'fake-localhost-127-0-0-1.nip.io', 'localhost'].includes(payload.host) || payload.port !== allowedTargetPort) { + socksProxy.sendSocketError({ uid: payload.uid, error: 'ECONNREFUSED' }); + return; + } + const target = new net.Socket(); + target.on('error', error => socksProxy.sendSocketError({ uid: payload.uid, error: error.toString() })); + target.on('end', () => socksProxy.sendSocketEnd({ uid: payload.uid })); + target.on('data', data => socksProxy.sendSocketData({ uid: payload.uid, data })); + target.setKeepAlive(false); + target.connect(forwardPort, '127.0.0.1'); + target.on('connect', () => { + connections.set(payload.uid, target); + if (!connectHosts.includes(`${payload.host}:${payload.port}`)) + connectHosts.push(`${payload.host}:${payload.port}`); + socksProxy.socketConnected({ uid: payload.uid, host: target.localAddress, port: target.localPort }); + }); + }); + socksProxy.addListener(SocksProxy.Events.SocksData, async (payload: SocksSocketDataPayload) => { + connections.get(payload.uid)?.write(payload.data); + }); + socksProxy.addListener(SocksProxy.Events.SocksClosed, (payload: SocksSocketClosedPayload) => { + connections.get(payload.uid)?.destroy(); + connections.delete(payload.uid); + }); + await socksProxy.listen(port, 'localhost'); + return { + closeProxyServer: () => socksProxy.close(), + proxyServerAddr: `socks5://localhost:${port}`, + connectHosts, + }; } diff --git a/tests/library/client-certificates.spec.ts b/tests/library/client-certificates.spec.ts index 899b9819be..156d80adfb 100644 --- a/tests/library/client-certificates.spec.ts +++ b/tests/library/client-certificates.spec.ts @@ -23,6 +23,7 @@ import type http from 'http'; import { expect, playwrightTest as base } from '../config/browserTest'; import type net from 'net'; import type { BrowserContextOptions } from 'packages/playwright-test'; +import { setupSocksForwardingServer } from '../config/proxy'; const { createHttpsServer, createHttp2Server } = require('../../packages/playwright-core/lib/utils'); type TestOptions = { @@ -88,14 +89,6 @@ const kValidationSubTests: [BrowserContextOptions, string][] = [ passphrase: kDummyFileName, }] }, 'pfx is specified together with cert, key or passphrase'], - [{ - proxy: { server: 'http://localhost:8080' }, - clientCertificates: [{ - origin: 'test', - certPath: kDummyFileName, - keyPath: kDummyFileName, - }] - }, 'Cannot specify both proxy and clientCertificates'], ]; test.describe('fetch', () => { @@ -180,6 +173,54 @@ test.describe('fetch', () => { await request.dispose(); }); + test('pass with trusted client certificates and when a http proxy is used', async ({ playwright, startCCServer, proxyServer, asset }) => { + const serverURL = await startCCServer(); + proxyServer.forwardTo(parseInt(new URL(serverURL).port, 10), { allowConnectRequests: true }); + const request = await playwright.request.newContext({ + ignoreHTTPSErrors: true, + clientCertificates: [{ + origin: new URL(serverURL).origin, + certPath: asset('client-certificates/client/trusted/cert.pem'), + keyPath: asset('client-certificates/client/trusted/key.pem'), + }], + proxy: { server: `localhost:${proxyServer.PORT}` } + }); + expect(proxyServer.connectHosts).toEqual([]); + const response = await request.get(serverURL); + expect(proxyServer.connectHosts).toEqual([new URL(serverURL).host]); + expect(response.url()).toBe(serverURL); + expect(response.status()).toBe(200); + expect(await response.text()).toContain('Hello Alice, your certificate was issued by localhost!'); + await request.dispose(); + }); + + test('pass with trusted client certificates and when a socks proxy is used', async ({ playwright, startCCServer, asset }) => { + const serverURL = await startCCServer({ host: '127.0.0.1' }); + const serverPort = parseInt(new URL(serverURL).port, 10); + const { proxyServerAddr, closeProxyServer, connectHosts } = await setupSocksForwardingServer({ + port: test.info().workerIndex + 2048 + 2, + forwardPort: serverPort, + allowedTargetPort: serverPort, + }); + const request = await playwright.request.newContext({ + ignoreHTTPSErrors: true, + clientCertificates: [{ + origin: new URL(serverURL).origin, + certPath: asset('client-certificates/client/trusted/cert.pem'), + keyPath: asset('client-certificates/client/trusted/key.pem'), + }], + proxy: { server: proxyServerAddr } + }); + expect(connectHosts).toEqual([]); + const response = await request.get(serverURL); + expect(connectHosts).toEqual([new URL(serverURL).host]); + expect(response.url()).toBe(serverURL); + expect(response.status()).toBe(200); + expect(await response.text()).toContain('Hello Alice, your certificate was issued by localhost!'); + await request.dispose(); + await closeProxyServer(); + }); + test('should throw a http error if the pfx passphrase is incorect', async ({ playwright, startCCServer, asset, browserName }) => { const serverURL = await startCCServer(); const request = await playwright.request.newContext({ @@ -311,6 +352,50 @@ test.describe('browser', () => { await page.close(); }); + test('should pass with matching certificates and when a http proxy is used', async ({ browser, startCCServer, asset, browserName, proxyServer }) => { + const serverURL = await startCCServer({ useFakeLocalhost: browserName === 'webkit' && process.platform === 'darwin' }); + proxyServer.forwardTo(parseInt(new URL(serverURL).port, 10), { allowConnectRequests: true }); + const page = await browser.newPage({ + ignoreHTTPSErrors: true, + clientCertificates: [{ + origin: new URL(serverURL).origin, + certPath: asset('client-certificates/client/trusted/cert.pem'), + keyPath: asset('client-certificates/client/trusted/key.pem'), + }], + proxy: { server: `localhost:${proxyServer.PORT}` } + }); + expect(proxyServer.connectHosts).toEqual([]); + await page.goto(serverURL); + expect([...new Set(proxyServer.connectHosts)]).toEqual([`localhost:${new URL(serverURL).port}`]); + await expect(page.getByTestId('message')).toHaveText('Hello Alice, your certificate was issued by localhost!'); + await page.close(); + }); + + test('should pass with matching certificates and when a socks proxy is used', async ({ browser, startCCServer, asset, browserName }) => { + const serverURL = await startCCServer({ useFakeLocalhost: browserName === 'webkit' && process.platform === 'darwin', host: '127.0.0.1' }); + const serverPort = parseInt(new URL(serverURL).port, 10); + const { proxyServerAddr, closeProxyServer, connectHosts } = await setupSocksForwardingServer({ + port: test.info().workerIndex + 2048 + 2, + forwardPort: serverPort, + allowedTargetPort: serverPort, + }); + const page = await browser.newPage({ + ignoreHTTPSErrors: true, + clientCertificates: [{ + origin: new URL(serverURL).origin, + certPath: asset('client-certificates/client/trusted/cert.pem'), + keyPath: asset('client-certificates/client/trusted/key.pem'), + }], + proxy: { server: proxyServerAddr } + }); + expect(connectHosts).toEqual([]); + await page.goto(serverURL); + expect(connectHosts).toEqual([`localhost:${serverPort}`]); + await expect(page.getByTestId('message')).toHaveText('Hello Alice, your certificate was issued by localhost!'); + await page.close(); + await closeProxyServer(); + }); + test('should not hang on tls errors during TLS 1.2 handshake', async ({ browser, asset, platform, browserName }) => { for (const tlsVersion of ['TLSv1.3', 'TLSv1.2'] as const) { await test.step(`TLS version: ${tlsVersion}`, async () => { diff --git a/tests/library/proxy.spec.ts b/tests/library/proxy.spec.ts index 947f3b8788..b2ddcadeba 100644 --- a/tests/library/proxy.spec.ts +++ b/tests/library/proxy.spec.ts @@ -14,10 +14,9 @@ * limitations under the License. */ +import { setupSocksForwardingServer } from '../config/proxy'; import { playwrightTest as it, expect } from '../config/browserTest'; import net from 'net'; -import type { SocksSocketClosedPayload, SocksSocketDataPayload, SocksSocketRequestedPayload } from '../../packages/playwright-core/src/common/socksProxy'; -import { SocksProxy } from '../../packages/playwright-core/lib/common/socksProxy'; it.skip(({ mode }) => mode.startsWith('service')); @@ -288,42 +287,13 @@ it('should use proxy with emulated user agent', async ({ browserType }) => { expect(requestText).toContain('MyUserAgent'); }); -async function setupSocksForwardingServer(port: number, forwardPort: number) { - const connections = new Map(); - const socksProxy = new SocksProxy(); - socksProxy.setPattern('*'); - socksProxy.addListener(SocksProxy.Events.SocksRequested, async (payload: SocksSocketRequestedPayload) => { - if (!['127.0.0.1', 'fake-localhost-127-0-0-1.nip.io'].includes(payload.host) || payload.port !== 1337) { - socksProxy.sendSocketError({ uid: payload.uid, error: 'ECONNREFUSED' }); - return; - } - const target = new net.Socket(); - target.on('error', error => socksProxy.sendSocketError({ uid: payload.uid, error: error.toString() })); - target.on('end', () => socksProxy.sendSocketEnd({ uid: payload.uid })); - target.on('data', data => socksProxy.sendSocketData({ uid: payload.uid, data })); - target.setKeepAlive(false); - target.connect(forwardPort, '127.0.0.1'); - target.on('connect', () => { - connections.set(payload.uid, target); - socksProxy.socketConnected({ uid: payload.uid, host: target.localAddress, port: target.localPort }); - }); - }); - socksProxy.addListener(SocksProxy.Events.SocksData, async (payload: SocksSocketDataPayload) => { - connections.get(payload.uid)?.write(payload.data); - }); - socksProxy.addListener(SocksProxy.Events.SocksClosed, (payload: SocksSocketClosedPayload) => { - connections.get(payload.uid)?.destroy(); - connections.delete(payload.uid); - }); - await socksProxy.listen(port, 'localhost'); - return { - closeProxyServer: () => socksProxy.close(), - proxyServerAddr: `socks5://localhost:${port}`, - }; -} -it('should use SOCKS proxy for websocket requests', async ({ browserName, platform, browserType, server }, testInfo) => { - const { proxyServerAddr, closeProxyServer } = await setupSocksForwardingServer(testInfo.workerIndex + 2048 + 2, server.PORT); +it('should use SOCKS proxy for websocket requests', async ({ browserType, server }) => { + const { proxyServerAddr, closeProxyServer } = await setupSocksForwardingServer({ + port: it.info().workerIndex + 2048 + 2, + forwardPort: server.PORT, + allowedTargetPort: 1337, + }); const browser = await browserType.launch({ proxy: { server: proxyServerAddr, From ce06a81aa6563cb63c8a5077b91fef753ed30ea3 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 16 Sep 2024 12:54:20 -0700 Subject: [PATCH 06/13] feat: make `npx playwright clear-cache` public (#32638) --- packages/playwright/src/program.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/playwright/src/program.ts b/packages/playwright/src/program.ts index dd8d181676..775ef27065 100644 --- a/packages/playwright/src/program.ts +++ b/packages/playwright/src/program.ts @@ -69,7 +69,7 @@ function addListFilesCommand(program: Command) { } function addClearCacheCommand(program: Command) { - const command = program.command('clear-cache', { hidden: true }); + const command = program.command('clear-cache'); command.description('clears build and test caches'); command.option('-c, --config ', `Configuration file, or a test directory with optional "playwright.config.{m,c}?{js,ts}"`); command.action(async opts => { From 92c6408b94c419ab101579bd1ef4403957d3d738 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 16 Sep 2024 13:47:13 -0700 Subject: [PATCH 07/13] fix(recorder): address the react race condition (#32628) --- .../playwright-core/src/server/recorder.ts | 5 +-- .../src/server/recorder/contextRecorder.ts | 5 +-- .../src/server/recorder/recorderApp.ts | 8 ++-- .../src/server/recorder/recorderFrontend.ts | 2 +- .../server/recorder/recorderInTraceViewer.ts | 2 +- packages/recorder/src/recorder.tsx | 40 ++++++++----------- packages/recorder/src/recorderTypes.ts | 2 +- 7 files changed, 27 insertions(+), 37 deletions(-) diff --git a/packages/playwright-core/src/server/recorder.ts b/packages/playwright-core/src/server/recorder.ts index 5e197f871f..ddaa035811 100644 --- a/packages/playwright-core/src/server/recorder.ts +++ b/packages/playwright-core/src/server/recorder.ts @@ -132,10 +132,9 @@ export class Recorder implements InstrumentationListener, IRecorder { this._context.instrumentation.removeListener(this); this._recorderApp?.close().catch(() => {}); }); - this._contextRecorder.on(ContextRecorder.Events.Change, (data: { sources: Source[], primaryFileName: string }) => { + this._contextRecorder.on(ContextRecorder.Events.Change, (data: { sources: Source[] }) => { this._recorderSources = data.sources; this._pushAllSources(); - this._recorderApp?.setFileIfNeeded(data.primaryFileName); }); await this._context.exposeBinding('__pw_recorderState', false, source => { @@ -294,7 +293,7 @@ export class Recorder implements InstrumentationListener, IRecorder { } this._pushAllSources(); if (fileToSelect) - this._recorderApp?.setFileIfNeeded(fileToSelect); + this._recorderApp?.setFile(fileToSelect); } private _pushAllSources() { diff --git a/packages/playwright-core/src/server/recorder/contextRecorder.ts b/packages/playwright-core/src/server/recorder/contextRecorder.ts index 9b4efb9e65..6642f13a62 100644 --- a/packages/playwright-core/src/server/recorder/contextRecorder.ts +++ b/packages/playwright-core/src/server/recorder/contextRecorder.ts @@ -97,10 +97,7 @@ export class ContextRecorder extends EventEmitter { if (languageGenerator === this._orderedLanguages[0]) this._throttledOutputFile?.setContent(source.text); } - this.emit(ContextRecorder.Events.Change, { - sources: this._recorderSources, - primaryFileName: this._orderedLanguages[0].id - }); + this.emit(ContextRecorder.Events.Change, { sources: this._recorderSources }); }); context.on(BrowserContext.Events.BeforeClose, () => { this._throttledOutputFile?.flush(); diff --git a/packages/playwright-core/src/server/recorder/recorderApp.ts b/packages/playwright-core/src/server/recorder/recorderApp.ts index 8044fadf41..67d8b6e8dc 100644 --- a/packages/playwright-core/src/server/recorder/recorderApp.ts +++ b/packages/playwright-core/src/server/recorder/recorderApp.ts @@ -30,7 +30,7 @@ import type { IRecorder, IRecorderApp, IRecorderAppFactory } from './recorderFro declare global { interface Window { - playwrightSetFileIfNeeded: (file: string) => void; + playwrightSetFile: (file: string) => void; playwrightSetMode: (mode: Mode) => void; playwrightSetPaused: (paused: boolean) => void; playwrightSetSources: (sources: Source[]) => void; @@ -46,7 +46,7 @@ export class EmptyRecorderApp extends EventEmitter implements IRecorderApp { async close(): Promise {} async setPaused(paused: boolean): Promise {} async setMode(mode: Mode): Promise {} - async setFileIfNeeded(file: string): Promise {} + async setFile(file: string): Promise {} async setSelector(selector: string, userGesture?: boolean): Promise {} async updateCallLogs(callLogs: CallLog[]): Promise {} async setSources(sources: Source[]): Promise {} @@ -144,9 +144,9 @@ export class RecorderApp extends EventEmitter implements IRecorderApp { }).toString(), { isFunction: true }, mode).catch(() => {}); } - async setFileIfNeeded(file: string): Promise { + async setFile(file: string): Promise { await this._page.mainFrame().evaluateExpression(((file: string) => { - window.playwrightSetFileIfNeeded(file); + window.playwrightSetFile(file); }).toString(), { isFunction: true }, file).catch(() => {}); } diff --git a/packages/playwright-core/src/server/recorder/recorderFrontend.ts b/packages/playwright-core/src/server/recorder/recorderFrontend.ts index 161aa71eca..162c9f9964 100644 --- a/packages/playwright-core/src/server/recorder/recorderFrontend.ts +++ b/packages/playwright-core/src/server/recorder/recorderFrontend.ts @@ -26,7 +26,7 @@ export interface IRecorderApp extends EventEmitter { close(): Promise; setPaused(paused: boolean): Promise; setMode(mode: Mode): Promise; - setFileIfNeeded(file: string): Promise; + setFile(file: string): Promise; setSelector(selector: string, userGesture?: boolean): Promise; updateCallLogs(callLogs: CallLog[]): Promise; setSources(sources: Source[]): Promise; diff --git a/packages/playwright-core/src/server/recorder/recorderInTraceViewer.ts b/packages/playwright-core/src/server/recorder/recorderInTraceViewer.ts index f7613ffc54..a9fd766141 100644 --- a/packages/playwright-core/src/server/recorder/recorderInTraceViewer.ts +++ b/packages/playwright-core/src/server/recorder/recorderInTraceViewer.ts @@ -55,7 +55,7 @@ export class RecorderInTraceViewer extends EventEmitter implements IRecorderApp this._transport.sendEvent?.('setMode', { mode }); } - async setFileIfNeeded(file: string): Promise { + async setFile(file: string): Promise { this._transport.sendEvent?.('setFileIfNeeded', { file }); } diff --git a/packages/recorder/src/recorder.tsx b/packages/recorder/src/recorder.tsx index 486aeae701..31bad2b70b 100644 --- a/packages/recorder/src/recorder.tsx +++ b/packages/recorder/src/recorder.tsx @@ -27,14 +27,6 @@ import { asLocator } from '@isomorphic/locatorGenerators'; import { toggleTheme } from '@web/theme'; import { copy } from '@web/uiUtils'; -declare global { - interface Window { - playwrightSetFileIfNeeded: (file: string) => void; - playwrightSetSelector: (selector: string, focus?: boolean) => void; - dispatch(data: any): Promise; - } -} - export interface RecorderProps { sources: Source[], paused: boolean, @@ -56,14 +48,22 @@ export const Recorder: React.FC = ({ setFileId(sources[0].id); }, [fileId, sources]); - const source: Source = sources.find(s => s.id === fileId) || { - id: 'default', - isRecorded: false, - text: '', - language: 'javascript', - label: '', - highlight: [] - }; + const source = React.useMemo(() => { + if (fileId) { + const source = sources.find(s => s.id === fileId); + if (source) + return source; + } + const source: Source = { + id: 'default', + isRecorded: false, + text: '', + language: 'javascript', + label: '', + highlight: [] + }; + return source; + }, [sources, fileId]); const [locator, setLocator] = React.useState(''); window.playwrightSetSelector = (selector: string, focus?: boolean) => { @@ -73,13 +73,7 @@ export const Recorder: React.FC = ({ setLocator(asLocator(language, selector)); }; - window.playwrightSetFileIfNeeded = (value: string) => { - const newSource = sources.find(s => s.id === value); - // Do not forcefully switch between two recorded sources, because - // user did explicitly choose one. - if (newSource && !newSource.isRecorded || !source.isRecorded) - setFileId(value); - }; + window.playwrightSetFile = setFileId; const messagesEndRef = React.useRef(null); React.useLayoutEffect(() => { diff --git a/packages/recorder/src/recorderTypes.ts b/packages/recorder/src/recorderTypes.ts index c56984ad6d..a5791e2306 100644 --- a/packages/recorder/src/recorderTypes.ts +++ b/packages/recorder/src/recorderTypes.ts @@ -96,7 +96,7 @@ declare global { playwrightSetSources: (sources: Source[]) => void; playwrightSetOverlayVisible: (visible: boolean) => void; playwrightUpdateLogs: (callLogs: CallLog[]) => void; - playwrightSetFileIfNeeded: (file: string) => void; + playwrightSetFile: (file: string) => void; playwrightSetSelector: (selector: string, focus?: boolean) => void; playwrightSourcesEchoForTest: Source[]; dispatch(data: any): Promise; From 6dbde62a6b7a326fc70869b693de7bc3885b5883 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 16 Sep 2024 14:39:36 -0700 Subject: [PATCH 08/13] chore: simplify signal handling while recording (#32624) --- .../src/server/codegen/language.ts | 17 +++ .../src/server/codegen/types.ts | 2 +- .../src/server/recorder/contextRecorder.ts | 59 ++------ .../src/server/recorder/recorderCollection.ts | 133 +++++++----------- .../src/server/recorder/recorderUtils.ts | 22 ++- tests/library/inspector/cli-codegen-1.spec.ts | 30 +++- 6 files changed, 129 insertions(+), 134 deletions(-) diff --git a/packages/playwright-core/src/server/codegen/language.ts b/packages/playwright-core/src/server/codegen/language.ts index 3e0c8f71e5..4b1ba99b6f 100644 --- a/packages/playwright-core/src/server/codegen/language.ts +++ b/packages/playwright-core/src/server/codegen/language.ts @@ -20,6 +20,7 @@ import type * as types from '../types'; import type { ActionInContext, LanguageGenerator, LanguageGeneratorOptions } from './types'; export function generateCode(actions: ActionInContext[], languageGenerator: LanguageGenerator, options: LanguageGeneratorOptions) { + actions = collapseActions(actions); const header = languageGenerator.generateHeader(options); const footer = languageGenerator.generateFooter(options.saveStorage); const actionTexts = actions.map(a => languageGenerator.generateAction(a)).filter(Boolean); @@ -83,3 +84,19 @@ export function toClickOptionsForSourceCode(action: actions.ClickAction): types. options.position = action.position; return options; } + +function collapseActions(actions: ActionInContext[]): ActionInContext[] { + const result: ActionInContext[] = []; + for (const action of actions) { + const lastAction = result[result.length - 1]; + const isSameAction = lastAction && lastAction.action.name === action.action.name && lastAction.frame.pageAlias === action.frame.pageAlias && lastAction.frame.framePath.join('|') === action.frame.framePath.join('|'); + const isSameSelector = lastAction && 'selector' in lastAction.action && 'selector' in action.action && action.action.selector === lastAction.action.selector; + const shouldMerge = isSameAction && (action.action.name === 'navigate' || (action.action.name === 'fill' && isSameSelector)); + if (!shouldMerge) { + result.push(action); + continue; + } + result[result.length - 1] = action; + } + return result; +} diff --git a/packages/playwright-core/src/server/codegen/types.ts b/packages/playwright-core/src/server/codegen/types.ts index 96f2aa85d1..48c1141a3e 100644 --- a/packages/playwright-core/src/server/codegen/types.ts +++ b/packages/playwright-core/src/server/codegen/types.ts @@ -36,7 +36,7 @@ export type ActionInContext = { frame: FrameDescription; description?: string; action: actions.Action; - committed?: boolean; + timestamp: number; }; export interface LanguageGenerator { diff --git a/packages/playwright-core/src/server/recorder/contextRecorder.ts b/packages/playwright-core/src/server/recorder/contextRecorder.ts index 6642f13a62..71a1d3ec75 100644 --- a/packages/playwright-core/src/server/recorder/contextRecorder.ts +++ b/packages/playwright-core/src/server/recorder/contextRecorder.ts @@ -18,7 +18,7 @@ import type * as channels from '@protocol/channels'; import type { Source } from '@recorder/recorderTypes'; import { EventEmitter } from 'events'; import * as recorderSource from '../../generated/recorderSource'; -import { eventsHelper, isUnderTest, monotonicTime, quoteCSSAttributeValue, type RegisteredListener } from '../../utils'; +import { eventsHelper, monotonicTime, quoteCSSAttributeValue, type RegisteredListener } from '../../utils'; import { raceAgainstDeadline } from '../../utils/timeoutRunner'; import { BrowserContext } from '../browserContext'; import type { ActionInContext, FrameDescription, LanguageGeneratorOptions, Language, LanguageGenerator } from '../codegen/types'; @@ -27,7 +27,6 @@ import type { Dialog } from '../dialog'; import { Frame } from '../frames'; import { Page } from '../page'; import type * as actions from './recorderActions'; -import { performAction } from './recorderRunner'; import { ThrottledFile } from './throttledFile'; import { RecorderCollection } from './recorderCollection'; import { generateCode } from '../codegen/language'; @@ -48,7 +47,6 @@ export class ContextRecorder extends EventEmitter { private _lastPopupOrdinal = 0; private _lastDialogOrdinal = -1; private _lastDownloadOrdinal = -1; - private _timers = new Set(); private _context: BrowserContext; private _params: channels.BrowserContextRecorderSupplementEnableParams; private _delegate: ContextRecorderDelegate; @@ -150,9 +148,6 @@ export class ContextRecorder extends EventEmitter { } dispose() { - for (const timer of this._timers) - clearTimeout(timer); - this._timers.clear(); eventsHelper.removeEventListeners(this._listeners); } @@ -162,11 +157,11 @@ export class ContextRecorder extends EventEmitter { page.on('close', () => { this._collection.addRecordedAction({ frame: this._describeMainFrame(page), - committed: true, action: { name: 'closePage', signals: [], - } + }, + timestamp: monotonicTime() }); this._pageAliases.delete(page); }); @@ -184,12 +179,12 @@ export class ContextRecorder extends EventEmitter { } else { this._collection.addRecordedAction({ frame: this._describeMainFrame(page), - committed: true, action: { name: 'openPage', url: page.mainFrame().url(), signals: [], - } + }, + timestamp: monotonicTime() }); } } @@ -220,54 +215,24 @@ export class ContextRecorder extends EventEmitter { return this._params.testIdAttributeName || this._context.selectors().testIdAttributeName() || 'data-testid'; } - private async _performAction(frame: Frame, action: actions.PerformOnRecordAction) { - // Commit last action so that no further signals are added to it. - this._collection.commitLastAction(); - + private async _createActionInContext(frame: Frame, action: actions.Action): Promise { const frameDescription = await this._describeFrame(frame); const actionInContext: ActionInContext = { frame: frameDescription, action, description: undefined, + timestamp: monotonicTime() }; - await this._delegate.rewriteActionInContext?.(this._pageAliases, actionInContext); + return actionInContext; + } - const callMetadata = await this._collection.willPerformAction(actionInContext); - if (!callMetadata) - return; - const error = await performAction(callMetadata, this._pageAliases, actionInContext).then(() => undefined).catch((e: Error) => e); - await this._collection.didPerformAction(callMetadata, actionInContext, error); - if (error) - actionInContext.committed = true; - else - this._setCommittedAfterTimeout(actionInContext); + private async _performAction(frame: Frame, action: actions.PerformOnRecordAction) { + await this._collection.performAction(await this._createActionInContext(frame, action)); } private async _recordAction(frame: Frame, action: actions.Action) { - // Commit last action so that no further signals are added to it. - this._collection.commitLastAction(); - - const frameDescription = await this._describeFrame(frame); - const actionInContext: ActionInContext = { - frame: frameDescription, - action, - description: undefined, - }; - - await this._delegate.rewriteActionInContext?.(this._pageAliases, actionInContext); - - this._setCommittedAfterTimeout(actionInContext); - this._collection.addRecordedAction(actionInContext); - } - - private _setCommittedAfterTimeout(actionInContext: ActionInContext) { - const timer = setTimeout(() => { - // Commit the action after 5 seconds so that no further signals are added to it. - actionInContext.committed = true; - this._timers.delete(timer); - }, isUnderTest() ? 500 : 5000); - this._timers.add(timer); + this._collection.addRecordedAction(await this._createActionInContext(frame, action)); } private _onFrameNavigated(frame: Frame, page: Page) { diff --git a/packages/playwright-core/src/server/recorder/recorderCollection.ts b/packages/playwright-core/src/server/recorder/recorderCollection.ts index fbfbf8f26e..e9c2b31427 100644 --- a/packages/playwright-core/src/server/recorder/recorderCollection.ts +++ b/packages/playwright-core/src/server/recorder/recorderCollection.ts @@ -19,13 +19,14 @@ import type { Frame } from '../frames'; import type { Page } from '../page'; import type { Signal } from './recorderActions'; import type { ActionInContext } from '../codegen/types'; -import type { CallMetadata } from '@protocol/callMetadata'; -import { createGuid } from '../../utils/crypto'; import { monotonicTime } from '../../utils/time'; -import { mainFrameForAction, traceParamsForAction } from './recorderUtils'; +import { callMetadataForAction } from './recorderUtils'; +import { serializeError } from '../errors'; +import { performAction } from './recorderRunner'; +import type { CallMetadata } from '@protocol/callMetadata'; +import { isUnderTest } from '../../utils/debug'; export class RecorderCollection extends EventEmitter { - private _lastAction: ActionInContext | null = null; private _actions: ActionInContext[] = []; private _enabled: boolean; private _pageAliases: Map; @@ -38,7 +39,6 @@ export class RecorderCollection extends EventEmitter { } restart() { - this._lastAction = null; this._actions = []; this.emit('change'); } @@ -51,98 +51,73 @@ export class RecorderCollection extends EventEmitter { this._enabled = enabled; } - async willPerformAction(actionInContext: ActionInContext): Promise { - if (!this._enabled) - return null; - const { callMetadata, mainFrame } = this._callMetadataForAction(actionInContext); - await mainFrame.instrumentation.onBeforeCall(mainFrame, callMetadata); - this._lastAction = actionInContext; - return callMetadata; - } - - private _callMetadataForAction(actionInContext: ActionInContext): { callMetadata: CallMetadata, mainFrame: Frame } { - const mainFrame = mainFrameForAction(this._pageAliases, actionInContext); - const { action } = actionInContext; - const callMetadata: CallMetadata = { - id: `call@${createGuid()}`, - apiName: 'frame.' + action.name, - objectId: mainFrame.guid, - pageId: mainFrame._page.guid, - frameId: mainFrame.guid, - startTime: monotonicTime(), - endTime: 0, - type: 'Frame', - method: action.name, - params: traceParamsForAction(actionInContext), - log: [], - }; - return { callMetadata, mainFrame }; - } - - async didPerformAction(callMetadata: CallMetadata, actionInContext: ActionInContext, error?: Error) { - if (!this._enabled) - return; - - if (!error) - this._actions.push(actionInContext); - - const mainFrame = mainFrameForAction(this._pageAliases, actionInContext); - callMetadata.endTime = monotonicTime(); - await mainFrame.instrumentation.onAfterCall(mainFrame, callMetadata); - - this.emit('change'); + async performAction(actionInContext: ActionInContext) { + await this._addAction(actionInContext, async callMetadata => { + await performAction(callMetadata, this._pageAliases, actionInContext); + }); } addRecordedAction(actionInContext: ActionInContext) { - if (!this._enabled) - return; - const action = actionInContext.action; - - const lastAction = this._lastAction && this._lastAction.frame.pageAlias === actionInContext.frame.pageAlias ? this._lastAction.action : undefined; - if (lastAction && action.name === 'navigate' && lastAction.name === 'navigate' && action.url === lastAction.url) { - // Already at a target URL. + if (['openPage', 'closePage'].includes(actionInContext.action.name)) { + this._actions.push(actionInContext); + this.emit('change'); return; } - - if (lastAction && action.name === 'fill' && lastAction.name === 'fill' && action.selector === lastAction.selector) - this._actions.pop(); - - this._lastAction = actionInContext; - this._actions.push(actionInContext); - this.emit('change'); + this._addAction(actionInContext).catch(() => {}); } - commitLastAction() { + private async _addAction(actionInContext: ActionInContext, callback?: (callMetadata: CallMetadata) => Promise) { if (!this._enabled) return; - const action = this._lastAction; - if (action) - action.committed = true; + + const { callMetadata, mainFrame } = callMetadataForAction(this._pageAliases, actionInContext); + await mainFrame.instrumentation.onBeforeCall(mainFrame, callMetadata); + this._actions.push(actionInContext); + this.emit('change'); + const error = await callback?.(callMetadata).catch((e: Error) => e); + callMetadata.endTime = monotonicTime(); + callMetadata.error = error ? serializeError(error) : undefined; + await mainFrame.instrumentation.onAfterCall(mainFrame, callMetadata); } signal(pageAlias: string, frame: Frame, signal: Signal) { if (!this._enabled) return; - if (this._lastAction && !this._lastAction.committed) { - this._lastAction.action.signals.push(signal); - this.emit('change'); + if (signal.name === 'navigation' && frame._page.mainFrame() === frame) { + const timestamp = monotonicTime(); + const lastAction = this._actions[this._actions.length - 1]; + const signalThreshold = isUnderTest() ? 500 : 5000; + + let generateGoto = false; + if (!lastAction) + generateGoto = true; + else if (lastAction.action.name !== 'click' && lastAction.action.name !== 'press') + generateGoto = true; + else if (timestamp - lastAction.timestamp > signalThreshold) + generateGoto = true; + + if (generateGoto) { + this.addRecordedAction({ + frame: { + pageAlias, + framePath: [], + }, + action: { + name: 'navigate', + url: frame.url(), + signals: [], + }, + timestamp + }); + } return; } - if (signal.name === 'navigation' && frame._page.mainFrame() === frame) { - this.addRecordedAction({ - frame: { - pageAlias, - framePath: [], - }, - committed: true, - action: { - name: 'navigate', - url: frame.url(), - signals: [], - }, - }); + if (this._actions.length) { + this._actions[this._actions.length - 1].action.signals.push(signal); + this.emit('change'); + return; } } } diff --git a/packages/playwright-core/src/server/recorder/recorderUtils.ts b/packages/playwright-core/src/server/recorder/recorderUtils.ts index 234fc79a0f..ac6c970489 100644 --- a/packages/playwright-core/src/server/recorder/recorderUtils.ts +++ b/packages/playwright-core/src/server/recorder/recorderUtils.ts @@ -22,6 +22,7 @@ import type { Frame } from '../frames'; import type * as actions from './recorderActions'; import { toKeyboardModifiers } from '../codegen/language'; import { serializeExpectedTextValues } from '../../utils/expectUtils'; +import { createGuid, monotonicTime } from '../../utils'; export function metadataToCallLog(metadata: CallMetadata, status: CallLogStatus): CallLog { let title = metadata.apiName || metadata.method; @@ -59,7 +60,7 @@ export function mainFrameForAction(pageAliases: Map, actionInConte const pageAlias = actionInContext.frame.pageAlias; const page = [...pageAliases.entries()].find(([, alias]) => pageAlias === alias)?.[0]; if (!page) - throw new Error('Internal error: page not found'); + throw new Error(`Internal error: page ${pageAlias} not found in [${[...pageAliases.values()]}]`); return page.mainFrame(); } @@ -129,3 +130,22 @@ export function traceParamsForAction(actionInContext: ActionInContext) { } } } + +export function callMetadataForAction(pageAliases: Map, actionInContext: ActionInContext): { callMetadata: CallMetadata, mainFrame: Frame } { + const mainFrame = mainFrameForAction(pageAliases, actionInContext); + const { action } = actionInContext; + const callMetadata: CallMetadata = { + id: `call@${createGuid()}`, + apiName: 'frame.' + action.name, + objectId: mainFrame.guid, + pageId: mainFrame._page.guid, + frameId: mainFrame.guid, + startTime: monotonicTime(), + endTime: 0, + type: 'Frame', + method: action.name, + params: traceParamsForAction(actionInContext), + log: [], + }; + return { callMetadata, mainFrame }; +} diff --git a/tests/library/inspector/cli-codegen-1.spec.ts b/tests/library/inspector/cli-codegen-1.spec.ts index 4320aac5e8..c8046ef9b4 100644 --- a/tests/library/inspector/cli-codegen-1.spec.ts +++ b/tests/library/inspector/cli-codegen-1.spec.ts @@ -706,7 +706,7 @@ var page1 = await page.RunAndWaitForPopupAsync(async () => expect(popup.url()).toBe('about:blank'); }); - test('should assert navigation', async ({ page, openRecorder }) => { + test('should attribute navigation to click', async ({ page, openRecorder }) => { const recorder = await openRecorder(); await recorder.setContentAndWait(`link`); @@ -720,24 +720,42 @@ var page1 = await page.RunAndWaitForPopupAsync(async () => ]); expect.soft(sources.get('JavaScript')!.text).toContain(` - await page.getByText('link').click();`); + await page.goto('about:blank'); + await page.getByText('link').click(); + + // --------------------- + await context.close();`); expect.soft(sources.get('Playwright Test')!.text).toContain(` - await page.getByText('link').click();`); + await page.goto('about:blank'); + await page.getByText('link').click(); +});`); expect.soft(sources.get('Java')!.text).toContain(` - page.getByText("link").click();`); + page.navigate(\"about:blank\"); + page.getByText(\"link\").click(); + }`); expect.soft(sources.get('Python')!.text).toContain(` - page.get_by_text("link").click()`); + page.goto("about:blank") + page.get_by_text("link").click() + + # --------------------- + context.close()`); expect.soft(sources.get('Python Async')!.text).toContain(` - await page.get_by_text("link").click()`); + await page.goto("about:blank") + await page.get_by_text("link").click() + + # --------------------- + await context.close()`); expect.soft(sources.get('Pytest')!.text).toContain(` + page.goto("about:blank") page.get_by_text("link").click()`); expect.soft(sources.get('C#')!.text).toContain(` +await page.GotoAsync("about:blank"); await page.GetByText("link").ClickAsync();`); expect(page.url()).toContain('about:blank#foo'); From 2a347b5494e7794a06770634bb2986929c32c727 Mon Sep 17 00:00:00 2001 From: Yury Semikhatsky Date: Mon, 16 Sep 2024 17:31:26 -0700 Subject: [PATCH 09/13] chore: support launchPersistentContext with bidi (#32641) --- .../src/server/bidi/bidiBrowser.ts | 15 ++++++++++++--- .../src/server/bidi/bidiFirefox.ts | 6 ------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/packages/playwright-core/src/server/bidi/bidiBrowser.ts b/packages/playwright-core/src/server/bidi/bidiBrowser.ts index cc98e2ff3a..9861fc80cf 100644 --- a/packages/playwright-core/src/server/bidi/bidiBrowser.ts +++ b/packages/playwright-core/src/server/bidi/bidiBrowser.ts @@ -94,6 +94,14 @@ export class BidiBrowser extends Browser { 'script', ], }); + + if (options.persistent) { + browser._defaultContext = new BidiBrowserContext(browser, undefined, options.persistent); + await (browser._defaultContext as BidiBrowserContext)._initialize(); + // Create default page as we cannot get access to the existing one. + const pageDelegate = await browser._defaultContext.newPageDelegate(); + await pageDelegate.pageOrError(); + } return browser; } @@ -294,10 +302,11 @@ export class BidiBrowserContext extends BrowserContext { } async doClose(reason: string | undefined) { - // TODO: implement for persistent context - if (!this._browserContextId) + if (!this._browserContextId) { + // Closing persistent context should close the browser. + await this._browser.close({ reason }); return; - + } await this._browser._browserSession.send('browser.removeUserContext', { userContext: this._browserContextId }); diff --git a/packages/playwright-core/src/server/bidi/bidiFirefox.ts b/packages/playwright-core/src/server/bidi/bidiFirefox.ts index 3a2da48c25..3fb7c15b90 100644 --- a/packages/playwright-core/src/server/bidi/bidiFirefox.ts +++ b/packages/playwright-core/src/server/bidi/bidiFirefox.ts @@ -76,12 +76,6 @@ export class BidiFirefox extends BrowserType { firefoxArguments.push('--foreground'); firefoxArguments.push(`--profile`, userDataDir); firefoxArguments.push(...args); - // TODO: make ephemeral context work without this argument. - firefoxArguments.push('about:blank'); - // if (isPersistent) - // firefoxArguments.push('about:blank'); - // else - // firefoxArguments.push('-silent'); return firefoxArguments; } From 3bff7b6ab1ce5100b59b8c07d0dc342cf87d6e24 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 16 Sep 2024 17:33:52 -0700 Subject: [PATCH 10/13] chore: preserve selected trace action in live trace (#32630) --- packages/trace-viewer/src/ui/errorsTab.tsx | 2 +- packages/trace-viewer/src/ui/modelUtil.ts | 4 -- .../trace-viewer/src/ui/uiModeTraceView.tsx | 11 +--- packages/trace-viewer/src/ui/workbench.tsx | 59 +++++++++++-------- 4 files changed, 36 insertions(+), 40 deletions(-) diff --git a/packages/trace-viewer/src/ui/errorsTab.tsx b/packages/trace-viewer/src/ui/errorsTab.tsx index 71b6059c29..acf5bf838e 100644 --- a/packages/trace-viewer/src/ui/errorsTab.tsx +++ b/packages/trace-viewer/src/ui/errorsTab.tsx @@ -22,7 +22,7 @@ import { renderAction } from './actionList'; import type { Language } from '@isomorphic/locatorGenerators'; import type { StackFrame } from '@protocol/channels'; -type ErrorDescription = { +export type ErrorDescription = { action?: modelUtil.ActionTraceEventInContext; stack?: StackFrame[]; }; diff --git a/packages/trace-viewer/src/ui/modelUtil.ts b/packages/trace-viewer/src/ui/modelUtil.ts index 0c22d954f4..a544d4dc3f 100644 --- a/packages/trace-viewer/src/ui/modelUtil.ts +++ b/packages/trace-viewer/src/ui/modelUtil.ts @@ -342,10 +342,6 @@ export function buildActionTree(actions: ActionTraceEventInContext[]): { rootIte return { rootItem, itemMap }; } -export function idForAction(action: ActionTraceEvent) { - return `${action.pageId || 'none'}:${action.callId}`; -} - export function context(action: ActionTraceEvent | trace.EventTraceEvent | ResourceSnapshot): ContextEntry { return (action as any)[contextSymbol]; } diff --git a/packages/trace-viewer/src/ui/uiModeTraceView.tsx b/packages/trace-viewer/src/ui/uiModeTraceView.tsx index 48fd59e3ca..c0763fee3d 100644 --- a/packages/trace-viewer/src/ui/uiModeTraceView.tsx +++ b/packages/trace-viewer/src/ui/uiModeTraceView.tsx @@ -16,14 +16,13 @@ import { artifactsFolderName } from '@testIsomorphic/folders'; import type { TreeItem } from '@testIsomorphic/testTree'; -import type { ActionTraceEvent } from '@trace/trace'; import '@web/common.css'; import '@web/third_party/vscode/codicon.css'; import type * as reporterTypes from 'playwright/types/testReporter'; import React from 'react'; import type { ContextEntry } from '../entries'; import type { SourceLocation } from './modelUtil'; -import { idForAction, MultiTraceModel } from './modelUtil'; +import { MultiTraceModel } from './modelUtil'; import { Workbench } from './workbench'; export const TraceView: React.FC<{ @@ -42,12 +41,6 @@ export const TraceView: React.FC<{ return { outputDir }; }, [item]); - // Preserve user selection upon live-reloading trace model by persisting the action id. - // This avoids auto-selection of the last action every time we reload the model. - const [selectedActionId, setSelectedActionId] = React.useState(); - const onSelectionChanged = React.useCallback((action: ActionTraceEvent) => setSelectedActionId(idForAction(action)), [setSelectedActionId]); - const initialSelection = selectedActionId ? model?.model.actions.find(a => idForAction(a) === selectedActionId) : undefined; - React.useEffect(() => { if (pollTimer.current) clearTimeout(pollTimer.current); @@ -98,8 +91,6 @@ export const TraceView: React.FC<{ model={model?.model} showSourcesFirst={true} rootDir={rootDir} - initialSelection={initialSelection} - onSelectionChanged={onSelectionChanged} fallbackLocation={item.testFile} isLive={model?.isLive} status={item.treeItem?.status} diff --git a/packages/trace-viewer/src/ui/workbench.tsx b/packages/trace-viewer/src/ui/workbench.tsx index 3565eb3850..e1ce2298ae 100644 --- a/packages/trace-viewer/src/ui/workbench.tsx +++ b/packages/trace-viewer/src/ui/workbench.tsx @@ -20,11 +20,11 @@ import { ActionList } from './actionList'; import { CallTab } from './callTab'; import { LogTab } from './logTab'; import { ErrorsTab, useErrorsTabModel } from './errorsTab'; +import type { ErrorDescription } from './errorsTab'; import type { ConsoleEntry } from './consoleTab'; import { ConsoleTab, useConsoleTabModel } from './consoleTab'; import type * as modelUtil from './modelUtil'; import { isRouteAction } from './modelUtil'; -import type { StackFrame } from '@protocol/channels'; import { NetworkTab, useNetworkTabModel } from './networkTab'; import { SnapshotTab } from './snapshotTab'; import { SourceTab } from './sourceTab'; @@ -49,8 +49,6 @@ export const Workbench: React.FunctionComponent<{ showSourcesFirst?: boolean, rootDir?: string, fallbackLocation?: modelUtil.SourceLocation, - initialSelection?: modelUtil.ActionTraceEventInContext, - onSelectionChanged?: (action: modelUtil.ActionTraceEventInContext) => void, isLive?: boolean, status?: UITestStatus, annotations?: { type: string; description?: string; }[]; @@ -59,9 +57,10 @@ export const Workbench: React.FunctionComponent<{ onOpenExternally?: (location: modelUtil.SourceLocation) => void, revealSource?: boolean, showSettings?: boolean, -}> = ({ model, showSourcesFirst, rootDir, fallbackLocation, initialSelection, onSelectionChanged, isLive, status, annotations, inert, openPage, onOpenExternally, revealSource, showSettings }) => { - const [selectedAction, setSelectedActionImpl] = React.useState(undefined); - const [revealedStack, setRevealedStack] = React.useState(undefined); +}> = ({ model, showSourcesFirst, rootDir, fallbackLocation, isLive, status, annotations, inert, openPage, onOpenExternally, revealSource, showSettings }) => { + const [selectedCallId, setSelectedCallId] = React.useState(undefined); + const [revealedError, setRevealedError] = React.useState(undefined); + const [highlightedAction, setHighlightedAction] = React.useState(); const [highlightedEntry, setHighlightedEntry] = React.useState(); const [highlightedConsoleMessage, setHighlightedConsoleMessage] = React.useState(); @@ -69,38 +68,39 @@ export const Workbench: React.FunctionComponent<{ const [selectedPropertiesTab, setSelectedPropertiesTab] = useSetting('propertiesTab', showSourcesFirst ? 'source' : 'call'); const [isInspecting, setIsInspectingState] = React.useState(false); const [highlightedLocator, setHighlightedLocator] = React.useState(''); - const activeAction = model ? highlightedAction || selectedAction : undefined; const [selectedTime, setSelectedTime] = React.useState(); const [sidebarLocation, setSidebarLocation] = useSetting<'bottom' | 'right'>('propertiesSidebarLocation', 'bottom'); const [showRouteActions, setShowRouteActions] = useSetting('show-route-actions', true); const [showScreenshot, setShowScreenshot] = useSetting('screenshot-instead-of-snapshot', false); - const filteredActions = React.useMemo(() => { return (model?.actions || []).filter(action => showRouteActions || !isRouteAction(action)); }, [model, showRouteActions]); const setSelectedAction = React.useCallback((action: modelUtil.ActionTraceEventInContext | undefined) => { - setSelectedActionImpl(action); - setRevealedStack(action?.stack); - }, [setSelectedActionImpl, setRevealedStack]); + setSelectedCallId(action?.callId); + setRevealedError(undefined); + }, []); const sources = React.useMemo(() => model?.sources || new Map(), [model]); React.useEffect(() => { setSelectedTime(undefined); - setRevealedStack(undefined); + setRevealedError(undefined); }, [model]); - React.useEffect(() => { - if (selectedAction && model?.actions.includes(selectedAction)) - return; + const selectedAction = React.useMemo(() => { + if (selectedCallId) { + const action = model?.actions.find(a => a.callId === selectedCallId); + if (action) + return action; + } + const failedAction = model?.failedAction(); - if (initialSelection && model?.actions.includes(initialSelection)) { - setSelectedAction(initialSelection); - } else if (failedAction) { - setSelectedAction(failedAction); - } else if (model?.actions.length) { + if (failedAction) + return failedAction; + + if (model?.actions.length) { // Select the last non-after hooks item. let index = model.actions.length - 1; for (let i = 0; i < model.actions.length; ++i) { @@ -109,15 +109,24 @@ export const Workbench: React.FunctionComponent<{ break; } } - setSelectedAction(model.actions[index]); + return model.actions[index]; } - }, [model, selectedAction, setSelectedAction, initialSelection]); + }, [model, selectedCallId]); + + const revealedStack = React.useMemo(() => { + if (revealedError) + return revealedError.stack; + return selectedAction?.stack; + }, [selectedAction, revealedError]); + + const activeAction = React.useMemo(() => { + return highlightedAction || selectedAction; + }, [selectedAction, highlightedAction]); const onActionSelected = React.useCallback((action: modelUtil.ActionTraceEventInContext) => { setSelectedAction(action); setHighlightedAction(undefined); - onSelectionChanged?.(action); - }, [setSelectedAction, onSelectionChanged, setHighlightedAction]); + }, [setSelectedAction, setHighlightedAction]); const selectPropertiesTab = React.useCallback((tab: string) => { setSelectedPropertiesTab(tab); @@ -177,7 +186,7 @@ export const Workbench: React.FunctionComponent<{ if (error.action) setSelectedAction(error.action); else - setRevealedStack(error.stack); + setRevealedError(error); selectPropertiesTab('source'); }} /> }; From 47713e8a66ebae6958260780be0d04b277e71fe9 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 16 Sep 2024 22:47:54 -0700 Subject: [PATCH 11/13] chore: make recorder tests pass in frozen mode (#32645) --- .../playwright-core/src/server/injected/recorder/recorder.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/playwright-core/src/server/injected/recorder/recorder.ts b/packages/playwright-core/src/server/injected/recorder/recorder.ts index 48639fefc8..3ad40d53e2 100644 --- a/packages/playwright-core/src/server/injected/recorder/recorder.ts +++ b/packages/playwright-core/src/server/injected/recorder/recorder.ts @@ -211,7 +211,7 @@ class RecordActionTool implements RecorderTool { private _hoveredElement: HTMLElement | null = null; private _activeModel: HighlightModel | null = null; private _expectProgrammaticKeyUp = false; - private _pendingClickAction: { action: actions.ClickAction, timeout: NodeJS.Timeout } | undefined; + private _pendingClickAction: { action: actions.ClickAction, timeout: number } | undefined; constructor(recorder: Recorder) { this._recorder = recorder; @@ -268,7 +268,7 @@ class RecordActionTool implements RecorderTool { modifiers: modifiersForEvent(event), clickCount: event.detail }, - timeout: setTimeout(() => this._commitPendingClickAction(), 200) + timeout: this._recorder.injectedScript.builtinSetTimeout(() => this._commitPendingClickAction(), 200) }; } } From b0f15b320fddec3e62ec1254111c6875ec484114 Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Tue, 17 Sep 2024 08:37:49 +0200 Subject: [PATCH 12/13] fix(recorder): reattach toolbar if it was unmounted by framework hydration (#32637) Closes https://github.com/microsoft/playwright/issues/32632. A side effect of Remix's hydration implementation is that it throws away the entire DOM. This is broadly discussed in https://github.com/remix-run/remix/issues/4822 - there might be a fix in coming React versions, but who knows. Besides breaking browser extensions, this also deletes our toolbar! This PR fixes it by periodically checking in on `x-pw-glass`, and remounting it if it was unmounted. Hacky but effective! --- .../src/server/injected/highlight.ts | 3 ++- .../src/server/injected/recorder/recorder.ts | 7 +++++++ tests/library/inspector/cli-codegen-3.spec.ts | 17 +++++++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/packages/playwright-core/src/server/injected/highlight.ts b/packages/playwright-core/src/server/injected/highlight.ts index c85216e106..c89df54a6f 100644 --- a/packages/playwright-core/src/server/injected/highlight.ts +++ b/packages/playwright-core/src/server/injected/highlight.ts @@ -90,7 +90,8 @@ export class Highlight { } install() { - this._injectedScript.document.documentElement.appendChild(this._glassPaneElement); + if (!this._injectedScript.document.documentElement.contains(this._glassPaneElement)) + this._injectedScript.document.documentElement.appendChild(this._glassPaneElement); } setLanguage(language: Language) { diff --git a/packages/playwright-core/src/server/injected/recorder/recorder.ts b/packages/playwright-core/src/server/injected/recorder/recorder.ts index 3ad40d53e2..dfbd4608f0 100644 --- a/packages/playwright-core/src/server/injected/recorder/recorder.ts +++ b/packages/playwright-core/src/server/injected/recorder/recorder.ts @@ -1036,7 +1036,14 @@ export class Recorder { addEventListener(this.document, 'focus', event => this._onFocus(event), true), addEventListener(this.document, 'scroll', event => this._onScroll(event), true), ]; + this.highlight.install(); + // some frameworks erase the DOM on hydration, this ensures it's reattached + const recreationInterval = setInterval(() => { + this.highlight.install(); + }, 500); + this._listeners.push(() => clearInterval(recreationInterval)); + this.overlay?.install(); this.document.adoptedStyleSheets.push(this._stylesheet); } diff --git a/tests/library/inspector/cli-codegen-3.spec.ts b/tests/library/inspector/cli-codegen-3.spec.ts index 6828576456..b2f0c39988 100644 --- a/tests/library/inspector/cli-codegen-3.spec.ts +++ b/tests/library/inspector/cli-codegen-3.spec.ts @@ -739,4 +739,21 @@ await page.GetByLabel("Coun\\"try").ClickAsync();`); expect.soft(sources1.get('Java')!.text).toContain(`assertThat(page.getByRole(AriaRole.TEXTBOX)).isVisible()`); expect.soft(sources1.get('C#')!.text).toContain(`await Expect(page.GetByRole(AriaRole.Textbox)).ToBeVisibleAsync()`); }); + + test('should keep toolbar visible even if webpage erases content in hydration', async ({ openRecorder }) => { + const recorder = await openRecorder(); + + const hydrate = () => { + setTimeout(() => { + document.documentElement.innerHTML = '

Post-Hydration Content

'; + }, 500); + }; + await recorder.setContentAndWait(` +

Pre-Hydration Content

+ + `); + + await expect(recorder.page.getByText('Post-Hydration Content')).toBeVisible(); + await expect(recorder.page.locator('x-pw-glass')).toBeVisible(); + }); }); From ec2ae1ed2d37803b0816d558b90d981c5ef7f24b Mon Sep 17 00:00:00 2001 From: Simon Knott Date: Tue, 17 Sep 2024 08:45:44 +0200 Subject: [PATCH 13/13] feat(watch mode): buffer mode (#32631) Closes https://github.com/microsoft/playwright/issues/32578. Adds a buffer mode that can be toggled by pressing b. When engaged, changed test files are collected and shown on screen. The test run is then kicked off by pressing Enter. This changes the signal to start a test run from Cmd+s to a Enter press in the test terminal. It should help users with auto-save and make it easier to run on long-running tests. It feels very similar to running `npx playwright test`, but without having to write a filter. https://github.com/user-attachments/assets/71e16139-9427-4e90-b523-8d218f09ed9d --- packages/playwright/src/runner/watchMode.ts | 64 ++++++++++++++++----- tests/playwright-test/watch.spec.ts | 51 ++++++++++++++++ 2 files changed, 100 insertions(+), 15 deletions(-) diff --git a/packages/playwright/src/runner/watchMode.ts b/packages/playwright/src/runner/watchMode.ts index 603f066601..a47ca0fe32 100644 --- a/packages/playwright/src/runner/watchMode.ts +++ b/packages/playwright/src/runner/watchMode.ts @@ -73,6 +73,7 @@ export async function runWatchModeLoop(configLocation: ConfigLocation, initialOp return 'restarted'; const options: WatchModeOptions = { ...initialOptions }; + let bufferMode = false; const testServerDispatcher = new TestServerDispatcher(configLocation); const transport = new InMemoryTransport( @@ -94,8 +95,9 @@ export async function runWatchModeLoop(configLocation: ConfigLocation, initialOp const teleSuiteUpdater = new TeleSuiteUpdater({ pathSeparator: path.sep, onUpdate() { } }); + const dirtyTestFiles = new Set(); const dirtyTestIds = new Set(); - let onDirtyTests = new ManualPromise(); + let onDirtyTests = new ManualPromise<'changed'>(); let queue = Promise.resolve(); const changedFiles = new Set(); @@ -110,14 +112,17 @@ export async function runWatchModeLoop(configLocation: ConfigLocation, initialOp teleSuiteUpdater.processListReport(report); for (const test of teleSuiteUpdater.rootSuite!.allTests()) { - if (changedFiles.has(test.location.file)) + if (changedFiles.has(test.location.file)) { + dirtyTestFiles.add(test.location.file); dirtyTestIds.add(test.id); + } } - changedFiles.clear(); - if (dirtyTestIds.size > 0) - onDirtyTests.resolve?.(); + if (dirtyTestIds.size > 0) { + onDirtyTests.resolve('changed'); + onDirtyTests = new ManualPromise(); + } }); }); testServerConnection.onReport(report => teleSuiteUpdater.processTestReportEvent(report)); @@ -134,21 +139,27 @@ export async function runWatchModeLoop(configLocation: ConfigLocation, initialOp let result: FullResult['status'] = 'passed'; while (true) { - printPrompt(); - const readCommandPromise = readCommand(); - await Promise.race([ + if (bufferMode) + printBufferPrompt(dirtyTestFiles, teleSuiteUpdater.config!.rootDir); + else + printPrompt(); + + const command = await Promise.race([ onDirtyTests, - readCommandPromise, + readCommand(), ]); - if (!readCommandPromise.isDone()) - readCommandPromise.resolve('changed'); - const command = await readCommandPromise; + if (bufferMode && command === 'changed') + continue; + + const shouldRunChangedFiles = bufferMode ? command === 'run' : command === 'changed'; + if (shouldRunChangedFiles) { + if (dirtyTestIds.size === 0) + continue; - if (command === 'changed') { - onDirtyTests = new ManualPromise(); const testIds = [...dirtyTestIds]; dirtyTestIds.clear(); + dirtyTestFiles.clear(); await runTests(options, testServerConnection, { testIds, title: 'files changed' }); lastRun = { type: 'changed', dirtyTestIds: testIds }; continue; @@ -234,6 +245,11 @@ export async function runWatchModeLoop(configLocation: ConfigLocation, initialOp continue; } + if (command === 'toggle-buffer-mode') { + bufferMode = !bufferMode; + continue; + } + if (command === 'exit') break; @@ -300,6 +316,7 @@ Change settings ${colors.bold('p')} ${colors.dim('set file filter')} ${colors.bold('t')} ${colors.dim('set title filter')} ${colors.bold('s')} ${colors.dim('toggle show & reuse the browser')} + ${colors.bold('b')} ${colors.dim('toggle buffer mode')} `); return; } @@ -312,6 +329,7 @@ Change settings case 't': result.resolve('grep'); break; case 'f': result.resolve('failed'); break; case 's': result.resolve('toggle-show-browser'); break; + case 'b': result.resolve('toggle-buffer-mode'); break; } }; @@ -350,6 +368,22 @@ function printConfiguration(options: WatchModeOptions, title?: string) { process.stdout.write(lines.join('\n')); } +function printBufferPrompt(dirtyTestFiles: Set, rootDir: string) { + const sep = separator(); + process.stdout.write('\x1Bc'); + process.stdout.write(`${sep}\n`); + + if (dirtyTestFiles.size === 0) { + process.stdout.write(`${colors.dim('Waiting for file changes. Press')} ${colors.bold('q')} ${colors.dim('to quit or')} ${colors.bold('h')} ${colors.dim('for more options.')}\n\n`); + return; + } + + process.stdout.write(`${colors.dim(`${dirtyTestFiles.size} test ${dirtyTestFiles.size === 1 ? 'file' : 'files'} changed:`)}\n\n`); + for (const file of dirtyTestFiles) + process.stdout.write(` · ${path.relative(rootDir, file)}\n`); + process.stdout.write(`\n${colors.dim(`Press`)} ${colors.bold('enter')} ${colors.dim('to run')}, ${colors.bold('q')} ${colors.dim('to quit or')} ${colors.bold('h')} ${colors.dim('for more options.')}\n\n`); +} + function printPrompt() { const sep = separator(); process.stdout.write(` @@ -371,4 +405,4 @@ async function toggleShowBrowser() { } } -type Command = 'run' | 'failed' | 'repeat' | 'changed' | 'project' | 'file' | 'grep' | 'exit' | 'interrupted' | 'toggle-show-browser'; +type Command = 'run' | 'failed' | 'repeat' | 'changed' | 'project' | 'file' | 'grep' | 'exit' | 'interrupted' | 'toggle-show-browser' | 'toggle-buffer-mode'; diff --git a/tests/playwright-test/watch.spec.ts b/tests/playwright-test/watch.spec.ts index ec05e5bb68..b56fb470ea 100644 --- a/tests/playwright-test/watch.spec.ts +++ b/tests/playwright-test/watch.spec.ts @@ -313,12 +313,17 @@ test('should respect project filter C', async ({ runWatchTest, writeFiles }) => await testProcess.waitForOutput('[foo] › a.test.ts:3:11 › passes'); expect(testProcess.output).not.toContain('[bar] › a.test.ts:3:11 › passes'); + await testProcess.waitForOutput('Waiting for file changes.'); testProcess.clearOutput(); await writeFiles(files); // file change triggers listTests with project filter await testProcess.waitForOutput('[foo] › a.test.ts:3:11 › passes'); + testProcess.clearOutput(); + await testProcess.waitForOutput('Waiting for file changes.'); + testProcess.write('c'); + testProcess.clearOutput(); await testProcess.waitForOutput('Select projects'); await testProcess.waitForOutput('foo'); await testProcess.waitForOutput('bar'); // second selection should still show all @@ -812,3 +817,49 @@ test('should run global teardown before exiting', async ({ runWatchTest }) => { testProcess.write('\x1B'); await testProcess.waitForOutput('running teardown'); }); + +test('buffer mode', async ({ runWatchTest, writeFiles }) => { + const testProcess = await runWatchTest({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('passes', () => {}); + `, + 'b.test.ts': ` + import { test, expect } from '@playwright/test'; + test('passes in b', () => {}); + `, + }); + + testProcess.clearOutput(); + testProcess.write('b'); + await testProcess.waitForOutput('Waiting for file changes. Press q to quit'); + + + testProcess.clearOutput(); + await writeFiles({ + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('passes again', () => {}); + `, + }); + + await testProcess.waitForOutput('1 test file changed:'); + await testProcess.waitForOutput('a.test.ts'); + + testProcess.clearOutput(); + await writeFiles({ + 'b.test.ts': ` + import { test, expect } from '@playwright/test'; + test('passes in b again', () => {}); + `, + }); + await testProcess.waitForOutput('2 test files changed:'); + await testProcess.waitForOutput('a.test.ts'); + await testProcess.waitForOutput('b.test.ts'); + + testProcess.clearOutput(); + testProcess.write('\r\n'); + + await testProcess.waitForOutput('a.test.ts:3:11 › passes'); + await testProcess.waitForOutput('b.test.ts:3:11 › passes'); +}); \ No newline at end of file