From c8913f8402d14c9dc51861f7851cd77ef94c5b03 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 19 Nov 2025 10:40:56 +0000 Subject: [PATCH 1/4] Initial plan From fc96ccc7b60213e8d4727cab9c8cc88d79c5187c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 19 Nov 2025 10:54:02 +0000 Subject: [PATCH 2/4] Fix test hang issue when restart: false by properly mapping to context restart Co-authored-by: kobenguyent <7845001+kobenguyent@users.noreply.github.com> --- lib/helper/extras/PlaywrightRestartOpts.js | 4 +- lib/listener/helpers.js | 10 +++-- test/helper/PlaywrightRestartOpts_test.js | 50 ++++++++++++++++++++++ 3 files changed, 58 insertions(+), 6 deletions(-) create mode 100644 test/helper/PlaywrightRestartOpts_test.js diff --git a/lib/helper/extras/PlaywrightRestartOpts.js b/lib/helper/extras/PlaywrightRestartOpts.js index ae122697b..f74885c45 100644 --- a/lib/helper/extras/PlaywrightRestartOpts.js +++ b/lib/helper/extras/PlaywrightRestartOpts.js @@ -14,9 +14,9 @@ export function setRestartStrategy(options) { return (restarts = restart) } - // When restart is false, don't restart anything + // When restart is false, map to 'context' restart (as per documentation) if (restart === false) { - restarts = null + restarts = 'context' return } diff --git a/lib/listener/helpers.js b/lib/listener/helpers.js index ed38daa61..d7150e3d6 100644 --- a/lib/listener/helpers.js +++ b/lib/listener/helpers.js @@ -74,9 +74,10 @@ export default function () { event.dispatcher.on(event.all.result, () => { // Skip _finishTest for all helpers if any browser helper restarts to avoid double cleanup + // Note: restart: false is equivalent to restart: 'context' per documentation const hasBrowserRestart = Object.values(helpers).some(helper => - (helper.config && (helper.config.restart === 'browser' || helper.config.restart === 'context' || helper.config.restart === true)) || - (helper.options && (helper.options.restart === 'browser' || helper.options.restart === 'context' || helper.options.restart === true)) + (helper.config && (helper.config.restart === 'browser' || helper.config.restart === 'context' || helper.config.restart === false || helper.config.restart === true)) || + (helper.options && (helper.options.restart === 'browser' || helper.options.restart === 'context' || helper.options.restart === false || helper.options.restart === true)) ) Object.keys(helpers).forEach(key => { @@ -89,9 +90,10 @@ export default function () { event.dispatcher.on(event.all.after, () => { // Skip _cleanup for all helpers if any browser helper restarts to avoid double cleanup + // Note: restart: false is equivalent to restart: 'context' per documentation const hasBrowserRestart = Object.values(helpers).some(helper => - (helper.config && (helper.config.restart === 'browser' || helper.config.restart === 'context' || helper.config.restart === true)) || - (helper.options && (helper.options.restart === 'browser' || helper.options.restart === 'context' || helper.options.restart === true)) + (helper.config && (helper.config.restart === 'browser' || helper.config.restart === 'context' || helper.config.restart === false || helper.config.restart === true)) || + (helper.options && (helper.options.restart === 'browser' || helper.options.restart === 'context' || helper.options.restart === false || helper.options.restart === true)) ) Object.keys(helpers).forEach(key => { diff --git a/test/helper/PlaywrightRestartOpts_test.js b/test/helper/PlaywrightRestartOpts_test.js new file mode 100644 index 000000000..5d5b524b9 --- /dev/null +++ b/test/helper/PlaywrightRestartOpts_test.js @@ -0,0 +1,50 @@ +import * as chai from 'chai' + +const assert = chai.assert +const expect = chai.expect + +import { + setRestartStrategy, + restartsSession, + restartsContext, + restartsBrowser +} from '../../lib/helper/extras/PlaywrightRestartOpts.js' + +describe('PlaywrightRestartOpts', function () { + describe('setRestartStrategy', function () { + it('should set context restart when restart is false', function () { + setRestartStrategy({ restart: false }) + assert.isFalse(restartsSession()) + assert.isTrue(restartsContext()) + assert.isFalse(restartsBrowser()) + }) + + it('should set browser restart when restart is true', function () { + setRestartStrategy({ restart: true }) + assert.isFalse(restartsSession()) + assert.isFalse(restartsContext()) + assert.isTrue(restartsBrowser()) + }) + + it('should set context restart when restart is "context"', function () { + setRestartStrategy({ restart: 'context' }) + assert.isFalse(restartsSession()) + assert.isTrue(restartsContext()) + assert.isFalse(restartsBrowser()) + }) + + it('should set session restart when restart is "session"', function () { + setRestartStrategy({ restart: 'session' }) + assert.isTrue(restartsSession()) + assert.isFalse(restartsContext()) + assert.isFalse(restartsBrowser()) + }) + + it('should set browser restart when restart is "browser"', function () { + setRestartStrategy({ restart: 'browser' }) + assert.isFalse(restartsSession()) + assert.isFalse(restartsContext()) + assert.isTrue(restartsBrowser()) + }) + }) +}) From f5682e7b6e1e2042be2f7f129d2ada266ef24422 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 19 Nov 2025 11:48:12 +0000 Subject: [PATCH 3/4] Fix browser close hanging by ensuring all pages and contexts are closed first Co-authored-by: kobenguyent <7845001+kobenguyent@users.noreply.github.com> --- lib/helper/Playwright.js | 23 ++++++++++ lib/helper/extras/PlaywrightRestartOpts.js | 4 +- lib/listener/helpers.js | 10 ++--- test/helper/PlaywrightRestartOpts_test.js | 50 ---------------------- 4 files changed, 29 insertions(+), 58 deletions(-) delete mode 100644 test/helper/PlaywrightRestartOpts_test.js diff --git a/lib/helper/Playwright.js b/lib/helper/Playwright.js index 882e7c455..6368c23e4 100644 --- a/lib/helper/Playwright.js +++ b/lib/helper/Playwright.js @@ -1389,6 +1389,29 @@ class Playwright extends Helper { } } + // Close all contexts and pages before closing browser to prevent hanging + if (this.browser) { + try { + const contexts = await Promise.race([ + this.browser.contexts(), + new Promise((_, reject) => setTimeout(() => reject(new Error('Get contexts timeout')), 1000)) + ]) + // Close all pages in all contexts first + await Promise.allSettled(contexts.map(async (ctx) => { + try { + const pages = await ctx.pages() + await Promise.allSettled(pages.map(p => p.close().catch(() => {}))) + } catch (e) { + // Ignore errors getting or closing pages + } + })) + // Then close all contexts + await Promise.allSettled(contexts.map(c => c.close().catch(() => {}))) + } catch (e) { + // Ignore errors if browser is already closed or timeout getting contexts + } + } + if (this.options.recordHar && this.browserContext) { try { await this.browserContext.close() diff --git a/lib/helper/extras/PlaywrightRestartOpts.js b/lib/helper/extras/PlaywrightRestartOpts.js index f74885c45..ae122697b 100644 --- a/lib/helper/extras/PlaywrightRestartOpts.js +++ b/lib/helper/extras/PlaywrightRestartOpts.js @@ -14,9 +14,9 @@ export function setRestartStrategy(options) { return (restarts = restart) } - // When restart is false, map to 'context' restart (as per documentation) + // When restart is false, don't restart anything if (restart === false) { - restarts = 'context' + restarts = null return } diff --git a/lib/listener/helpers.js b/lib/listener/helpers.js index d7150e3d6..ed38daa61 100644 --- a/lib/listener/helpers.js +++ b/lib/listener/helpers.js @@ -74,10 +74,9 @@ export default function () { event.dispatcher.on(event.all.result, () => { // Skip _finishTest for all helpers if any browser helper restarts to avoid double cleanup - // Note: restart: false is equivalent to restart: 'context' per documentation const hasBrowserRestart = Object.values(helpers).some(helper => - (helper.config && (helper.config.restart === 'browser' || helper.config.restart === 'context' || helper.config.restart === false || helper.config.restart === true)) || - (helper.options && (helper.options.restart === 'browser' || helper.options.restart === 'context' || helper.options.restart === false || helper.options.restart === true)) + (helper.config && (helper.config.restart === 'browser' || helper.config.restart === 'context' || helper.config.restart === true)) || + (helper.options && (helper.options.restart === 'browser' || helper.options.restart === 'context' || helper.options.restart === true)) ) Object.keys(helpers).forEach(key => { @@ -90,10 +89,9 @@ export default function () { event.dispatcher.on(event.all.after, () => { // Skip _cleanup for all helpers if any browser helper restarts to avoid double cleanup - // Note: restart: false is equivalent to restart: 'context' per documentation const hasBrowserRestart = Object.values(helpers).some(helper => - (helper.config && (helper.config.restart === 'browser' || helper.config.restart === 'context' || helper.config.restart === false || helper.config.restart === true)) || - (helper.options && (helper.options.restart === 'browser' || helper.options.restart === 'context' || helper.options.restart === false || helper.options.restart === true)) + (helper.config && (helper.config.restart === 'browser' || helper.config.restart === 'context' || helper.config.restart === true)) || + (helper.options && (helper.options.restart === 'browser' || helper.options.restart === 'context' || helper.options.restart === true)) ) Object.keys(helpers).forEach(key => { diff --git a/test/helper/PlaywrightRestartOpts_test.js b/test/helper/PlaywrightRestartOpts_test.js deleted file mode 100644 index 5d5b524b9..000000000 --- a/test/helper/PlaywrightRestartOpts_test.js +++ /dev/null @@ -1,50 +0,0 @@ -import * as chai from 'chai' - -const assert = chai.assert -const expect = chai.expect - -import { - setRestartStrategy, - restartsSession, - restartsContext, - restartsBrowser -} from '../../lib/helper/extras/PlaywrightRestartOpts.js' - -describe('PlaywrightRestartOpts', function () { - describe('setRestartStrategy', function () { - it('should set context restart when restart is false', function () { - setRestartStrategy({ restart: false }) - assert.isFalse(restartsSession()) - assert.isTrue(restartsContext()) - assert.isFalse(restartsBrowser()) - }) - - it('should set browser restart when restart is true', function () { - setRestartStrategy({ restart: true }) - assert.isFalse(restartsSession()) - assert.isFalse(restartsContext()) - assert.isTrue(restartsBrowser()) - }) - - it('should set context restart when restart is "context"', function () { - setRestartStrategy({ restart: 'context' }) - assert.isFalse(restartsSession()) - assert.isTrue(restartsContext()) - assert.isFalse(restartsBrowser()) - }) - - it('should set session restart when restart is "session"', function () { - setRestartStrategy({ restart: 'session' }) - assert.isTrue(restartsSession()) - assert.isFalse(restartsContext()) - assert.isFalse(restartsBrowser()) - }) - - it('should set browser restart when restart is "browser"', function () { - setRestartStrategy({ restart: 'browser' }) - assert.isFalse(restartsSession()) - assert.isFalse(restartsContext()) - assert.isTrue(restartsBrowser()) - }) - }) -}) From c91d94caa1b4c8caeaf67e0a584348cd9a5dcc8c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 19 Nov 2025 14:40:35 +0000 Subject: [PATCH 4/4] Don't wait for browser.close() to prevent hanging - fire and forget approach Co-authored-by: kobenguyent <7845001+kobenguyent@users.noreply.github.com> --- lib/helper/Playwright.js | 38 ++++++++------------------------------ 1 file changed, 8 insertions(+), 30 deletions(-) diff --git a/lib/helper/Playwright.js b/lib/helper/Playwright.js index 6368c23e4..931119763 100644 --- a/lib/helper/Playwright.js +++ b/lib/helper/Playwright.js @@ -1389,29 +1389,7 @@ class Playwright extends Helper { } } - // Close all contexts and pages before closing browser to prevent hanging - if (this.browser) { - try { - const contexts = await Promise.race([ - this.browser.contexts(), - new Promise((_, reject) => setTimeout(() => reject(new Error('Get contexts timeout')), 1000)) - ]) - // Close all pages in all contexts first - await Promise.allSettled(contexts.map(async (ctx) => { - try { - const pages = await ctx.pages() - await Promise.allSettled(pages.map(p => p.close().catch(() => {}))) - } catch (e) { - // Ignore errors getting or closing pages - } - })) - // Then close all contexts - await Promise.allSettled(contexts.map(c => c.close().catch(() => {}))) - } catch (e) { - // Ignore errors if browser is already closed or timeout getting contexts - } - } - + // Close browserContext if recordHar is enabled if (this.options.recordHar && this.browserContext) { try { await this.browserContext.close() @@ -1421,16 +1399,16 @@ class Playwright extends Helper { } this.browserContext = null + // Initiate browser close without waiting for it to complete + // The browser process will be cleaned up when the Node process exits if (this.browser) { try { - // Add timeout to prevent browser.close() from hanging indefinitely - await Promise.race([this.browser.close(), new Promise((_, reject) => setTimeout(() => reject(new Error('Browser close timeout')), 5000))]) + // Fire and forget - don't wait for close to complete + this.browser.close().catch(() => { + // Silently ignore any errors during async close + }) } catch (e) { - // Ignore errors if browser is already closed or timeout - if (!e.message?.includes('Browser close timeout')) { - // Non-timeout error, can be ignored as well - } - // Force cleanup even on error + // Ignore any synchronous errors } } this.browser = null