From d36bffb9a938146a297d4bdbe0735d7e5ea7dc7b Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 26 May 2021 15:18:52 -0700 Subject: [PATCH] fix(connect): respect timeout in all scenarios (#6762) Drive-by: fix console error in test arising from bad usage of test websocket server in `connect()` calls. --- src/client/browserType.ts | 14 +++++++++++++- tests/browsertype-connect.spec.ts | 11 ++++++++++- tests/capabilities.spec.ts | 1 + tests/chromium/chromium.spec.ts | 6 ++++-- tests/ignorehttpserrors.spec.ts | 1 + tests/web-socket.spec.ts | 5 +++++ utils/testserver/index.d.ts | 1 + utils/testserver/index.js | 11 ++++++++--- 8 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/client/browserType.ts b/src/client/browserType.ts index 10122efd08..d38809c05f 100644 --- a/src/client/browserType.ts +++ b/src/client/browserType.ts @@ -143,7 +143,12 @@ export class BrowserType extends ChannelOwner(async (fulfill, reject) => { + + let timeoutCallback = (e: Error) => {}; + const timeoutPromise = new Promise((f, r) => timeoutCallback = r); + const timer = params.timeout ? setTimeout(() => timeoutCallback(new Error(`Timeout ${params.timeout}ms exceeded.`)), params.timeout) : undefined; + + const successPromise = new Promise(async (fulfill, reject) => { if ((params as any).__testHookBeforeCreateBrowser) { try { await (params as any).__testHookBeforeCreateBrowser(); @@ -202,6 +207,13 @@ export class BrowserType extends ChannelOwner { + const e = await browserType.connect({ + wsEndpoint: `ws://localhost:${server.PORT}/ws`, + timeout: 100, + }).catch(e => e); + expect(e.message).toContain('browserType.connect: Timeout 100ms exceeded.'); +}); + test('should send extra headers with connect request', async ({browserType, startRemoteServer, server}) => { const [request] = await Promise.all([ server.waitForWebSocketConnectionRequest(), @@ -70,7 +78,8 @@ test('should send extra headers with connect request', async ({browserType, star headers: { 'User-Agent': 'Playwright', 'foo': 'bar', - } + }, + timeout: 100, }).catch(() => {}) ]); expect(request.headers['user-agent']).toBe('Playwright'); diff --git a/tests/capabilities.spec.ts b/tests/capabilities.spec.ts index 934db5ad53..3789b0b137 100644 --- a/tests/capabilities.spec.ts +++ b/tests/capabilities.spec.ts @@ -26,6 +26,7 @@ it('Web Assembly should work', async function({page, server, browserName, platfo }); it('WebSocket should work', async ({page, server}) => { + server.sendOnWebSocketConnection('incoming'); const value = await page.evaluate(port => { let cb; const result = new Promise(f => cb = f); diff --git a/tests/chromium/chromium.spec.ts b/tests/chromium/chromium.spec.ts index 6da9a0930b..af5e43a406 100644 --- a/tests/chromium/chromium.spec.ts +++ b/tests/chromium/chromium.spec.ts @@ -217,7 +217,8 @@ playwrightTest('should send extra headers with connect request', async ({browser headers: { 'User-Agent': 'Playwright', 'foo': 'bar', - } + }, + timeout: 100, }).catch(() => {}) ]); expect(request.headers['user-agent']).toBe('Playwright'); @@ -231,7 +232,8 @@ playwrightTest('should send extra headers with connect request', async ({browser headers: { 'User-Agent': 'Playwright', 'foo': 'bar', - } + }, + timeout: 100, }).catch(() => {}) ]); expect(request.headers['user-agent']).toBe('Playwright'); diff --git a/tests/ignorehttpserrors.spec.ts b/tests/ignorehttpserrors.spec.ts index a684ae8263..5fb6b58143 100644 --- a/tests/ignorehttpserrors.spec.ts +++ b/tests/ignorehttpserrors.spec.ts @@ -63,6 +63,7 @@ it('should work with mixed content', async ({browser, server, httpsServer}) => { }); it('should work with WebSocket', async ({browser, httpsServer}) => { + httpsServer.sendOnWebSocketConnection('incoming'); const context = await browser.newContext({ ignoreHTTPSErrors: true }); const page = await context.newPage(); const value = await page.evaluate(endpoint => { diff --git a/tests/web-socket.spec.ts b/tests/web-socket.spec.ts index 76a8080c7e..5cbd4f653e 100644 --- a/tests/web-socket.spec.ts +++ b/tests/web-socket.spec.ts @@ -19,6 +19,7 @@ import { contextTest as it, expect } from './config/browserTest'; import { Server as WebSocketServer } from 'ws'; it('should work', async ({ page, server }) => { + server.sendOnWebSocketConnection('incoming'); const value = await page.evaluate(port => { let cb; const result = new Promise(f => cb = f); @@ -49,6 +50,7 @@ it('should emit close events', async ({ page, server }) => { }); it('should emit frame events', async ({ page, server }) => { + server.sendOnWebSocketConnection('incoming'); let socketClosed; const socketClosePromise = new Promise(f => socketClosed = f); const log = []; @@ -126,6 +128,7 @@ it('should emit error', async ({page, server, browserName}) => { }); it('should not have stray error events', async ({page, server}) => { + server.sendOnWebSocketConnection('incoming'); let error; page.on('websocket', ws => ws.on('socketerror', e => error = e)); await Promise.all([ @@ -142,6 +145,7 @@ it('should not have stray error events', async ({page, server}) => { }); it('should reject waitForEvent on socket close', async ({page, server}) => { + server.sendOnWebSocketConnection('incoming'); const [ws] = await Promise.all([ page.waitForEvent('websocket').then(async ws => { await ws.waitForEvent('framereceived'); @@ -157,6 +161,7 @@ it('should reject waitForEvent on socket close', async ({page, server}) => { }); it('should reject waitForEvent on page close', async ({page, server}) => { + server.sendOnWebSocketConnection('incoming'); const [ws] = await Promise.all([ page.waitForEvent('websocket').then(async ws => { await ws.waitForEvent('framereceived'); diff --git a/utils/testserver/index.d.ts b/utils/testserver/index.d.ts index 56779ae93e..b37fb23db2 100644 --- a/utils/testserver/index.d.ts +++ b/utils/testserver/index.d.ts @@ -29,6 +29,7 @@ export class TestServer { setRedirect(from: string, to: string); waitForRequest(path: string): Promise; waitForWebSocketConnectionRequest(): Promise; + sendOnWebSocketConnection(data: string); reset(); serveFile(request: IncomingMessage, response: ServerResponse); serveFile(request: IncomingMessage, response: ServerResponse, filePath: string); diff --git a/utils/testserver/index.js b/utils/testserver/index.js index d7e9b0e6fb..def8c362ff 100644 --- a/utils/testserver/index.js +++ b/utils/testserver/index.js @@ -71,7 +71,10 @@ class TestServer { this._server = http.createServer(this._onRequest.bind(this)); this._server.on('connection', socket => this._onSocket(socket)); this._wsServer = new WebSocketServer({server: this._server, path: '/ws'}); - this._wsServer.on('connection', this._onWebSocketConnection.bind(this)); + this._wsServer.on('connection', ws => { + if (this._onWebSocketConnectionData !== undefined) + ws.send(this._onWebSocketConnectionData); + }); this._server.listen(port); this._dirPath = dirPath; this.debugServer = require('debug')('pw:server'); @@ -91,6 +94,8 @@ class TestServer { this._gzipRoutes = new Set(); /** @type {!Map} */ this._requestSubscribers = new Map(); + /** @type {string|undefined} */ + this._onWebSocketConnectionData = undefined; const cross_origin = loopback || '127.0.0.1'; const same_origin = loopback || 'localhost'; @@ -299,8 +304,8 @@ class TestServer { }); } - _onWebSocketConnection(ws) { - ws.send('incoming'); + sendOnWebSocketConnection(data) { + this._onWebSocketConnectionData = data; } }