fix: update firefox to close extra windows between specs (#29475)

cypress-io · May 6, 2024 · 555a924 · 555a924 · cypress-bot · May 6, 2024
1 parent 4782f89
commit 555a924
Show file tree

Hide file tree

Showing 13 changed files with 96 additions and 29 deletions.
diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md
@@ -11,6 +11,7 @@ _Released 5/7/2024 (PENDING)_
 
 - Fixed a bug where promises rejected with `undefined` were failing inside `cy.origin()`. Addresses [#23937](https://github.com/cypress-io/cypress/issues/23937).
 - We now pass the same default Chromium flags to Electron as we do to Chrome. As a result of this change, the application under test's `navigator.webdriver` property will now correctly be `true` when testing in Electron. Fixes [#27939](https://github.com/cypress-io/cypress/issues/27939).
+- Fixed an issue where extra windows weren't being closed between specs in Firefox causing potential issues in subsequent specs. Fixes [#29473](https://github.com/cypress-io/cypress/issues/29473).
 
 **Misc:**
 

diff --git a/packages/data-context/src/actions/ProjectActions.ts b/packages/data-context/src/actions/ProjectActions.ts
@@ -52,7 +52,7 @@ export interface ProjectApiShape {
     emitter: EventEmitter
   }
   isListening: (url: string) => Promise<void>
-  resetBrowserTabsForNextTest(shouldKeepTabOpen: boolean): Promise<void>
+  resetBrowserTabsForNextSpec(shouldKeepTabOpen: boolean): Promise<void>
   resetServer(): void
   runSpec(spec: Cypress.Spec): Promise<void>
   routeToDebug(runNumber: number): void
@@ -279,7 +279,7 @@ export class ProjectActions {
 
     // Used for run-all-specs feature
     if (options?.shouldLaunchNewTab) {
-      await this.api.resetBrowserTabsForNextTest(true)
+      await this.api.resetBrowserTabsForNextSpec(true)
       this.api.resetServer()
     }
 

diff --git a/packages/extension/app/v2/background.js b/packages/extension/app/v2/background.js
@@ -136,8 +136,8 @@ const connect = function (host, path, extraOpts) {
         return invoke('takeScreenshot', id)
       case 'reset:browser:state':
         return invoke('resetBrowserState', id)
-      case 'reset:browser:tabs:for:next:test':
-        return invoke('resetBrowserTabsForNextTest', id)
+      case 'reset:browser:tabs:for:next:spec':
+        return invoke('resetBrowserTabsForNextSpec', id)
       default:
         return fail(id, { message: `No handler registered for: '${msg}'` })
     }
@@ -284,10 +284,21 @@ const automation = {
     return browser.browsingData.remove({}, { cache: true, cookies: true, downloads: true, formData: true, history: true, indexedDB: true, localStorage: true, passwords: true, pluginData: true, serviceWorkers: true }).then(fn)
   },
 
-  resetBrowserTabsForNextTest (fn) {
+  resetBrowserTabsForNextSpec (callback) {
     return Promise.try(() => {
       return browser.windows.getCurrent({ populate: true })
-    }).then(async (windowInfo) => {
+    }).then(async (currentWindowInfo) => {
+      const windows = await browser.windows.getAll().catch(() => [])
+
+      for (const window of windows) {
+        // remove/close the window if it's not the current window
+        if (window.id !== currentWindowInfo.id) {
+          await browser.windows.remove(window.id).catch(() => {})
+        }
+      }
+
+      return currentWindowInfo
+    }).then(async (currentWindowInfo) => {
       let newTabId = null
 
       try {
@@ -300,8 +311,8 @@ const automation = {
       // eslint-disable-next-line no-empty
       } catch (e) {}
 
-      return browser.tabs.remove(windowInfo.tabs.map((tab) => tab.id).filter((tab) => tab.id !== newTabId))
-    }).then(fn)
+      return browser.tabs.remove(currentWindowInfo.tabs.map((tab) => tab.id).filter((tab) => tab.id !== newTabId))
+    }).then(callback)
   },
 
   query (data) {

diff --git a/packages/extension/test/integration/v2/background_spec.js b/packages/extension/test/integration/v2/background_spec.js
@@ -24,8 +24,10 @@ const browser = {
     },
   },
   windows: {
-    getLastFocused () {},
+    getAll () {},
     getCurrent () {},
+    getLastFocused () {},
+    remove () {},
     update () {},
   },
   runtime: {},
@@ -844,7 +846,7 @@ describe('app/background', () => {
       })
     })
 
-    describe('reset:browser:tabs:for:next:test', () => {
+    describe('reset:browser:tabs:for:next:spec', () => {
       beforeEach(() => {
         sinon.stub(browser.windows, 'getCurrent').withArgs({ populate: true }).resolves({ id: '10', tabs: [{ id: '1' }, { id: '2' }, { id: '3' }] })
         sinon.stub(browser.tabs, 'remove').withArgs(['1', '2', '3']).resolves()
@@ -855,6 +857,8 @@ describe('app/background', () => {
 
       // @see https://github.com/cypress-io/cypress/issues/29172 for Firefox versions 124 and up
       it('closes the tabs in the current browser window and creates a new "about:blank" tab', function (done) {
+        sinon.stub(browser.windows, 'getAll').resolves([{ id: '10' }])
+
         this.socket.on('automation:response', (id, obj) => {
           expect(id).to.eq(123)
           expect(obj.response).to.be.undefined
@@ -866,7 +870,58 @@ describe('app/background', () => {
           done()
         })
 
-        this.server.emit('automation:request', 123, 'reset:browser:tabs:for:next:test')
+        this.server.emit('automation:request', 123, 'reset:browser:tabs:for:next:spec')
+      })
+
+      it('closes any extra windows', function (done) {
+        sinon.stub(browser.windows, 'getAll').resolves([{ id: '9' }, { id: '10' }, { id: '11' }])
+        sinon.stub(browser.windows, 'remove').resolves()
+
+        this.socket.on('automation:response', (id, obj) => {
+          expect(id).to.eq(123)
+          expect(obj.response).to.be.undefined
+
+          expect(browser.windows.remove).to.be.calledWith('9')
+          expect(browser.windows.remove).to.be.calledWith('11')
+          expect(browser.windows.remove).not.to.be.calledWith('10')
+
+          done()
+        })
+
+        this.server.emit('automation:request', 123, 'reset:browser:tabs:for:next:spec')
+      })
+
+      it('does not fail if we are unable to close the window', function (done) {
+        sinon.stub(browser.windows, 'getAll').resolves([{ id: '9' }, { id: '10' }, { id: '11' }])
+        sinon.stub(browser.windows, 'remove').rejects()
+
+        this.socket.on('automation:response', (id, obj) => {
+          expect(id).to.eq(123)
+          expect(obj.response).to.be.undefined
+
+          expect(browser.windows.remove).to.be.calledWith('9')
+          expect(browser.windows.remove).to.be.calledWith('11')
+
+          expect(browser.windows.remove).not.to.be.calledWith('10')
+          done()
+        })
+
+        this.server.emit('automation:request', 123, 'reset:browser:tabs:for:next:spec')
+      })
+
+      it('does not fail if we are unable to retrieve the windows', function (done) {
+        sinon.stub(browser.windows, 'getAll').rejects()
+        sinon.stub(browser.windows, 'remove')
+
+        this.socket.on('automation:response', (id, obj) => {
+          expect(id).to.eq(123)
+          expect(obj.response).to.be.undefined
+
+          expect(browser.windows.remove).not.to.be.called
+          done()
+        })
+
+        this.server.emit('automation:request', 123, 'reset:browser:tabs:for:next:spec')
       })
     })
   })

diff --git a/packages/server/lib/browsers/cdp_automation.ts b/packages/server/lib/browsers/cdp_automation.ts
@@ -578,7 +578,7 @@ export class CdpAutomation implements CDPClient {
           this.sendDebuggerCommandFn('Storage.clearDataForOrigin', { origin: '*', storageTypes: 'all' }),
           this.sendDebuggerCommandFn('Network.clearBrowserCache'),
         ])
-      case 'reset:browser:tabs:for:next:test':
+      case 'reset:browser:tabs:for:next:spec':
         return this.sendCloseCommandFn(data.shouldKeepTabOpen)
       case 'focus:browser:window':
         return this.sendDebuggerCommandFn('Page.bringToFront')

diff --git a/packages/server/lib/browsers/webkit-automation.ts b/packages/server/lib/browsers/webkit-automation.ts
@@ -373,7 +373,7 @@ export class WebKitAutomation {
         debug('stubbed reset:browser:state')
 
         return
-      case 'reset:browser:tabs:for:next:test':
+      case 'reset:browser:tabs:for:next:spec':
         if (data.shouldKeepTabOpen) return await this.reset({})
 
         return await this.context.browser()?.close()

diff --git a/packages/server/lib/makeDataContext.ts b/packages/server/lib/makeDataContext.ts
@@ -155,8 +155,8 @@ export function makeDataContext (options: MakeDataContextOptions): DataContext {
         return devServer
       },
       isListening,
-      resetBrowserTabsForNextTest (shouldKeepTabOpen: boolean) {
-        return openProject.resetBrowserTabsForNextTest(shouldKeepTabOpen)
+      resetBrowserTabsForNextSpec (shouldKeepTabOpen: boolean) {
+        return openProject.resetBrowserTabsForNextSpec(shouldKeepTabOpen)
       },
       resetServer () {
         return openProject.getProject()?.server.reset()

diff --git a/packages/server/lib/modes/run.ts b/packages/server/lib/modes/run.ts
@@ -652,7 +652,7 @@ async function waitForTestsToFinishRunning (options: { project: Project, screens
   if (!usingExperimentalSingleTabMode || isLastSpec) {
     debug('attempting to close the browser tab')
 
-    await openProject.resetBrowserTabsForNextTest(shouldKeepTabOpen)
+    await openProject.resetBrowserTabsForNextSpec(shouldKeepTabOpen)
 
     debug('resetting server state')
 

diff --git a/packages/server/lib/open_project.ts b/packages/server/lib/open_project.ts
@@ -199,9 +199,9 @@ export class OpenProject {
     return browsers.close()
   }
 
-  async resetBrowserTabsForNextTest (shouldKeepTabOpen: boolean) {
+  async resetBrowserTabsForNextSpec (shouldKeepTabOpen: boolean) {
     try {
-      await this.projectBase?.resetBrowserTabsForNextTest(shouldKeepTabOpen)
+      await this.projectBase?.resetBrowserTabsForNextSpec(shouldKeepTabOpen)
     } catch (e) {
       // If the CRI client disconnected or crashed, we want to no-op here so that anything
       // depending on resetting the browser tabs can continue with further operations

diff --git a/packages/server/lib/project-base.ts b/packages/server/lib/project-base.ts
@@ -440,8 +440,8 @@ export class ProjectBase extends EE {
     this.ctx.actions.servers.setAppSocketServer(ios)
   }
 
-  async resetBrowserTabsForNextTest (shouldKeepTabOpen: boolean) {
-    return this.server.socket.resetBrowserTabsForNextTest(shouldKeepTabOpen)
+  async resetBrowserTabsForNextSpec (shouldKeepTabOpen: boolean) {
+    return this.server.socket.resetBrowserTabsForNextSpec(shouldKeepTabOpen)
   }
 
   async resetBrowserState () {

diff --git a/packages/server/lib/socket-base.ts b/packages/server/lib/socket-base.ts
@@ -41,7 +41,7 @@ const retry = (fn: (res: any) => void) => {
 }
 
 export class SocketBase {
-  private _sendResetBrowserTabsForNextTestMessage
+  private _sendResetBrowserTabsForNextSpecMessage
   private _sendResetBrowserStateMessage
   private _isRunnerSocketConnected
   private _sendFocusBrowserMessage
@@ -281,8 +281,8 @@ export class SocketBase {
           })
         })
 
-        this._sendResetBrowserTabsForNextTestMessage = async (shouldKeepTabOpen: boolean) => {
-          await automationRequest('reset:browser:tabs:for:next:test', { shouldKeepTabOpen })
+        this._sendResetBrowserTabsForNextSpecMessage = async (shouldKeepTabOpen: boolean) => {
+          await automationRequest('reset:browser:tabs:for:next:spec', { shouldKeepTabOpen })
         }
 
         this._sendResetBrowserStateMessage = async () => {
@@ -616,9 +616,9 @@ export class SocketBase {
     })
   }
 
-  async resetBrowserTabsForNextTest (shouldKeepTabOpen: boolean) {
-    if (this._sendResetBrowserTabsForNextTestMessage) {
-      await this._sendResetBrowserTabsForNextTestMessage(shouldKeepTabOpen)
+  async resetBrowserTabsForNextSpec (shouldKeepTabOpen: boolean) {
+    if (this._sendResetBrowserTabsForNextSpecMessage) {
+      await this._sendResetBrowserTabsForNextSpecMessage(shouldKeepTabOpen)
     }
   }
 

diff --git a/packages/server/test/unit/browsers/cdp_automation_spec.ts b/packages/server/test/unit/browsers/cdp_automation_spec.ts
@@ -570,11 +570,11 @@ context('lib/browsers/cdp_automation', () => {
       })
     })
 
-    describe('reset:browser:tabs:for:next:test', function () {
+    describe('reset:browser:tabs:for:next:spec', function () {
       it('sends the close target message for the attached target tabs', async function () {
         this.sendCloseTargetCommand.resolves()
 
-        await this.onRequest('reset:browser:tabs:for:next:test', { shouldKeepTabOpen: true })
+        await this.onRequest('reset:browser:tabs:for:next:spec', { shouldKeepTabOpen: true })
 
         expect(this.sendCloseTargetCommand).to.be.calledWith(true)
       })

diff --git a/packages/server/test/unit/browsers/electron_spec.js b/packages/server/test/unit/browsers/electron_spec.js
@@ -435,7 +435,7 @@ describe('lib/browsers/electron', () => {
         expect(this.automation.use).to.be.called
         expect(this.automation.use.lastCall.args[0].onRequest).to.be.a('function')
 
-        await this.automation.use.lastCall.args[0].onRequest('reset:browser:tabs:for:next:test', { shouldKeepTabOpen: true })
+        await this.automation.use.lastCall.args[0].onRequest('reset:browser:tabs:for:next:spec', { shouldKeepTabOpen: true })
 
         expect(this.win.destroy).to.be.called
       })