From dfb31f3e92f7aef695d018080851c9b0595255ba Mon Sep 17 00:00:00 2001 From: Nikolay Vitkov <34244704+Lightning00Blade@users.noreply.github.com> Date: Thu, 2 May 2024 09:22:06 +0200 Subject: [PATCH] test: puppeteer.connect with `using` disconnects the browser (#12371) --- package.json | 2 +- packages/puppeteer-core/src/api/Browser.ts | 10 ++++- packages/testserver/src/index.ts | 2 +- test/src/browser.spec.ts | 5 +-- test/src/browsercontext.spec.ts | 3 +- test/src/cdp/devtools.spec.ts | 4 +- test/src/chromiumonly.spec.ts | 5 +-- test/src/fixtures.spec.ts | 2 +- test/src/launcher.spec.ts | 51 ++++++++++++---------- test/src/oopif.spec.ts | 2 +- 10 files changed, 47 insertions(+), 39 deletions(-) diff --git a/package.json b/package.json index f5af2792034..53a88cc675e 100644 --- a/package.json +++ b/package.json @@ -145,6 +145,7 @@ "@typescript-eslint/eslint-plugin": "7.7.1", "@typescript-eslint/parser": "7.7.1", "esbuild": "0.20.2", + "eslint": "8.57.0", "eslint-config-prettier": "9.1.0", "eslint-import-resolver-typescript": "3.6.1", "eslint-plugin-import": "2.29.1", @@ -153,7 +154,6 @@ "eslint-plugin-rulesdir": "0.2.2", "eslint-plugin-tsdoc": "0.2.17", "eslint-plugin-unused-imports": "3.1.0", - "eslint": "8.57.0", "execa": "8.0.1", "expect": "29.7.0", "gts": "5.3.0", diff --git a/packages/puppeteer-core/src/api/Browser.ts b/packages/puppeteer-core/src/api/Browser.ts index 67835e4aa26..04b012448e8 100644 --- a/packages/puppeteer-core/src/api/Browser.ts +++ b/packages/puppeteer-core/src/api/Browser.ts @@ -424,12 +424,18 @@ export abstract class Browser extends EventEmitter { /** @internal */ [disposeSymbol](): void { - return void this.close().catch(debugError); + if (this.process()) { + return void this.close().catch(debugError); + } + return void this.disconnect().catch(debugError); } /** @internal */ [asyncDisposeSymbol](): Promise { - return this.close(); + if (this.process()) { + return this.close(); + } + return this.disconnect(); } /** diff --git a/packages/testserver/src/index.ts b/packages/testserver/src/index.ts index 89439a08dde..416e903ec6c 100644 --- a/packages/testserver/src/index.ts +++ b/packages/testserver/src/index.ts @@ -197,7 +197,7 @@ export class TestServer { this.#requestSubscribers.clear(); for (const request of this.#requests.values()) { if (!request.writableEnded) { - request.end(); + request.destroy(); } } this.#requests.clear(); diff --git a/test/src/browser.spec.ts b/test/src/browser.spec.ts index edfa075c4d3..7f4907b98be 100644 --- a/test/src/browser.spec.ts +++ b/test/src/browser.spec.ts @@ -57,12 +57,11 @@ describe('Browser specs', function () { }); const browserWSEndpoint = browser.wsEndpoint(); - const remoteBrowser = await puppeteer.connect({ + using remoteBrowser = await puppeteer.connect({ browserWSEndpoint, protocol: browser.protocol, }); expect(remoteBrowser.process()).toBe(null); - await remoteBrowser.disconnect(); }); it('should keep connected after the last page is closed', async () => { const {browser, close} = await launch({}, {createContext: false}); @@ -90,7 +89,7 @@ describe('Browser specs', function () { }); const browserWSEndpoint = browser.wsEndpoint(); - const newBrowser = await puppeteer.connect({ + using newBrowser = await puppeteer.connect({ browserWSEndpoint, protocol: browser.protocol, }); diff --git a/test/src/browsercontext.spec.ts b/test/src/browsercontext.spec.ts index 66ddaeb1837..0ff69662084 100644 --- a/test/src/browsercontext.spec.ts +++ b/test/src/browsercontext.spec.ts @@ -222,13 +222,12 @@ describe('BrowserContext', function () { const context = await browser.createBrowserContext(); try { expect(browser.browserContexts()).toHaveLength(2); - const remoteBrowser = await puppeteer.connect({ + using remoteBrowser = await puppeteer.connect({ browserWSEndpoint: browser.wsEndpoint(), protocol: browser.protocol, }); const contexts = remoteBrowser.browserContexts(); expect(contexts).toHaveLength(2); - await remoteBrowser.disconnect(); } finally { await context.close(); } diff --git a/test/src/cdp/devtools.spec.ts b/test/src/cdp/devtools.spec.ts index 0f9330d15fa..a8fcddadfab 100644 --- a/test/src/cdp/devtools.spec.ts +++ b/test/src/cdp/devtools.spec.ts @@ -50,7 +50,7 @@ describe('DevTools', function () { const browserWSEndpoint = originalBrowser.wsEndpoint(); - const browser = await puppeteer.connect({ + using browser = await puppeteer.connect({ browserWSEndpoint, _isPageTarget(target) { return ( @@ -75,7 +75,7 @@ describe('DevTools', function () { const browserWSEndpoint = originalBrowser.wsEndpoint(); - const browser = await puppeteer.connect({ + using browser = await puppeteer.connect({ browserWSEndpoint, }); const devtoolsPageTarget = await browser.waitForTarget(target => { diff --git a/test/src/chromiumonly.spec.ts b/test/src/chromiumonly.spec.ts index e0c41317aa9..68b13ff548a 100644 --- a/test/src/chromiumonly.spec.ts +++ b/test/src/chromiumonly.spec.ts @@ -21,7 +21,7 @@ describe('Chromium-Specific Launcher tests', function () { try { const browserURL = 'http://127.0.0.1:21222'; - const browser1 = await puppeteer.connect({browserURL}); + using browser1 = await puppeteer.connect({browserURL}); const page1 = await browser1.newPage(); expect( await page1.evaluate(() => { @@ -30,7 +30,7 @@ describe('Chromium-Specific Launcher tests', function () { ).toBe(56); await browser1.disconnect(); - const browser2 = await puppeteer.connect({ + using browser2 = await puppeteer.connect({ browserURL: browserURL + '/', }); const page2 = await browser2.newPage(); @@ -39,7 +39,6 @@ describe('Chromium-Specific Launcher tests', function () { return 8 * 7; }) ).toBe(56); - await browser2.disconnect(); } finally { await close(); } diff --git a/test/src/fixtures.spec.ts b/test/src/fixtures.spec.ts index e7a2e1ac9bf..919ffbb5c5a 100644 --- a/test/src/fixtures.spec.ts +++ b/test/src/fixtures.spec.ts @@ -101,7 +101,7 @@ describe('Fixtures', function () { wsEndPointCallback(output.substring(0, output.indexOf('\n'))); } }); - const browser = await puppeteer.connect({ + using browser = await puppeteer.connect({ browserWSEndpoint: await wsEndPointPromise, }); const promises = [ diff --git a/test/src/launcher.spec.ts b/test/src/launcher.spec.ts index e1eefaceb19..7844e6f38df 100644 --- a/test/src/launcher.spec.ts +++ b/test/src/launcher.spec.ts @@ -31,13 +31,13 @@ describe('Launcher specs', function () { const {browser, close, puppeteer, server} = await launch({}); server.setRoute('/one-style.css', () => {}); try { - const remote = await puppeteer.connect({ + using remote = await puppeteer.connect({ browserWSEndpoint: browser.wsEndpoint(), protocol: browser.protocol, }); const page = await remote.newPage(); const navigationPromise = page - .goto(server.PREFIX + '/one-style.html', {timeout: 60000}) + .goto(server.PREFIX + '/one-style.html', {timeout: 60_000}) .catch(error_ => { return error_; }); @@ -61,13 +61,13 @@ describe('Launcher specs', function () { const {browser, close, server, puppeteer} = await launch({}); server.setRoute('/empty.html', () => {}); try { - const remote = await puppeteer.connect({ + using remote = await puppeteer.connect({ browserWSEndpoint: browser.wsEndpoint(), protocol: browser.protocol, }); const page = await remote.newPage(); const watchdog = page - .waitForSelector('div', {timeout: 60000}) + .waitForSelector('div', {timeout: 60_000}) .catch(error_ => { return error_; }); @@ -83,7 +83,7 @@ describe('Launcher specs', function () { it('should terminate network waiters', async () => { const {browser, close, server, puppeteer} = await launch({}); try { - const remote = await puppeteer.connect({ + using remote = await puppeteer.connect({ browserWSEndpoint: browser.wsEndpoint(), protocol: browser.protocol, }); @@ -119,9 +119,9 @@ describe('Launcher specs', function () { it('can launch multiple instances without node warnings', async () => { const instances = []; - let warning = null; + let warning: Error | undefined; const warningHandler: NodeJS.WarningListener = w => { - return (warning = w); + warning = w; }; process.on('warning', warningHandler); process.setMaxListeners(1); @@ -138,7 +138,7 @@ describe('Launcher specs', function () { process.setMaxListeners(10); } process.off('warning', warningHandler); - expect(warning).toBe(null); + expect(warning?.stack).toBe(undefined); }); it('should have default url when launching browser', async function () { const {browser, close} = await launch({}, {createContext: false}); @@ -536,11 +536,10 @@ describe('Launcher specs', function () { } }); it('should pass the timeout parameter to browser.waitForTarget', async () => { - const options = { - timeout: 1, - }; let error!: Error; - await launch(options).catch(error_ => { + await launch({ + timeout: 1, + }).catch(error_ => { return (error = error_); }); expect(error).toBeInstanceOf(TimeoutError); @@ -660,7 +659,7 @@ describe('Launcher specs', function () { it('should be able to connect multiple times to the same browser', async () => { const {puppeteer, browser, close} = await launch({}); try { - const otherBrowser = await puppeteer.connect({ + using otherBrowser = await puppeteer.connect({ browserWSEndpoint: browser.wsEndpoint(), protocol: browser.protocol, }); @@ -685,7 +684,7 @@ describe('Launcher specs', function () { it('should be able to close remote browser', async () => { const {puppeteer, browser, close} = await launch({}); try { - const remoteBrowser = await puppeteer.connect({ + using remoteBrowser = await puppeteer.connect({ browserWSEndpoint: browser.wsEndpoint(), protocol: browser.protocol, }); @@ -707,7 +706,7 @@ describe('Launcher specs', function () { return page.close(); }) ); - const remoteBrowser = await puppeteer.connect({ + using remoteBrowser = await puppeteer.connect({ browserWSEndpoint: browser.wsEndpoint(), protocol: browser.protocol, }); @@ -729,7 +728,7 @@ describe('Launcher specs', function () { try { const browserWSEndpoint = browser.wsEndpoint(); - const remoteBrowser = await puppeteer.connect({ + using remoteBrowser = await puppeteer.connect({ browserWSEndpoint, ignoreHTTPSErrors: true, protocol: browser.protocol, @@ -746,7 +745,6 @@ describe('Launcher specs', function () { .replace('v', ' '); expect(response!.securityDetails()!.protocol()).toBe(protocol); await page.close(); - await remoteBrowser.close(); } finally { await close(); } @@ -791,7 +789,7 @@ describe('Launcher specs', function () { const page2 = await browser.newPage(); await page2.goto(server.EMPTY_PAGE + '?should-be-ignored'); - const remoteBrowser = await puppeteer.connect({ + using remoteBrowser = await puppeteer.connect({ browserWSEndpoint, targetFilter: target => { return !target.url().includes('should-be-ignored'); @@ -819,6 +817,8 @@ describe('Launcher specs', function () { }); it('should be able to reconnect to a disconnected browser', async () => { const {puppeteer, server, browser, close} = await launch({}); + // Connection is closed on the original one + let remoteClose!: () => Promise; try { const browserWSEndpoint = browser.wsEndpoint(); const page = await browser.newPage(); @@ -829,6 +829,8 @@ describe('Launcher specs', function () { browserWSEndpoint, protocol: browser.protocol, }); + remoteClose = remoteBrowser.close.bind(remoteBrowser); + console.log(remoteClose); const pages = await remoteBrowser.pages(); const restoredPage = pages.find(page => { return page.url() === server.PREFIX + '/frames/nested-frames.html'; @@ -845,8 +847,8 @@ describe('Launcher specs', function () { return 7 * 8; }) ).toBe(56); - await remoteBrowser.close(); } finally { + await remoteClose(); await close(); } }); @@ -855,7 +857,7 @@ describe('Launcher specs', function () { const {puppeteer, browser: browserOne, close} = await launch({}); try { - const browserTwo = await puppeteer.connect({ + using browserTwo = await puppeteer.connect({ browserWSEndpoint: browserOne.wsEndpoint(), protocol: browserOne.protocol, }); @@ -889,6 +891,8 @@ describe('Launcher specs', function () { browser: browserOne, close, } = await launch({}); + // Connection is closed on the original one + let remoteClose!: () => Promise; try { const browserWSEndpoint = browserOne.wsEndpoint(); const pageOne = await browserOne.newPage(); @@ -899,6 +903,7 @@ describe('Launcher specs', function () { browserWSEndpoint, protocol: browserOne.protocol, }); + remoteClose = browserTwo.close.bind(browserTwo); const pages = await browserTwo.pages(); const pageTwo = pages.find(page => { return page.url() === server.EMPTY_PAGE; @@ -907,8 +912,8 @@ describe('Launcher specs', function () { using _ = await pageTwo.waitForSelector('body', { timeout: 10000, }); - await browserTwo.close(); } finally { + await remoteClose(); await close(); } }); @@ -993,11 +998,11 @@ describe('Launcher specs', function () { const {puppeteer, browser, close} = await launch({}); try { const browserWSEndpoint = browser.wsEndpoint(); - const remoteBrowser1 = await puppeteer.connect({ + using remoteBrowser1 = await puppeteer.connect({ browserWSEndpoint, protocol: browser.protocol, }); - const remoteBrowser2 = await puppeteer.connect({ + using remoteBrowser2 = await puppeteer.connect({ browserWSEndpoint, protocol: browser.protocol, }); diff --git a/test/src/oopif.spec.ts b/test/src/oopif.spec.ts index 0213e14d5d5..aa9f3774522 100644 --- a/test/src/oopif.spec.ts +++ b/test/src/oopif.spec.ts @@ -404,7 +404,7 @@ describe('OOPIF', function () { expect(page.frames()).toHaveLength(2); const browserURL = 'http://127.0.0.1:21222'; - const browser1 = await puppeteer.connect({browserURL}); + using browser1 = await puppeteer.connect({browserURL}); const target = await browser1.waitForTarget(target => { return target.url().endsWith('dynamic-oopif.html'); });