From 7ab0c10d7b6166c4292823a070686bf7d4fda0c4 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 14 Sep 2020 14:43:39 -0700 Subject: [PATCH] fix(launchServer): do not throw when 'port' option is present (#3877) We now use 'launch' under the hood, which erroneously throws when 'port' is present. Instead, moved validation to the client side where it belongs, added tests for validation errors. --- src/client/browserType.ts | 3 +-- src/server/browserType.ts | 5 +---- test/browsertype-launch-server.spec.ts | 6 ++++++ test/browsertype-launch.spec.ts | 14 +++++++++++++- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/client/browserType.ts b/src/client/browserType.ts index d0c3158896ec6..824a59804f4bd 100644 --- a/src/client/browserType.ts +++ b/src/client/browserType.ts @@ -63,7 +63,6 @@ export class BrowserType extends ChannelOwner { const logger = options.logger; - options = { ...options, logger: undefined }; return this._wrapApiCall('browserType.launch', async () => { 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.'); @@ -87,8 +86,8 @@ export class BrowserType extends ChannelOwner { const logger = options.logger; - options = { ...options, logger: undefined }; return this._wrapApiCall('browserType.launchPersistentContext', async () => { + assert(!(options as any).port, 'Cannot specify a port without launching as a server.'); if (options.extraHTTPHeaders) validateHeaders(options.extraHTTPHeaders); const persistentOptions: channels.BrowserTypeLaunchPersistentContextParams = { diff --git a/src/server/browserType.ts b/src/server/browserType.ts index 088fc23e8773b..b353cdb7da2b0 100644 --- a/src/server/browserType.ts +++ b/src/server/browserType.ts @@ -28,7 +28,7 @@ import { Progress, ProgressController } from './progress'; import * as types from './types'; import { TimeoutSettings } from '../utils/timeoutSettings'; import { validateHostRequirements } from './validateDependencies'; -import { assert, isDebugMode } from '../utils/utils'; +import { isDebugMode } from '../utils/utils'; const mkdirAsync = util.promisify(fs.mkdir); const mkdtempAsync = util.promisify(fs.mkdtemp); @@ -65,8 +65,6 @@ export abstract class BrowserType { } async launch(options: types.LaunchOptions = {}): Promise { - 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.'); options = validateLaunchOptions(options); const controller = new ProgressController(TimeoutSettings.timeout(options)); controller.setLogName('browser'); @@ -75,7 +73,6 @@ export abstract class BrowserType { } async launchPersistentContext(userDataDir: string, options: types.LaunchPersistentOptions = {}): Promise { - assert(!(options as any).port, 'Cannot specify a port without launching as a server.'); options = validateLaunchOptions(options); const persistent: types.BrowserContextOptions = options; validateBrowserContextOptions(persistent); diff --git a/test/browsertype-launch-server.spec.ts b/test/browsertype-launch-server.spec.ts index 423213e55dd17..26c4b8ceb87ab 100644 --- a/test/browsertype-launch-server.spec.ts +++ b/test/browsertype-launch-server.spec.ts @@ -26,6 +26,12 @@ describe('lauch server', suite => { await browserServer.close(); }); + it('should work with port', async ({browserType, defaultBrowserOptions, parallelIndex}) => { + const browserServer = await browserType.launchServer({ ...defaultBrowserOptions, port: 8800 + parallelIndex }); + expect(browserServer.wsEndpoint()).toContain(String(8800 + parallelIndex)); + await browserServer.close(); + }); + it('should fire "close" event during kill', async ({browserType, defaultBrowserOptions}) => { const order = []; const browserServer = await browserType.launchServer(defaultBrowserOptions); diff --git a/test/browsertype-launch.spec.ts b/test/browsertype-launch.spec.ts index c02d54d9b4dbf..9178ffd24120c 100644 --- a/test/browsertype-launch.spec.ts +++ b/test/browsertype-launch.spec.ts @@ -33,7 +33,19 @@ it('should throw if userDataDir option is passed', async ({browserType, defaultB let waitError = null; const options = Object.assign({}, defaultBrowserOptions, {userDataDir: 'random-path'}); await browserType.launch(options).catch(e => waitError = e); - expect(waitError.message).toContain('launchPersistentContext'); + expect(waitError.message).toContain('userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead'); +}); + +it('should throw if port option is passed', async ({browserType, defaultBrowserOptions}) => { + const options = Object.assign({}, defaultBrowserOptions, {port: 1234}); + const error = await browserType.launch(options).catch(e => e); + expect(error.message).toContain('Cannot specify a port without launching as a server.'); +}); + +it('should throw if port option is passed for persistent context', async ({browserType, defaultBrowserOptions}) => { + const options = Object.assign({}, defaultBrowserOptions, {port: 1234}); + const error = await browserType.launchPersistentContext('foo', options).catch(e => e); + expect(error.message).toContain('Cannot specify a port without launching as a server.'); }); it('should throw if page argument is passed', (test, parameters) => {