fix: Only save session if validation succeeds (#24565)

Author: chrisbreiding

packages/app/cypress/e2e/runner/sessions.ui.cy.ts

@@ -132,6 +132,46 @@ describe('runner/cypress sessions.ui.spec', {
     // cy.percySnapshot() // TODO: restore when Percy CSS is fixed. See https://github.com/cypress-io/cypress/issues/23435
   })
 
+  // https://github.com/cypress-io/cypress/issues/24208
+  it('does not save session when validation fails', () => {
+    loadSpec({
+      projectName: 'session-and-origin-e2e-specs',
+      filePath: 'session/new_session_and_fails_validation_retries.cy.js',
+      failCount: 1,
+    })
+
+    validateSessionsInstrumentPanel(['blank_session'])
+
+    cy.contains('Attempt 1').click()
+    cy.get('.attempt-item').eq(0).within(() => {
+      cy.contains('validation error')
+    })
+
+    cy.get('.attempt-item').eq(1).within(() => {
+      cy.contains('validation error')
+      // when we stored sessions pre-validation, the 2nd attempt would fail
+      // with this error instead of the validation failing again
+      cy.contains('session already exists').should('not.exist')
+    })
+  })
+
+  it('creates, not recreates, session when validation fails then succeeds', () => {
+    loadSpec({
+      projectName: 'session-and-origin-e2e-specs',
+      filePath: 'session/new_session_and_fails_then_succeeds_validation_retries.cy.js',
+      failCount: 1,
+    })
+
+    validateSessionsInstrumentPanel(['blank_session'])
+
+    cy.get('.attempt-item').eq(1).within(() => {
+      cy.contains('Create new session')
+      // when we stored sessions pre-validation, the 2nd attempt would
+      // say "Recreate session"
+      cy.contains('Recreate session').should('not.exist')
+    })
+  })
+
   it('restores session as expected', () => {
     loadSpec({
       projectName: 'session-and-origin-e2e-specs',

packages/app/cypress/e2e/runner/support/spec-loader.ts

@@ -2,6 +2,7 @@ export const shouldHaveTestResults = ({ passCount, failCount, pendingCount }) =>
   passCount = passCount || '--'
   failCount = failCount || '--'
 
+  cy.get('button.restart', { timeout: 30000 }).should('be.visible') // ensure tests are finished running
   cy.findByLabelText('Stats', { timeout: 10000 }).within(() => {
     cy.get('.passed .num', { timeout: 30000 }).should('have.text', `${passCount}`)
     cy.get('.failed .num', { timeout: 30000 }).should('have.text', `${failCount}`)

packages/driver/cypress/e2e/commands/sessions/manager.cy.ts

@@ -235,7 +235,6 @@ describe('src/cy/commands/sessions/manager.ts', () => {
   describe('.sessions', () => {
     it('sessions.defineSession()', () => {
       const sessionsManager = new SessionsManager(CypressInstance, cy)
-      const sessionsSpy = cy.stub(sessionsManager, 'setActiveSession')
       const setup = cy.stub()
       const sess = sessionsManager.sessions.defineSession({ id: '1', setup })
 
@@ -249,9 +248,6 @@ describe('src/cy/commands/sessions/manager.ts', () => {
         sessionStorage: null,
         hydrated: false,
       })
-
-      expect(sessionsSpy).to.be.calledOnce
-      expect(sessionsSpy.getCall(0).args[0]).to.deep.eq({ 1: sess })
     })
 
     it('sessions.clearAllSavedSessions()', async () => {

packages/driver/src/cy/commands/sessions/index.ts

@@ -145,8 +145,6 @@ export default function (Commands, Cypress, cy) {
           validate: options.validate,
           cacheAcrossSpecs: options.cacheAcrossSpecs,
         })
-
-        sessionsManager.registeredSessions.set(id, true)
       }
 
       function setSessionLogStatus (status: string) {
@@ -159,7 +157,7 @@ export default function (Commands, Cypress, cy) {
         })
       }
 
-      function createSession (existingSession, recreateSession = false) {
+      function createSession (existingSession: SessionData, recreateSession = false) {
         logGroup(Cypress, {
           name: 'session',
           displayName: recreateSession ? 'Recreate session' : 'Create new session',
@@ -187,7 +185,6 @@ export default function (Commands, Cypress, cy) {
 
             _.extend(existingSession, data)
             existingSession.hydrated = true
-            await sessions.saveSessionData(existingSession)
 
             _log.set({ consoleProps: () => getConsoleProps(existingSession) })
 
@@ -348,7 +345,7 @@ export default function (Commands, Cypress, cy) {
        *   1. create session
        *   2. validate session
        */
-      const createSessionWorkflow = (existingSession, recreateSession = false) => {
+      const createSessionWorkflow = (existingSession: SessionData, recreateSession = false) => {
         return cy.then(async () => {
           setSessionLogStatus(recreateSession ? 'recreating' : 'creating')
 
@@ -358,11 +355,14 @@ export default function (Commands, Cypress, cy) {
           return createSession(existingSession, recreateSession)
         })
         .then(() => validateSession(existingSession))
-        .then((isValidSession: boolean) => {
+        .then(async (isValidSession: boolean) => {
           if (!isValidSession) {
             return
           }
 
+          sessionsManager.registeredSessions.set(existingSession.id, true)
+          await sessions.saveSessionData(existingSession)
+
           setSessionLogStatus(recreateSession ? 'recreated' : 'created')
         })
       }
@@ -373,7 +373,7 @@ export default function (Commands, Cypress, cy) {
        *   2. validate session
        *   3. if validation fails, catch error and recreate session
        */
-      const restoreSessionWorkflow = (existingSession) => {
+      const restoreSessionWorkflow = (existingSession: SessionData) => {
         return cy.then(async () => {
           setSessionLogStatus('restoring')
           await navigateAboutBlank()

packages/driver/src/cy/commands/sessions/manager.ts

@@ -116,7 +116,7 @@ export default class SessionsManager {
   // this the public api exposed to consumers as Cypress.session
   sessions = {
     defineSession: (options = {} as any): SessionData => {
-      const sess_state: SessionData = {
+      return {
         id: options.id,
         cookies: null,
         localStorage: null,
@@ -126,10 +126,6 @@ export default class SessionsManager {
         validate: options.validate,
         cacheAcrossSpecs: !!options.cacheAcrossSpecs,
       }
-
-      this.setActiveSession({ [sess_state.id]: sess_state })
-
-      return sess_state
     },
 
     clearAllSavedSessions: async () => {

system-tests/projects/session-and-origin-e2e-specs/cypress/e2e/session/new_session_and_fails_then_succeeds_validation_retries.cy.js

@@ -0,0 +1,14 @@
+/**
+ * Used in cy-in-cy tests in @packages/app.
+ */
+it('t1', { retries: 1 }, () => {
+  const setupFn = cy.stub()
+  const validateFn = cy.stub()
+
+  validateFn.onCall(0).throws(new Error('validation error'))
+  validateFn.onCall(1).returns(true)
+
+  cy.session('blank_session', setupFn, {
+    validate: validateFn,
+  })
+})

system-tests/projects/session-and-origin-e2e-specs/cypress/e2e/session/new_session_and_fails_validation_retries.cy.js

@@ -0,0 +1,11 @@
+/**
+ * Used in cy-in-cy tests in @packages/app.
+ */
+it('t1', { retries: 1 }, () => {
+  const setupFn = cy.stub()
+  const validateFn = cy.stub().throws(new Error('validation error'))
+
+  cy.session('blank_session', setupFn, {
+    validate: validateFn,
+  })
+})