fix: browser-skipped tests are correctly recorded to the dashboard (#…

…24217)

packages/app/cypress/e2e/runner/retries.mochaEvents.cy.ts

@@ -11,7 +11,6 @@ import { snapshots } from './retries.mochaEvents.snapshots'
  */
 describe('src/cypress/runner retries mochaEvents', { retries: 0, defaultCommandTimeout: 7500 }, () => {
   // NOTE: for test-retries
-
   it('simple retry', (done) => {
     const { assertMatchingSnapshot } = runCypressInCypressMochaEventsTest(
       snapshots,

packages/app/cypress/e2e/runner/support/mochaEventsUtils.ts

@@ -134,13 +134,6 @@ declare global {
   }
 }
 
-class SnapshotError extends Error {
-  constructor (message: string) {
-    super()
-    this.message = `\n${message}`
-  }
-}
-
 export function runCypressInCypressMochaEventsTest<T> (snapshots: T, snapToCompare: keyof T, done: Mocha.Done) {
   const bus = new EventEmitter()
   const outerRunner = window.top!.window
@@ -157,10 +150,17 @@ export function runCypressInCypressMochaEventsTest<T> (snapshots: T, snapToCompa
     if (diff !== '') {
       /* eslint-disable no-console */
       console.info('Received snapshot:', JSON.stringify(snapshot, null, 2))
-      throw new SnapshotError(diff)
+
+      return cy.fail(new Error(`The captured mocha events did not match the "${String(snapToCompare)}" snapshot.\n${diff}`), { async: false })
     }
 
-    done()
+    Cypress.log({
+      name: 'assert',
+      message: `The captured mocha events for the spec matched the "${String(snapToCompare)}" snapshot!`,
+      state: 'passed',
+    })
+
+    return done()
   })
 
   const assertMatchingSnapshot = (win: Cypress.AUTWindow) => {

packages/driver/src/cypress/error_utils.ts

@@ -9,7 +9,7 @@ import $stackUtils, { StackAndCodeFrameIndex } from './stack_utils'
 import $utils from './utils'
 import type { HandlerType } from './runner'
 
-const ERROR_PROPS = 'message type name stack parsedStack fileName lineNumber columnNumber host uncaught actual expected showDiff isPending docsUrl codeFrame'.split(' ')
+const ERROR_PROPS = ['message', 'type', 'name', 'stack', 'parsedStack', 'fileName', 'lineNumber', 'columnNumber', 'host', 'uncaught', 'actual', 'expected', 'showDiff', 'isPending', 'docsUrl', 'codeFrame'] as const
 const ERR_PREPARED_FOR_SERIALIZATION = Symbol('ERR_PREPARED_FOR_SERIALIZATION')
 
 const crossOriginScriptRe = /^script error/i

packages/driver/src/cypress/mocha.ts

@@ -35,13 +35,22 @@ const suiteAfterEach = Suite.prototype.afterEach
 delete (window as any).mocha
 delete (window as any).Mocha
 
-function invokeFnWithOriginalTitle (ctx, originalTitle, mochaArgs, fn, _testConfig) {
-  const ret = fn.apply(ctx, mochaArgs)
+type MochaArgs = [string, Function | undefined]
+function createRunnable (ctx, fnType: 'Test' | 'Suite', mochaArgs: MochaArgs, runnableFn: Function, testCallback: Function | string = '', _testConfig?: Record<string, any>) {
+  const runnable = runnableFn.apply(ctx, mochaArgs)
 
-  ret._testConfig = _testConfig
-  ret.originalTitle = originalTitle
+  // attached testConfigOverrides will execute before `runner:test:before:run` event
+  if (_testConfig) {
+    runnable._testConfig = _testConfig
+  }
+
+  if (fnType === 'Test') {
+    // persist the original callback so we can send it to the cloud
+    // to prevent it from being registered as a modified test
+    runnable.body = testCallback.toString()
+  }
 
-  return ret
+  return runnable
 }
 
 function overloadMochaFnForConfig (fnName, specWindow) {
@@ -66,40 +75,44 @@ function overloadMochaFnForConfig (fnName, specWindow) {
 
       const origFn = subFn ? _fn[subFn] : _fn
 
+      // fallback to empty string for stubbed runnables written like:
+      // - describe('concept')
+      // - it('does something')
+      let testCallback = args[1]
+
       if (args.length > 2 && _.isObject(args[1])) {
         const _testConfig = _.extend({}, args[1]) as any
 
-        const mochaArgs = [args[0], args[2]]
+        const mochaArgs: MochaArgs = [args[0], args[2]]
+        const originalTitle = mochaArgs[0]
+
+        // fallback to empty string for stubbed runnables written like:
+        // - describe('concept')
+        // - it('does something')
+        testCallback = mochaArgs[1]
 
         const configMatchesBrowser = _testConfig.browser == null || Cypress.isBrowser(_testConfig.browser, `${fnType} config value \`{ browser }\``)
 
         if (!configMatchesBrowser) {
-          // TODO: this would mess up the dashboard since it would be registered as a new test
-          const originalTitle = mochaArgs[0]
-
           mochaArgs[0] = `${originalTitle} (skipped due to browser)`
 
-          // TODO: weird edge case where you have an .only but also skipped the test due to the browser
+          // skip test at run-time when test is marked with .only but should also be skipped the test due to the browser
           if (subFn === 'only') {
             mochaArgs[1] = function () {
               this.skip()
             }
 
-            return invokeFnWithOriginalTitle(this, originalTitle, mochaArgs, origFn, _testConfig)
+            return createRunnable(this, fnType, mochaArgs, origFn, testCallback, _testConfig)
           }
 
-          return invokeFnWithOriginalTitle(this, originalTitle, mochaArgs, _fn['skip'], _testConfig)
+          // skip test with .skip func to ignore the test case and not run it
+          return createRunnable(this, fnType, mochaArgs, _fn['skip'], testCallback, _testConfig)
         }
 
-        const ret = origFn.apply(this, mochaArgs)
-
-        // attached testConfigOverrides will execute before `runner:test:before:run` event
-        ret._testConfig = _testConfig
-
-        return ret
+        return createRunnable(this, fnType, mochaArgs, origFn, testCallback, _testConfig)
       }
 
-      return origFn.apply(this, args)
+      return createRunnable(this, fnType, args as MochaArgs, origFn, testCallback)
     }
   }
 

packages/driver/src/cypress/runner.ts

@@ -14,15 +14,17 @@ import type { Emissions } from '@packages/types'
 const mochaCtxKeysRe = /^(_runnable|test)$/
 const betweenQuotesRe = /\"(.+?)\"/
 
-const HOOKS = 'beforeAll beforeEach afterEach afterAll'.split(' ')
+const HOOKS = ['beforeAll', 'beforeEach', 'afterEach', 'afterAll'] as const
 const TEST_BEFORE_RUN_ASYNC_EVENT = 'runner:test:before:run:async'
 // event fired before hooks and test execution
 const TEST_BEFORE_RUN_EVENT = 'runner:test:before:run'
 const TEST_AFTER_RUN_EVENT = 'runner:test:after:run'
 const TEST_AFTER_RUN_ASYNC_EVENT = 'runner:runnable:after:run:async'
 
-const RUNNABLE_LOGS = 'routes agents commands hooks'.split(' ')
-const RUNNABLE_PROPS = '_testConfig id order title _titlePath root hookName hookId err state failedFromHookId body speed type duration wallClockStartedAt wallClockDuration timings file originalTitle invocationDetails final currentRetry retries _slow'.split(' ')
+const RUNNABLE_LOGS = ['routes', 'agents', 'commands', 'hooks'] as const
+const RUNNABLE_PROPS = [
+  '_testConfig', 'id', 'order', 'title', '_titlePath', 'root', 'hookName', 'hookId', 'err', 'state', 'failedFromHookId', 'body', 'speed', 'type', 'duration', 'wallClockStartedAt', 'wallClockDuration', 'timings', 'file', 'originalTitle', 'invocationDetails', 'final', 'currentRetry', 'retries', '_slow',
+] as const
 
 const debug = debugFn('cypress:driver:runner')
 const debugErrors = debugFn('cypress:driver:errors')
@@ -147,11 +149,14 @@ const setWallClockDuration = (test) => {
 // tests to an id-based object which prevents
 // us from recursively iterating through every
 // parent since we could just return the found test
-const wrap = (runnable) => {
+const wrap = (runnable): Record<string, any> | null => {
   return $utils.reduceProps(runnable, RUNNABLE_PROPS)
 }
 
-const wrapAll = (runnable): any => {
+// Reduce runnable down to its props and collections.
+// Sent to the Reporter to populate command log
+// and send to the Dashboard when in record mode.
+const wrapAll = (runnable): Record<string, any> => {
   return _.extend(
     {},
     $utils.reduceProps(runnable, RUNNABLE_PROPS),
@@ -468,9 +473,8 @@ const overrideRunnerHook = (Cypress, _runner, getTestById, getTest, setTest, get
 
 const getTestResults = (tests) => {
   return _.map(tests, (test) => {
-    const obj: Record<string, any> = _.pick(test, 'id', 'duration', 'state')
+    const obj: Record<string, any> = _.pick(test, 'title', 'id', 'duration', 'state')
 
-    obj.title = test.originalTitle
     // TODO FIX THIS!
     if (!obj.state) {
       obj.state = 'skipped'
@@ -565,11 +569,7 @@ const normalize = (runnable, tests, initialTests, getRunnableId, getHookId, getO
       prevAttempts = []
 
       if (i.prevAttempts) {
-        prevAttempts = _.map(i.prevAttempts, (test) => {
-          // reduce this runnable down to its props
-          // and collections
-          return wrapAll(test)
-        })
+        prevAttempts = _.map(i.prevAttempts, wrapAll)
       }
 
       _.extend(runnable, i)
@@ -578,8 +578,6 @@ const normalize = (runnable, tests, initialTests, getRunnableId, getHookId, getO
     // merge all hooks into single array
     runnable.hooks = condenseHooks(runnable, getHookId)
 
-    // reduce this runnable down to its props
-    // and collections
     const wrappedRunnable = wrapAll(runnable)
 
     if (runnable.type === 'test') {

packages/driver/src/cypress/utils.ts

@@ -96,7 +96,7 @@ export default {
     throw new Error(`The switch/case value: '${value}' did not match any cases: ${keys.join(', ')}.`)
   },
 
-  reduceProps (obj, props: string[] = []) {
+  reduceProps (obj, props: readonly string[] = []) {
     if (!obj) {
       return null
     }

packages/server/lib/modes/record.js

@@ -735,15 +735,18 @@ const createRunAndRecordSpecs = (options = {}) => {
         const tests = _.chain(r[0])
         .uniqBy('id')
         .map((v) => {
-          if (v.originalTitle) {
-            v._titlePath.splice(-1, 1, v.originalTitle)
-          }
-
           return _.pick({
             ...v,
             clientId: v.id,
             config: v._testConfig?.unverifiedTestConfig || null,
-            title: v._titlePath,
+            title: v._titlePath.map((title) => {
+              // sanitize the title which may have been altered by a suite-/test-level
+              // browser skip to ensure the original title is used so the test recorded
+              // to the cloud is correct registered as a pending test
+              const BROWSER_SKIP_TITLE = ' (skipped due to browser)'
+
+              return title.replace(BROWSER_SKIP_TITLE, '')
+            }),
             hookIds: v.hooks.map((hook) => hook.hookId),
           },
           'clientId', 'body', 'title', 'config', 'hookIds')

packages/server/lib/reporter.js

@@ -41,31 +41,23 @@ overrideRequire((depPath, _load) => {
 // Mocha.Runnable.prototype.titlePath = ->
 //   @parent.titlePath().concat([@title])
 
-const getParentTitle = function (runnable, titles) {
-  // if the browser/reporter changed the runnable title (for display purposes)
-  // it will have .originalTitle which is the name of the test before title change
-  let p
-
+const getTitlePath = function (runnable, titles = []) {
+  // `originalTitle` is a Mocha Hook concept used to associated the
+  // hook to the test that executed it
   if (runnable.originalTitle) {
     runnable.title = runnable.originalTitle
   }
 
-  if (!titles) {
-    titles = [runnable.title]
-  }
-
-  p = runnable.parent
-
-  if (p) {
-    let t
+  if (runnable.title) {
+    // sanitize the title which may have been altered by a suite-/
+    // test-level browser skip to ensure the original title is used
+    const BROWSER_SKIP_TITLE = ' (skipped due to browser)'
 
-    t = p.title
-
-    if (t) {
-      titles.unshift(t)
-    }
+    titles.unshift(runnable.title.replace(BROWSER_SKIP_TITLE, ''))
+  }
 
-    return getParentTitle(p, titles)
+  if (runnable.parent) {
+    return getTitlePath(runnable.parent, titles)
   }
 
   return titles
@@ -385,15 +377,15 @@ class Reporter {
     return {
       hookId: hook.hookId,
       hookName: hook.hookName,
-      title: getParentTitle(hook),
+      title: getTitlePath(hook),
       body: hook.body,
     }
   }
 
   normalizeTest (test = {}) {
     const normalizedTest = {
       testId: orNull(test.id),
-      title: getParentTitle(test),
+      title: getTitlePath(test),
       state: orNull(test.state),
       body: orNull(test.body),
       displayError: orNull(test.err && test.err.stack),