From e1699ea39662e817ff0faa8d851d79a0e0f19cdc Mon Sep 17 00:00:00 2001 From: vigneshshanmugam Date: Thu, 23 Mar 2023 23:20:26 -0700 Subject: [PATCH 1/4] fix: drop network events once test runs to completion --- __tests__/plugins/network.test.ts | 23 ++++++++--------- src/core/gatherer.ts | 22 ++++++++++++---- src/core/index.ts | 4 +-- src/core/logger.ts | 2 +- src/plugins/network.ts | 42 ++++++++++++++++--------------- 5 files changed, 52 insertions(+), 41 deletions(-) diff --git a/__tests__/plugins/network.test.ts b/__tests__/plugins/network.test.ts index a3f0e6b6..f1281fe4 100644 --- a/__tests__/plugins/network.test.ts +++ b/__tests__/plugins/network.test.ts @@ -311,15 +311,7 @@ describe('network', () => { res.writeHead(200, { 'Content-Type': 'text/html', }); - res.end(` - `); + res.end(''); }); server.route('/chunked.txt', async (req, res) => { res.writeHead(200, { @@ -330,11 +322,16 @@ describe('network', () => { // response is never ended }); - driver.page.on('console', (msg: any) => { - console.log('console', msg.text()); - }); await driver.page.goto(server.PREFIX + '/index'); - await driver.page.evaluate(() => (window as any).delayedPromise); + await Promise.all([ + driver.page.evaluate(() => { + // hangs forever + const x = new XMLHttpRequest(); + x.open('GET', 'chunked.txt', true); + x.send(); + }, [server.PREFIX + 'chunked.txt']), + delay(1000), + ]); const netinfo = await network.stop(); await Gatherer.stop(); expect(netinfo.length).toBe(2); diff --git a/src/core/gatherer.ts b/src/core/gatherer.ts index 958a7c0d..0339a9df 100644 --- a/src/core/gatherer.ts +++ b/src/core/gatherer.ts @@ -39,6 +39,7 @@ import { Driver, NetworkConditions, RunOptions } from '../common_types'; * related capabilities for the runner to run all journeys */ export class Gatherer { + static isClosing = false; static browser: ChromiumBrowser; static async setupDriver(options: RunOptions): Promise { @@ -72,6 +73,12 @@ export class Gatherer { const page = await context.newPage(); const client = await context.newCDPSession(page); const request = await apiRequest.newContext({ ...playwrightOptions }); + + // Register sig int handler to close the browser + process.on('SIGINT', async () => { + await Gatherer.closeBrowser(); + process.exit(130); + }); return { browser: Gatherer.browser, context, page, client, request }; } @@ -108,6 +115,15 @@ export class Gatherer { } } + static async closeBrowser() { + log(`Gatherer: closing browser`); + if (Gatherer.browser && !Gatherer.isClosing) { + Gatherer.isClosing = true; + await Gatherer.browser.close(); + Gatherer.browser = null; + } + } + /** * Starts recording all events related to the v8 devtools protocol * https://chromedevtools.github.io/devtools-protocol/v8/ @@ -131,10 +147,6 @@ export class Gatherer { } static async stop() { - log(`Gatherer: closing browser`); - if (Gatherer.browser) { - await Gatherer.browser.close(); - Gatherer.browser = null; - } + await Gatherer.closeBrowser(); } } diff --git a/src/core/index.ts b/src/core/index.ts index ce806e6a..b6bb81cf 100644 --- a/src/core/index.ts +++ b/src/core/index.ts @@ -47,7 +47,7 @@ export const journey = wrapFnWithLocation( options: JourneyOptions | string, callback: JourneyCallback ) => { - log(`register journey: ${JSON.stringify(options)}`); + log(`Journey register: ${JSON.stringify(options)}`); if (typeof options === 'string') { options = { name: options, id: options }; } @@ -59,7 +59,7 @@ export const journey = wrapFnWithLocation( export const step = wrapFnWithLocation( (location: Location, name: string, callback: VoidCallback) => { - log(`register step: ${name}`); + log(`Step register: ${name}`); return runner.currentJourney?.addStep(name, callback, location); } ); diff --git a/src/core/logger.ts b/src/core/logger.ts index d123ef20..bb28e4d5 100644 --- a/src/core/logger.ts +++ b/src/core/logger.ts @@ -41,5 +41,5 @@ export function log(msg) { msg = JSON.stringify(msg); } const time = dim(cyan(`at ${parseInt(String(now()))} ms `)); - console.log(time + italic(grey(msg))); + process.stderr.write(time + italic(grey(msg)) + '\n'); } diff --git a/src/plugins/network.ts b/src/plugins/network.ts index e21e8e50..c1fd53f3 100644 --- a/src/plugins/network.ts +++ b/src/plugins/network.ts @@ -180,31 +180,25 @@ export class NetworkManager { networkEntry.response.mimeType = contentType.split(';')[0]; }) ); + this._addBarrier( + page, + response.serverAddr().then(server => { + networkEntry.response.remoteIPAddress = server?.ipAddress; + networkEntry.response.remotePort = server?.port; + }) + ); + this._addBarrier( + page, + response.securityDetails().then(details => { + if (details) networkEntry.response.securityDetails = details; + }) + ); } private async _onRequestCompleted(request: Request) { const networkEntry = this._findNetworkEntry(request); if (!networkEntry) return; - const page = request.frame().page(); - const response = await request.response(); - // response can be null if the request failed - if (response != null) { - this._addBarrier( - page, - response.serverAddr().then(server => { - networkEntry.response.remoteIPAddress = server?.ipAddress; - networkEntry.response.remotePort = server?.port; - }) - ); - this._addBarrier( - page, - response.securityDetails().then(details => { - if (details) networkEntry.response.securityDetails = details; - }) - ); - } - networkEntry.loadEndTime = epochTimeInSeconds(); const timing = request.timing(); const { loadEndTime, requestSentTime } = networkEntry; @@ -281,6 +275,7 @@ export class NetworkManager { total, }; + const page = request.frame().page(); // For aborted/failed requests sizes will not be present this._addBarrier( page, @@ -301,7 +296,14 @@ export class NetworkManager { } async stop() { - await Promise.all(this._barrierPromises); + /** + * Waiting for all network events is error prone and might hang the tests + * from getting closed forever when there are upstream bugs in browsers or + * Playwright. So we log and drop these events once the test run is completed + */ + if (this._barrierPromises.size > 0) { + log(`Plugins: dropping ${this._barrierPromises.size} network events}`); + } const context = this.driver.context; context.off('request', this._onRequest.bind(this)); context.off('response', this._onResponse.bind(this)); From add97fd1f78205cc8ac80360afb34e8787022bea Mon Sep 17 00:00:00 2001 From: vigneshshanmugam Date: Fri, 24 Mar 2023 10:27:58 -0700 Subject: [PATCH 2/4] update pre-commit --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 1da7f16f..90f80aca 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,6 +1,6 @@ repos: - repo: git@github.com:pre-commit/pre-commit-hooks - rev: v2.2.3 + rev: v4.4.0 hooks: - id: check-case-conflict - id: check-executables-have-shebangs From 21660519016097405d84563b95044e6e18be2ca0 Mon Sep 17 00:00:00 2001 From: vigneshshanmugam Date: Fri, 24 Mar 2023 11:01:51 -0700 Subject: [PATCH 3/4] fix test --- src/core/gatherer.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/core/gatherer.ts b/src/core/gatherer.ts index 0339a9df..f78a2e26 100644 --- a/src/core/gatherer.ts +++ b/src/core/gatherer.ts @@ -39,7 +39,6 @@ import { Driver, NetworkConditions, RunOptions } from '../common_types'; * related capabilities for the runner to run all journeys */ export class Gatherer { - static isClosing = false; static browser: ChromiumBrowser; static async setupDriver(options: RunOptions): Promise { @@ -117,8 +116,7 @@ export class Gatherer { static async closeBrowser() { log(`Gatherer: closing browser`); - if (Gatherer.browser && !Gatherer.isClosing) { - Gatherer.isClosing = true; + if (Gatherer.browser) { await Gatherer.browser.close(); Gatherer.browser = null; } From 05cb13de6b93a3430e11a1428867f958b8ac4953 Mon Sep 17 00:00:00 2001 From: vigneshshanmugam Date: Fri, 24 Mar 2023 12:20:36 -0700 Subject: [PATCH 4/4] use https for precommit --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 90f80aca..c9a53836 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,5 +1,5 @@ repos: -- repo: git@github.com:pre-commit/pre-commit-hooks +- repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.4.0 hooks: - id: check-case-conflict