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.
microsoft · Sep 14, 2020 · 7ab0c10 · 7ab0c10
1 parent 01198f8
commit 7ab0c10
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 7 deletions.
diff --git a/src/client/browserType.ts b/src/client/browserType.ts
@@ -63,7 +63,6 @@ export class BrowserType extends ChannelOwner<channels.BrowserTypeChannel, chann
 
   async launch(options: LaunchOptions = {}): Promise<Browser> {
     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<channels.BrowserTypeChannel, chann
 
   async launchPersistentContext(userDataDir: string, options: LaunchPersistentContextOptions = {}): Promise<BrowserContext> {
     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
@@ -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<Browser> {
-    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<BrowserContext> {
-    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
@@ -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
@@ -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) => {