fix: use open instead of connectToNewSpec in electron (#27128)

Co-authored-by: Ryan Manuel <[email protected]>

.circleci/workflows.yml

@@ -41,7 +41,7 @@ macWorkflowFilters: &darwin-workflow-filters
     - equal: [ develop, << pipeline.git.branch >> ]
     # use the following branch as well to ensure that v8 snapshot cache updates are fully tested
     - equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
-    - equal: [ 'fix/chrome_crash_recovery', << pipeline.git.branch >> ]
+    - equal: [ 'mschile/issue-26900_browserCriClient', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -52,7 +52,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters
     - equal: [ develop, << pipeline.git.branch >> ]
     # use the following branch as well to ensure that v8 snapshot cache updates are fully tested
     - equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
-    - equal: [ 'fix/chrome_crash_recovery', << pipeline.git.branch >> ]
+    - equal: [ 'mschile/issue-26900_browserCriClient', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -72,7 +72,7 @@ windowsWorkflowFilters: &windows-workflow-filters
     - equal: [ develop, << pipeline.git.branch >> ]
     # use the following branch as well to ensure that v8 snapshot cache updates are fully tested
     - equal: [ 'update-v8-snapshot-cache-on-develop', << pipeline.git.branch >> ]
-    - equal: [ 'fix/chrome_crash_recovery', << pipeline.git.branch >> ]
+    - equal: [ 'mschile/issue-26900_browserCriClient', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -139,7 +139,7 @@ commands:
       - run:
           name: Check current branch to persist artifacts
           command: |
-            if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "update-v8-snapshot-cache-on-develop" && "$CIRCLE_BRANCH" != "fix/chrome_crash_recovery" ]]; then
+            if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "update-v8-snapshot-cache-on-develop" && "$CIRCLE_BRANCH" != "mschile/issue-26900_browserCriClient" ]]; then
               echo "Not uploading artifacts or posting install comment for this branch."
               circleci-agent step halt
             fi

packages/server/lib/browsers/electron.ts

@@ -114,10 +114,6 @@ async function recordVideo (cdpAutomation: CdpAutomation, videoApi: RunModeVideo
 }
 
 export = {
-  // For testing
-  _clearBrowserCriClient () {
-    browserCriClient = null
-  },
   _defaultOptions (projectRoot: string | undefined, state: Preferences, options: BrowserLaunchOpts, automation: Automation): ElectronOpts {
     const _this = this
 
@@ -431,10 +427,8 @@ export = {
     browserCriClient = null
   },
 
-  async connectToNewSpec (browser: Browser, options: ElectronOpts, automation: Automation) {
-    if (!options.url) throw new Error('Missing url in connectToNewSpec')
-
-    return this.open(browser, options.url, options, automation)
+  connectToNewSpec (browser: Browser, options: ElectronOpts, automation: Automation) {
+    throw new Error('Attempting to connect to a new spec is not supported for electron, use open instead')
   },
 
   connectToExisting () {

packages/server/lib/browsers/index.ts

@@ -140,13 +140,7 @@ export = {
   async connectToNewSpec (browser: Browser, options: BrowserNewTabOpts, automation: Automation): Promise<BrowserInstance | null> {
     const browserLauncher = await getBrowserLauncher(browser, options.browsers)
 
-    const newInstance = await browserLauncher.connectToNewSpec(browser, options, automation)
-
-    // if a new instance was returned, update our instance to use the new one
-    if (newInstance) {
-      instance = newInstance
-      instance.browser = browser
-    }
+    await browserLauncher.connectToNewSpec(browser, options, automation)
 
     return this.getBrowserInstance()
   },

packages/server/lib/browsers/types.ts

@@ -30,7 +30,7 @@ export type BrowserInstance = EventEmitter & {
 
 export type BrowserLauncher = {
   open: (browser: Browser, url: string, options: BrowserLaunchOpts, automation: Automation) => Promise<BrowserInstance>
-  connectToNewSpec: (browser: Browser, options: BrowserNewTabOpts, automation: Automation) => Promise<BrowserInstance | void>
+  connectToNewSpec: (browser: Browser, options: BrowserNewTabOpts, automation: Automation) => Promise<void>
   /**
    * Used in Cypress-in-Cypress tests to connect to the existing browser instance.
    */

packages/server/lib/open_project.ts

@@ -174,7 +174,9 @@ export class OpenProject {
         return await browsers.connectToExisting(browser, options, automation)
       }
 
-      if (options.shouldLaunchNewTab) {
+      // if we should launch a new tab and we are not running in electron (which does not support connecting to a new spec)
+      // then we can connect to the new spec
+      if (options.shouldLaunchNewTab && browser.name !== 'electron') {
         const onInitializeNewBrowserTab = async () => {
           await this.resetBrowserState()
         }

packages/server/test/integration/cypress_spec.js

@@ -526,7 +526,7 @@ describe('lib/cypress', () => {
       .then(() => {
         expect(browsers.open).to.be.calledWithMatch(ELECTRON_BROWSER, { url: 'http://localhost:8888/__/#/specs/runner?file=tests/test2.coffee' })
       }).then(() => {
-        expect(browsers.connectToNewSpec).to.be.calledWithMatch(ELECTRON_BROWSER, { url: 'http://localhost:8888/__/#/specs/runner?file=tests/test1.js' })
+        expect(browsers.open).to.be.calledWithMatch(ELECTRON_BROWSER, { url: 'http://localhost:8888/__/#/specs/runner?file=tests/test1.js' })
         this.expectExitWith(0)
       })
     })

packages/server/test/unit/browsers/electron_spec.js

@@ -73,6 +73,7 @@ describe('lib/browsers/electron', () => {
     this.browserCriClient = {
       attachToTargetUrl: sinon.stub().resolves(this.pageCriClient),
       currentlyAttachedTarget: this.pageCriClient,
+      close: sinon.stub().resolves(),
     }
 
     sinon.stub(BrowserCriClient, 'create').resolves(this.browserCriClient)
@@ -92,14 +93,14 @@ describe('lib/browsers/electron', () => {
   })
 
   afterEach(function () {
-    electron._clearBrowserCriClient()
+    electron.clearInstanceState()
   })
 
   context('.connectToNewSpec', () => {
-    it('calls open with the browser, url, options, and automation', async function () {
-      sinon.stub(electron, 'open').withArgs({ isHeaded: true }, 'http://www.example.com', { url: 'http://www.example.com' }, this.automation)
-      await electron.connectToNewSpec({ isHeaded: true }, { url: 'http://www.example.com' }, this.automation)
-      expect(electron.open).to.be.called
+    it('throws an error', async function () {
+      expect(() => {
+        electron.connectToNewSpec({ isHeaded: true }, { url: 'http://www.example.com' }, this.automation)
+      }).to.throw('Attempting to connect to a new spec is not supported for electron, use open instead')
     })
   })
 

packages/server/test/unit/open_project_spec.js

@@ -204,10 +204,15 @@ describe('lib/open_project', () => {
         })
       })
 
-      it('calls connectToNewSpec when shouldLaunchNewTab is set', async function () {
+      it('calls connectToNewSpec when shouldLaunchNewTab is set and the browser is not electron', async function () {
         await openProject.launch(this.browser, this.spec, { shouldLaunchNewTab: true })
         expect(browsers.connectToNewSpec.lastCall.args[0]).to.be.equal(this.browser)
       })
+
+      it('calls open when shouldLaunchNewTab is set and the browser is electron', async function () {
+        await openProject.launch({ name: 'electron' }, this.spec, { shouldLaunchNewTab: true })
+        expect(browsers.open).to.have.been.calledOnce
+      })
     })
   })