fix: handle async before:spec event handler (#26055)

Co-authored-by: Ryan Manuel <ryanm@cypress.io>
cypress-io · Mar 13, 2023 · 1c5a67f · 1c5a67f · cypress-bot · Mar 13, 2023
1 parent 9c98b5e
commit 1c5a67f
Show file tree

Hide file tree

Showing 23 changed files with 210 additions and 65 deletions.
diff --git a/.circleci/workflows.yml b/.circleci/workflows.yml
@@ -30,8 +30,7 @@ mainBuildFilters: &mainBuildFilters
         - /^release\/\d+\.\d+\.\d+$/
         # use the following branch as well to ensure that v8 snapshot cache updates are fully tested
         - 'update-v8-snapshot-cache-on-develop'
-        - 'lmiller/issue-25947'
-        - 'fix/preflight'
+        - 'emily/before-spec-promise'
 
 # usually we don't build Mac app - it takes a long time
 # but sometimes we want to really confirm we are doing the right thing
@@ -42,8 +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/preflight', << pipeline.git.branch >> ]
-    - equal: [ 'lmiller/issue-25947', << pipeline.git.branch >> ]
+    - equal: [ 'emily/before-spec-promise', << pipeline.git.branch >> ]
     - matches:
         pattern: /^release\/\d+\.\d+\.\d+$/
         value: << pipeline.git.branch >>
@@ -141,7 +139,7 @@ commands:
       - run:
           name: Check current branch to persist artifacts
           command: |
-            if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "lmiller/issue-25947" && "$CIRCLE_BRANCH" != "update-v8-snapshot-cache-on-develop" ]]; then
+            if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "release/"* && "$CIRCLE_BRANCH" != "emily/before-spec-promise" && "$CIRCLE_BRANCH" != "update-v8-snapshot-cache-on-develop" ]]; then
               echo "Not uploading artifacts or posting install comment for this branch."
               circleci-agent step halt
             fi

diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md
@@ -11,6 +11,7 @@ _Released 03/14/2023 (PENDING)_
 **Bugfixes:**
 
 - Fixed an issue where using `Cypress.require()` would throw the error `Cannot find module 'typescript'`. Fixes [#25885](https://github.com/cypress-io/cypress/issues/25885).
+- The [`before:spec`](https://docs.cypress.io/api/plugins/before-spec-api) API was updated to correctly support async event handlers in `run` mode. Fixes [#24403](https://github.com/cypress-io/cypress/issues/24403).
 
 **Misc:**
 

diff --git a/packages/app/src/example.spec.ts b/packages/app/src/example.spec.ts
diff --git a/packages/app/src/runner/event-manager.ts b/packages/app/src/runner/event-manager.ts
@@ -8,9 +8,8 @@ import type { RunState, CachedTestState, AutomationElementId, FileDetails, Repor
 
 import { logger } from './logger'
 import type { Socket } from '@packages/socket/lib/browser'
-import { automation, useRunnerUiStore } from '../store'
+import { automation, useRunnerUiStore, useSpecStore } from '../store'
 import { useScreenshotStore } from '../store/screenshot-store'
-import { useSpecStore } from '../store/specs-store'
 import { useStudioStore } from '../store/studio-store'
 import { getAutIframeModel } from '.'
 import { handlePausing } from './events/pausing'
@@ -360,20 +359,30 @@ export class EventManager {
     }
   }
 
-  setup (config) {
+  async setup (config) {
+    this.ws.emit('watch:test:file', config.spec)
+
+    if (config.isTextTerminal || config.experimentalInteractiveRunEvents) {
+      await new Promise((resolve, reject) => {
+        this.ws.emit('plugins:before:spec', config.spec, (res?: { error: Error }) => {
+          // FIXME: handle surfacing the error to the browser instead of hanging with
+          // 'Your tests are loading...' message. Fix in https://github.com/cypress-io/cypress/issues/23627
+          if (res && res.error) {
+            reject(res.error)
+          }
+
+          resolve(null)
+        })
+      })
+    }
+
     Cypress = this.Cypress = this.$CypressDriver.create(config)
 
     // expose Cypress globally
     // @ts-ignore
     window.Cypress = Cypress
 
     this._addListeners()
-
-    this.ws.emit('watch:test:file', config.spec)
-
-    if (config.isTextTerminal || config.experimentalInteractiveRunEvents) {
-      this.ws.emit('plugins:before:spec', config.spec)
-    }
   }
 
   isBrowser (browserName) {

diff --git a/packages/app/src/runner/index.ts b/packages/app/src/runner/index.ts
@@ -401,7 +401,7 @@ async function executeSpec (spec: SpecFile, isRerun: boolean = false) {
 
   // creates a new instance of the Cypress driver for this spec,
   // initializes a bunch of listeners watches spec file for changes.
-  getEventManager().setup(config)
+  await getEventManager().setup(config)
 
   if (window.__CYPRESS_TESTING_TYPE__ === 'e2e') {
     return runSpecE2E(config, spec)

diff --git a/packages/server/lib/modes/run.ts b/packages/server/lib/modes/run.ts
@@ -601,7 +601,7 @@ async function waitForTestsToFinishRunning (options: { project: Project, screens
     }
   }
 
-  await runEvents.execute('after:spec', config, spec, results)
+  await runEvents.execute('after:spec', spec, results)
   debug('executed after:spec')
 
   const videoName = videoRecording?.api.videoName
@@ -746,7 +746,7 @@ async function runSpecs (options: { config: Cfg, browser: Browser, sys: any, hea
     autoCancelAfterFailures,
   }
 
-  await runEvents.execute('before:run', config, beforeRunDetails)
+  await runEvents.execute('before:run', beforeRunDetails)
 
   const runs = await iterateThroughSpecs({
     specs,
@@ -816,7 +816,7 @@ async function runSpecs (options: { config: Cfg, browser: Browser, sys: any, hea
     })),
   })
 
-  await runEvents.execute('after:run', config, moduleAPIResults)
+  await runEvents.execute('after:run', moduleAPIResults)
   await writeOutput(outputPath, moduleAPIResults)
 
   return results

diff --git a/packages/server/lib/open_project.ts b/packages/server/lib/open_project.ts
@@ -136,7 +136,7 @@ export class OpenProject {
         return Bluebird.resolve()
       }
 
-      return runEvents.execute('after:spec', cfg, spec)
+      return runEvents.execute('after:spec', spec)
     }
 
     const { onBrowserClose } = options

diff --git a/packages/server/lib/plugins/index.ts b/packages/server/lib/plugins/index.ts
@@ -1,5 +1,6 @@
 import {
-  getCtx, registerServerPluginHandler,
+  getCtx,
+  registerServerPluginHandler,
 } from '@packages/data-context'
 
 export const registerEvent = (event, callback) => {

diff --git a/packages/server/lib/plugins/run_events.js b/packages/server/lib/plugins/run_events.js
@@ -4,7 +4,7 @@ const errors = require('../errors')
 const plugins = require('../plugins')
 
 module.exports = {
-  execute: Promise.method((eventName, config = {}, ...args) => {
+  execute: Promise.method((eventName, ...args) => {
     if (!plugins.has(eventName)) return
 
     return plugins.execute(eventName, ...args)

diff --git a/packages/server/lib/project-base.ts b/packages/server/lib/project-base.ts
@@ -235,7 +235,7 @@ export class ProjectBase<TServer extends Server> extends EE {
 
     this.isOpen = true
 
-    return runEvents.execute('before:run', cfg, beforeRunDetails)
+    return runEvents.execute('before:run', beforeRunDetails)
   }
 
   reset () {
@@ -287,7 +287,7 @@ export class ProjectBase<TServer extends Server> extends EE {
 
     if (config.isTextTerminal || !config.experimentalInteractiveRunEvents) return
 
-    return runEvents.execute('after:run', config)
+    return runEvents.execute('after:run')
   }
 
   initializeReporter ({

diff --git a/packages/server/lib/socket-base.ts b/packages/server/lib/socket-base.ts
@@ -46,12 +46,14 @@ export class SocketBase {
   private _isRunnerSocketConnected
   private _sendFocusBrowserMessage
 
+  protected inRunMode: boolean
   protected supportsRunEvents: boolean
   protected ended: boolean
   protected _io?: socketIo.SocketIOServer
   localBus: EventEmitter
 
   constructor (config: Record<string, any>) {
+    this.inRunMode = config.isTextTerminal
     this.supportsRunEvents = config.isTextTerminal || config.experimentalInteractiveRunEvents
     this.ended = false
     this.localBus = new EventEmitter()
@@ -532,10 +534,17 @@ export class SocketBase {
       })
 
       if (this.supportsRunEvents) {
-        socket.on('plugins:before:spec', (spec) => {
-          runEvents.execute('before:spec', {}, spec).catch((error) => {
-            socket.disconnect()
-            throw error
+        socket.on('plugins:before:spec', (spec, cb) => {
+          runEvents.execute('before:spec', spec)
+          .then(cb)
+          .catch((error) => {
+            if (this.inRunMode) {
+              socket.disconnect()
+              throw error
+            }
+
+            // surfacing the error to the app in open mode
+            cb({ error })
           })
         })
       }

diff --git a/packages/server/test/unit/open_project_spec.js b/packages/server/test/unit/open_project_spec.js
@@ -131,7 +131,7 @@ describe('lib/open_project', () => {
           return browsers.open.lastCall.args[1].onBrowserClose()
         })
         .then(() => {
-          expect(runEvents.execute).to.be.calledWith('after:spec', this.config, this.spec)
+          expect(runEvents.execute).to.be.calledWith('after:spec', this.spec)
         })
       })
 

diff --git a/packages/server/test/unit/plugins/run_events_spec.js b/packages/server/test/unit/plugins/run_events_spec.js
@@ -13,7 +13,7 @@ describe('lib/plugins/run_events', () => {
     })
 
     it('returns a promise noop if event is not registered', () => {
-      return runEvents.execute('before:spec', {})
+      return runEvents.execute('before:spec')
       .then(() => {
         expect(plugins.execute).not.to.be.called
       })
@@ -23,7 +23,7 @@ describe('lib/plugins/run_events', () => {
       plugins.has.returns(true)
       plugins.execute.resolves('the result')
 
-      return runEvents.execute('before:spec', {}, 'arg1', 'arg2')
+      return runEvents.execute('before:spec', 'arg1', 'arg2')
       .then(() => {
         expect(plugins.execute).to.be.calledWith('before:spec', 'arg1', 'arg2')
       })
@@ -33,7 +33,7 @@ describe('lib/plugins/run_events', () => {
       plugins.has.returns(true)
       plugins.execute.resolves('the result')
 
-      return runEvents.execute('before:spec', {}, 'arg1', 'arg2')
+      return runEvents.execute('before:spec', 'arg1', 'arg2')
       .then((result) => {
         expect(result).to.equal('the result')
       })
@@ -43,7 +43,7 @@ describe('lib/plugins/run_events', () => {
       plugins.has.returns(true)
       plugins.execute.rejects({ name: 'Error', message: 'The event threw an error', stack: 'stack trace' })
 
-      return runEvents.execute('before:spec', {}, 'arg1', 'arg2')
+      return runEvents.execute('before:spec', 'arg1', 'arg2')
       .then(() => {
         expect(errors.throwErr).to.be.calledWith('PLUGINS_RUN_EVENT_ERROR', 'before:spec', { name: 'Error', message: 'The event threw an error', stack: 'stack trace' })
       })

diff --git a/packages/server/test/unit/project_spec.js b/packages/server/test/unit/project_spec.js
@@ -264,7 +264,7 @@ This option will not have an effect in Some-other-name. Tests that rely on web s
 
       return this.project.open()
       .then(() => {
-        expect(runEvents.execute).to.be.calledWith('before:run', this.config, {
+        expect(runEvents.execute).to.be.calledWith('before:run', {
           config: this.config,
           cypressVersion: pkg.version,
           system: sysInfo,
@@ -417,7 +417,7 @@ This option will not have an effect in Some-other-name. Tests that rely on web s
 
       return this.project.close()
       .then(() => {
-        expect(runEvents.execute).to.be.calledWith('after:run', this.config)
+        expect(runEvents.execute).to.be.calledWith('after:run')
       })
     })