chore: add _browserTypes helpers to playwright #34611

Skn0tt · 2025-02-04T15:11:07Z

There's a couple of spots throughout the client where we iterate through all browser types and pages. This PR extract some logic onto Playwright. This came up earlier today, while discussing with Dima. We already have similar logic on the Playwright server side.

dgozman · 2025-02-04T15:20:38Z

packages/playwright/src/index.ts

-    for (const browserType of [playwright.chromium, playwright.firefox, playwright.webkit, playwright._bidiChromium, playwright._bidiFirefox])
-      (browserType as any)._defaultLaunchOptions = options;
+    for (const browserType of playwright._browserTypes())
+      browserType._defaultLaunchOptions = options;


Let's move these to playwright right away!

dgozman · 2025-02-04T15:22:49Z

packages/playwright/src/index.ts

-      contexts.push(...(browserType as any)._contexts);
-    const pages = contexts.map(ctx => ctx.pages()).flat();
-    await Promise.all(pages.map(page => this._screenshotPage(page, false)));
+    await Promise.all(this._playwright._allPages().map(page => this._screenshotPage(page, false)));


What about places like this one: https://github.com/microsoft/playwright/blob/main/packages/playwright/src/index.ts#L625?

dgozman · 2025-02-04T16:32:50Z

packages/playwright/src/index.ts

-      (browserType as any)._defaultContextNavigationTimeout = navigationTimeout || 0;
+
+    for (const browserType of playwright._browserTypes()) {
+      browserType._defaultContextOptions = _combinedContextOptions;


Might as well move these to Playwright.

dgozman · 2025-02-04T16:33:02Z

packages/playwright/src/index.ts

-    (playwright.request as any)._defaultContextOptions = { ..._combinedContextOptions };
-    (playwright.request as any)._defaultContextOptions.tracesDir = tracing().tracesDir();
-    (playwright.request as any)._defaultContextOptions.timeout = actionTimeout || 0;
+    playwright.request._defaultContextOptions = { ..._combinedContextOptions };


And this one.

dgozman

Almost there!

dgozman · 2025-02-06T10:51:50Z

packages/playwright-core/src/client/fetch.ts

@@ -47,7 +47,7 @@ export type FetchOptions = {

 type NewContextOptions = Omit<channels.PlaywrightNewRequestOptions, 'extraHTTPHeaders' | 'clientCertificates' | 'storageState' | 'tracesDir'> & {
  extraHTTPHeaders?: Headers,
-  storageState?: string | StorageState,
+  storageState?: string | SetStorageState,


This change seems unrelated, please move it into some IndexedDB pr instead?

It's related! NewContextOptions is for writing, so it needs to use SetStorageState. This should've been fixed earlier, but was always covered because we had all these (... as any)s in the code.

packages/playwright-core/src/client/playwright.ts

dgozman · 2025-02-06T10:52:35Z

packages/playwright-core/src/client/playwright.ts

@@ -73,4 +82,16 @@ export class Playwright extends ChannelOwner<channels.PlaywrightChannel> {
  static from(channel: channels.PlaywrightChannel): Playwright {
    return (channel as any)._object;
  }
+
+  _browserTypes(): BrowserType[] {


Do we still need this one?

at least for the lines below - I made it private for now.

packages/playwright/src/index.ts

packages/playwright-core/src/client/playwright.ts

tests/config/remoteServer.ts

tests/library/browsertype-connect.spec.ts

tests/library/tracing.spec.ts

dgozman · 2025-02-06T12:06:53Z

packages/playwright-core/src/client/browserType.ts

    const launchOptions: channels.BrowserTypeLaunchParams = {
      ...options,
+      tracesDir: options.tracesDir ?? this._playwright._defaultTracesDir,


I think it's better update options variable at line 67 with the new tracesDir instead of changing launchOptions, because options is later used at line 77 and so we want all the values there be correct.

Since I moved tracesDir back into _defaultLaunchOptions, things should be good now.

dgozman · 2025-02-06T12:07:47Z

packages/playwright-core/src/client/browserType.ts

    assert(!(options as any).port, 'Cannot specify a port without launching as a server.');
-    options = { ...this._defaultLaunchOptions, ...this._defaultContextOptions, ...options };
+    options = { ...this._playwright._defaultLaunchOptions, ...this._playwright._defaultContextOptions, ...options };


Missing tracesDir here.

i've moved tracesDir back into _defaultLaunchOptions, so it should work now

packages/playwright-core/src/client/fetch.ts

tests/config/remoteServer.ts

dgozman · 2025-02-06T12:16:51Z

packages/playwright/src/index.ts

@@ -81,13 +83,11 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
      options.headless = headless;
    if (channel !== undefined)
      options.channel = channel;
-    options.tracesDir = tracing().tracesDir();


I see that _browserOptions is also used for connect, and we are now missing tracesDir there. Perhaps it was not the best idea to plumb it separately, sorry.

I've tangled tracesDir back into _defaultLaunchOptions :D Hoping that works.

dgozman

Looks good with just a single comment.

dgozman · 2025-02-06T17:59:14Z

packages/playwright/src/index.ts

-    for (const browserType of [playwright.chromium, playwright.firefox, playwright.webkit, playwright._bidiChromium, playwright._bidiFirefox])
-      (browserType as any)._defaultLaunchOptions = options;
+    playwright._defaultLaunchOptions = options;
+    playwright._defaultLaunchOptions.tracesDir = tracing().tracesDir();


Now this makes options exposed in the next line missing the tracesDir 😄

github-actions · 2025-02-07T14:45:41Z

Test results for "tests 1"

1 failed
❌ [webkit-page] › tests/page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18

7 flaky

⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [chromium-library] › tests/library/video.spec.ts:379:5 › screencast › should capture navigation @ubuntu-20.04-chromium-tip-of-tree
⚠️ [webkit-library] › tests/library/proxy.spec.ts:44:3 › should use proxy for second page @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/locator-misc-2.spec.ts:101:3 › should take screenshot @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:161:5 › waitFor should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:147:3 › should upload large file @webkit-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

37814 passed, 655 skipped
✔️✔️✔️

Merge workflow run.

chore: add _browserTypes helpers to playwright

447cda4

Skn0tt requested a review from dgozman February 4, 2025 15:11

Skn0tt self-assigned this Feb 4, 2025

dgozman reviewed Feb 4, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

more

0dea2f4

dgozman reviewed Feb 4, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

Skn0tt added 2 commits February 6, 2025 10:46

move some more

8a22302

(browserType as any)

a8795ce

This comment has been minimized.

Sign in to view

Skn0tt added 2 commits February 6, 2025 11:00

Merge branch 'main' into refactor-all-pages

e03cb28

fix types

9348dd4

dgozman reviewed Feb 6, 2025

View reviewed changes

apply

0c70096

dgozman approved these changes Feb 6, 2025

View reviewed changes

fix typo

f90e1c5

This comment has been minimized.

Sign in to view

apply

c7b95f1

This comment has been minimized.

Sign in to view

fix timeout precedence

f944f54

This comment has been minimized.

Sign in to view

fix timeout

ffc0ea9

dgozman approved these changes Feb 6, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

Skn0tt added 2 commits February 7, 2025 14:42

move tracesDir

ff45677

Merge branch 'main' into refactor-all-pages

baac04f

Skn0tt merged commit 893e7bb into microsoft:main Feb 7, 2025
28 of 29 checks passed

playwrightmachine mentioned this pull request Feb 7, 2025

[Ports]: Backport client side changes for 1.51 microsoft/playwright-python#2717

Open

45 tasks

This was referenced Feb 7, 2025

[Ports]: Backport client side changes for 1.51 microsoft/playwright-java#1731

Open

[Ports]: Backport client side changes for 1.51 microsoft/playwright-dotnet#3096

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add _browserTypes helpers to playwright #34611

chore: add _browserTypes helpers to playwright #34611

Skn0tt commented Feb 4, 2025 •

edited

Loading

dgozman Feb 4, 2025

dgozman Feb 4, 2025

This comment has been minimized.

dgozman Feb 4, 2025

dgozman Feb 4, 2025

This comment has been minimized.

This comment has been minimized.

dgozman left a comment

dgozman Feb 6, 2025

Skn0tt Feb 6, 2025

dgozman Feb 6, 2025

Skn0tt Feb 6, 2025

dgozman Feb 6, 2025

Skn0tt Feb 6, 2025

dgozman Feb 6, 2025

Skn0tt Feb 6, 2025

dgozman Feb 6, 2025

Skn0tt Feb 6, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dgozman left a comment

dgozman Feb 6, 2025

This comment has been minimized.

github-actions bot commented Feb 7, 2025

chore: add _browserTypes helpers to playwright #34611

chore: add _browserTypes helpers to playwright #34611

Conversation

Skn0tt commented Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

dgozman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dgozman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

github-actions bot commented Feb 7, 2025

Test results for "tests 1"

Skn0tt commented Feb 4, 2025 •

edited

Loading