From 841c82ddd4a21b4d8c6bcb1e2c9b0844778c9055 Mon Sep 17 00:00:00 2001
From: Ryan Manuel <ryanm@cypress.io>
Date: Fri, 10 Dec 2021 10:30:18 -0600
Subject: [PATCH] fix: Do not screenshot or trigger the failed event when tests
 are skipped

---
 .../integration/cy/skipping_async_spec.js     | 62 ++++++++++++++++
 .../integration/cy/skipping_sync_spec.js      | 70 +++++++++++++++++++
 packages/driver/src/cypress/command_queue.ts  |  8 +++
 packages/driver/src/cypress/cy.ts             |  6 ++
 4 files changed, 146 insertions(+)
 create mode 100644 packages/driver/cypress/integration/cy/skipping_async_spec.js
 create mode 100644 packages/driver/cypress/integration/cy/skipping_sync_spec.js

diff --git a/packages/driver/cypress/integration/cy/skipping_async_spec.js b/packages/driver/cypress/integration/cy/skipping_async_spec.js
new file mode 100644
index 000000000000..17a1dc02525b
--- /dev/null
+++ b/packages/driver/cypress/integration/cy/skipping_async_spec.js
@@ -0,0 +1,62 @@
+const { Screenshot } = Cypress
+
+let failedEventFired = false
+
+Cypress.on('fail', (error) => {
+  failedEventFired = true
+  throw new Error(error)
+})
+
+let screenshotTaken = false
+
+Screenshot.defaults({ onAfterScreenshot: () => {
+  screenshotTaken = true
+} })
+
+const pendingTests = []
+const passedTests = []
+
+Cypress.on('test:after:run', (test) => {
+  if (test.state === 'pending') {
+    return pendingTests.push(test)
+  }
+
+  if (test.state === 'passed') {
+    return passedTests.push(test)
+  }
+})
+
+beforeEach(() => {
+  // Set isInteractive to false to ensure that screenshots will be
+  // triggered in both run and open mode
+  Cypress.config('isInteractive', false)
+})
+
+describe('skipped test', () => {
+  it('does not fail', function () {
+    cy.then(() => {
+      this.skip()
+    }).then(() => {
+      expect(true).to.be.false
+    })
+  })
+
+  it('does not prevent subsequent tests from running', () => {
+    expect(true).to.be.true
+  })
+})
+
+describe('skipped test side effects', () => {
+  it('does not have a screenshot taken', () => {
+    expect(screenshotTaken).to.be.false
+  })
+
+  it('does not fire failed event', () => {
+    expect(failedEventFired).to.be.false
+  })
+
+  it('does still mark all tests with the correct state', () => {
+    expect(pendingTests).to.have.length(1)
+    expect(passedTests).to.have.length(3)
+  })
+})
diff --git a/packages/driver/cypress/integration/cy/skipping_sync_spec.js b/packages/driver/cypress/integration/cy/skipping_sync_spec.js
new file mode 100644
index 000000000000..48c53a6b0bb6
--- /dev/null
+++ b/packages/driver/cypress/integration/cy/skipping_sync_spec.js
@@ -0,0 +1,70 @@
+const { Screenshot } = Cypress
+
+let failedEventFired = false
+
+Cypress.on('fail', (error) => {
+  failedEventFired = true
+  throw new Error(error)
+})
+
+let screenshotTaken = false
+
+Screenshot.defaults({ onAfterScreenshot: () => {
+  screenshotTaken = true
+} })
+
+const pendingTests = []
+const passedTests = []
+
+Cypress.on('test:after:run', (test) => {
+  if (test.state === 'pending') {
+    return pendingTests.push(test)
+  }
+
+  if (test.state === 'passed') {
+    return passedTests.push(test)
+  }
+})
+
+beforeEach(() => {
+  // Set isInteractive to false to ensure that screenshots will be
+  // triggered in both run and open mode
+  Cypress.config('isInteractive', false)
+})
+
+describe('generally skipped test', () => {
+  before(function () {
+    this.skip()
+  })
+
+  it('does not fail', function () {
+    expect(true).to.be.false
+  })
+})
+
+describe('individually skipped tests', () => {
+  it('does not fail when using this.skip', function () {
+    this.skip()
+    expect(true).to.be.false
+  })
+
+  // NOTE: We are skipping this test in order to test skip functionality
+  it.skip('does not fail when using it.skip', () => {
+    expect(true).to.be.false
+  })
+})
+
+describe('skipped test side effects', () => {
+  it('does not have a screenshot taken', () => {
+    expect(screenshotTaken).to.be.false
+  })
+
+  it('does not fire failed event', () => {
+    expect(failedEventFired).to.be.false
+  })
+
+  it('does still mark all tests with the correct state', () => {
+    expect(pendingTests).to.have.length(3)
+    expect(passedTests).to.have.length(2)
+  })
+})
diff --git a/packages/driver/src/cypress/command_queue.ts b/packages/driver/src/cypress/command_queue.ts
index 95fcd4239ac3..d9321d35de45 100644
--- a/packages/driver/src/cypress/command_queue.ts
+++ b/packages/driver/src/cypress/command_queue.ts
@@ -343,6 +343,14 @@ export class CommandQueue extends Queue<Command> {
     }
 
     const onError = (err: Error | string) => {
+      // If the runnable was marked as pending, this test was skipped
+      // go ahead and just return
+      const runnable = this.state('runnable')
+
+      if (runnable.isPending()) {
+        return
+      }
+
       if (this.state('onCommandFailed')) {
         return this.state('onCommandFailed')(err, this, next)
       }
diff --git a/packages/driver/src/cypress/cy.ts b/packages/driver/src/cypress/cy.ts
index 38c753c830a7..39bb7c76320b 100644
--- a/packages/driver/src/cypress/cy.ts
+++ b/packages/driver/src/cypress/cy.ts
@@ -942,6 +942,12 @@ export class $Cy implements ITimeouts, IStability, IAssertions, IRetries, IJQuer
         // else just return ret
         return ret
       } catch (err) {
+        // If the runnable was marked as pending, this test was skipped
+        // go ahead and just return
+        if (runnable.isPending()) {
+          return
+        }
+
         // if runnable.fn threw synchronously, then it didnt fail from
         // a cypress command, but we should still teardown and handle
         // the error