Skip to content

Commit

Permalink
fix: remove connectInsteadOfLaunching (#26828)
Browse files Browse the repository at this point in the history
  • Loading branch information
dgozman committed Aug 31, 2023
1 parent b387249 commit 6d85ba1
Show file tree
Hide file tree
Showing 5 changed files with 17 additions and 61 deletions.
20 changes: 1 addition & 19 deletions packages/playwright-core/src/client/browserType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { Connection } from './connection';
import { Events } from './events';
import type { ChildProcess } from 'child_process';
import { envObjectToArray } from './clientHelper';
import { jsonStringifyForceASCII, assert, headersObjectToArray, monotonicTime } from '../utils';
import { assert, headersObjectToArray, monotonicTime } from '../utils';
import type * as api from '../../types/types';
import { kBrowserClosedError } from '../common/errors';
import { raceAgainstDeadline } from '../utils/timeoutRunner';
Expand Down Expand Up @@ -51,7 +51,6 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel> imple
_defaultContextTimeout?: number;
_defaultContextNavigationTimeout?: number;
private _defaultLaunchOptions?: LaunchOptions;
private _defaultConnectOptions?: ConnectOptions;

static from(browserType: channels.BrowserTypeChannel): BrowserType {
return (browserType as any)._object;
Expand All @@ -71,9 +70,6 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel> imple
assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead');
assert(!(options as any).port, 'Cannot specify a port without launching as a server.');

if (this._defaultConnectOptions)
return await this._connectInsteadOfLaunching(this._defaultConnectOptions, options);

const logger = options.logger || this._defaultLaunchOptions?.logger;
options = { ...this._defaultLaunchOptions, ...options };
const launchOptions: channels.BrowserTypeLaunchParams = {
Expand All @@ -89,20 +85,6 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel> imple
});
}

private async _connectInsteadOfLaunching(connectOptions: ConnectOptions, launchOptions: LaunchOptions): Promise<Browser> {
return this._connect({
wsEndpoint: connectOptions.wsEndpoint,
headers: {
// HTTP headers are ASCII only (not UTF-8).
'x-playwright-launch-options': jsonStringifyForceASCII({ ...this._defaultLaunchOptions, ...launchOptions }),
...connectOptions.headers,
},
exposeNetwork: connectOptions.exposeNetwork ?? connectOptions._exposeNetwork,
slowMo: connectOptions.slowMo,
timeout: connectOptions.timeout ?? 3 * 60 * 1000, // 3 minutes
});
}

async launchServer(options: LaunchServerOptions = {}): Promise<api.BrowserServer> {
if (!this._serverLauncher)
throw new Error('Launching server is not supported');
Expand Down
25 changes: 8 additions & 17 deletions packages/playwright-test/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
channel: [({ launchOptions }, use) => use(launchOptions.channel), { scope: 'worker', option: true }],
launchOptions: [{}, { scope: 'worker', option: true }],
connectOptions: [async ({}, use) => {
// Usually, when connect options are specified (e.g, in the config or in the environment),
// all launch() calls are turned into connect() calls.
// However, when running in "reuse browser" mode and connecting to the reusable server,
// only the default "browser" fixture should turn into reused browser.
await use(process.env.PW_TEST_REUSE_CONTEXT ? undefined : connectOptionsFromEnv());
await use(connectOptionsFromEnv());
}, { scope: 'worker', option: true }],
screenshot: ['off', { scope: 'worker', option: true }],
video: ['off', { scope: 'worker', option: true }],
Expand All @@ -81,7 +77,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
await use(process.env.TEST_ARTIFACTS_DIR!);
}, { scope: 'worker', _title: 'playwright configuration' } as any],

_browserOptions: [async ({ playwright, headless, channel, launchOptions, connectOptions, _artifactsDir }, use) => {
_browserOptions: [async ({ playwright, headless, channel, launchOptions, _artifactsDir }, use) => {
const options: LaunchOptions = {
handleSIGINT: false,
...launchOptions,
Expand All @@ -92,28 +88,23 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
options.channel = channel;
options.tracesDir = path.join(_artifactsDir, 'traces');

for (const browserType of [playwright.chromium, playwright.firefox, playwright.webkit]) {
for (const browserType of [playwright.chromium, playwright.firefox, playwright.webkit])
(browserType as any)._defaultLaunchOptions = options;
(browserType as any)._defaultConnectOptions = connectOptions;
}
await use(options);
for (const browserType of [playwright.chromium, playwright.firefox, playwright.webkit]) {
for (const browserType of [playwright.chromium, playwright.firefox, playwright.webkit])
(browserType as any)._defaultLaunchOptions = undefined;
(browserType as any)._defaultConnectOptions = undefined;
}
}, { scope: 'worker', auto: true }],

browser: [async ({ playwright, browserName, _browserOptions }, use, testInfo) => {
browser: [async ({ playwright, browserName, _browserOptions, connectOptions }, use, testInfo) => {
if (!['chromium', 'firefox', 'webkit'].includes(browserName))
throw new Error(`Unexpected browserName "${browserName}", must be one of "chromium", "firefox" or "webkit"`);

// Support for "reuse browser" mode.
const connectOptions = connectOptionsFromEnv();
if (connectOptions && process.env.PW_TEST_REUSE_CONTEXT) {
if (connectOptions) {
const browser = await playwright[browserName].connect({
...connectOptions,
exposeNetwork: connectOptions.exposeNetwork ?? (connectOptions as any)._exposeNetwork,
headers: {
'x-playwright-reuse-context': '1',
...(process.env.PW_TEST_REUSE_CONTEXT ? { 'x-playwright-reuse-context': '1' } : {}),
// HTTP headers are ASCII only (not UTF-8).
'x-playwright-launch-options': jsonStringifyForceASCII(_browserOptions),
...connectOptions.headers,
Expand Down
16 changes: 0 additions & 16 deletions tests/library/browsertype-connect.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -698,22 +698,6 @@ for (const kind of ['launchServer', 'run-server'] as const) {
await Promise.all([uploadFile, file1.filepath].map(fs.promises.unlink));
});

test('should connect when launching', async ({ browserType, startRemoteServer }) => {
const remoteServer = await startRemoteServer(kind);
(browserType as any)._defaultConnectOptions = {
wsEndpoint: remoteServer.wsEndpoint()
};

const browser = await browserType.launch();

await Promise.all([
new Promise(f => browser.on('disconnected', f)),
remoteServer.close(),
]);

(browserType as any)._defaultConnectOptions = undefined;
});

test('should connect over http', async ({ connect, startRemoteServer, mode }) => {
test.skip(mode !== 'default');
const remoteServer = await startRemoteServer(kind);
Expand Down
13 changes: 6 additions & 7 deletions tests/library/debug-controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,12 @@ const test = baseTest.extend<Fixtures>({
connectedBrowserFactory: async ({ wsEndpoint, browserType }, use) => {
const browsers: BrowserWithReuse [] = [];
await use(async () => {
const oldValue = (browserType as any)._defaultConnectOptions;
(browserType as any)._defaultConnectOptions = {
wsEndpoint,
headers: { 'x-playwright-reuse-context': '1', },
};
const browser = await browserType.launch() as BrowserWithReuse;
(browserType as any)._defaultConnectOptions = oldValue;
const browser = await browserType.connect(wsEndpoint, {
headers: {
'x-playwright-launch-options': JSON.stringify((browserType as any)._defaultLaunchOptions),
'x-playwright-reuse-context': '1',
},
}) as BrowserWithReuse;
browsers.push(browser);
return browser;
});
Expand Down
4 changes: 2 additions & 2 deletions tests/playwright-test/playwright.connect.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ test('should throw with bad connectOptions', async ({ runInlineTest }) => {
});
expect(result.exitCode).toBe(1);
expect(result.passed).toBe(0);
expect(result.output).toContain('browserType.launch:');
expect(result.output).toContain('browserType.connect:');
expect(result.output).toContain('does-not-exist-bad-domain');
});

Expand All @@ -124,7 +124,7 @@ test('should respect connectOptions.timeout', async ({ runInlineTest }) => {
});
expect(result.exitCode).toBe(1);
expect(result.passed).toBe(0);
expect(result.output).toContain('browserType.launch: Timeout 1ms exceeded.');
expect(result.output).toContain('browserType.connect: Timeout 1ms exceeded.');
});

test('should print debug log when failed to connect', async ({ runInlineTest }) => {
Expand Down

0 comments on commit 6d85ba1

Please sign in to comment.