fix(unify): Do not screenshot or trigger the failed event when tests are skipped

[ryanthemanuel]

• Closes Screenshot (failed) captured during skipped tests #17660
• Closes sync skip; aborting execution error thrown when this.skip() inside function #14867

User facing changelog

Tests that are skipped within then blocks will no longer throw errors causing the test to fail. Tests that are skipped outside of then blocks will no longer trigger the fail event. This will prevent screenshots from happening from errors thrown by the fail event.

Additional details

When a test is skipped, mocha triggers a "sync skip; aborting execution" error. In investigating #17660, it was determined that #14867 had the same root cause. The main difference is that #17660 deals with synchronous tests and #14867 deals with asynchronous tests. In both cases, the fix is to update the failure handling to check if the test is pending (mocha's indicator that the test was skipped) and not fail in that scenario.

How has the user experience changed?

#17660 Before:

#17660 After:

#14866 Before:

#14866 After:

PR Tasks

• Have tests been added/updated?
• [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
• [na] Has a PR for user-facing changes been opened in cypress-documentation?
• [na] Have API changes been updated in the type definitions?
• [na] Have new configuration options been added to the cypress.schema.json?

[cypress-bot]

Thanks for taking the time to open a PR!

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
• Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

[cypress]

---

Test summary

8502 • 1 • 105 • 0 • 1

---

Run details

Project	cypress
Status	Failed
Commit	a0c36a9
Started	Dec 9, 2021 9:18 PM
Ended	Dec 9, 2021 9:30 PM
Duration	11:48 💡
OS	Linux Debian - 10.10
Browser	Multiple

View run in Cypress Dashboard ➡️

---

Failures

	cypress/integration/commands/net_stubbing_spec.ts		1
1	network stubbing > intercepting request > should delay the same amount on every response

---

Flakiness

	cypress/integration/commands/net_stubbing_spec.ts		1
1	network stubbing > waiting and aliasing > can timeout incrementally waiting on requests

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[mjhenkes]

What specifically is erroring in the above try block? Could we check is pending up there and return instead of relying on the try block?

[ryanthemanuel]

Mocha is actually throwing the error: https://github.com/mochajs/mocha/blob/ac43029d6a86150a48ccd59e50e89ca10c72a9c0/lib/runnable.js#L143

I don't believe there's a way we could catch it earlier than this as it's being triggered directly from the test being executed here: https://github.com/cypress-io/cypress/pull/19279/files#diff-231d74866a51e1411d172ab6ca0658f7fa712e9676f216ba797c544c47dcbf94R1144

[chrisbreiding]

@ryanthemanuel Does this also fix this user's case? If so, I think we should add a test covering that. Probably need tests covering it.skip('test', () => {}) in addition to this.skip() in any case.

[ryanthemanuel]

@ryanthemanuel Does this also fix this user's case? If so, I think we should add a test covering that. Probably need tests covering it.skip('test', () => {}) in addition to this.skip() in any case.

Good catch @chrisbreiding I added the test case and ensured that snapshots actually would be generated in this case. Previously they were not due to configuration for the driver tests. I was unable to recreate the issue described here

[chrisbreiding]

Looks good 👍 It's just a nitpick, but I prefer not to prefix every test title with 'should'. Just ends up being a bit redundant.