fix(server): use run#run instead of run#ready to replicate e2e exit behavior in CT

[lmiller1990]

• Closes run-ct returns with exit code 0 when tests fail - breaking CI detection of failed tests #17752
• Closes Run CT w/ module API does not reproduce correct report #17885

User facing changelog

Fix a bug where correct exit code is not propagated when using run-ct with the bundled electron browser (13.x).

Additional details

See these posts for my original debugging. A regression was introduced when we updated to electron 12.x to 13.x. This was release in Cypress 8.3.

When running run-ct, even when tests fail, we would exit(0), instead of exit(num_failed_tests). This means your CI would always be green. This only happens when running run-ct using the bundled electron browser.

The specific electron version that introduced this problem was between [email protected] -> [email protected]. Diff is here: electron/electron@v13.0.0-beta.27...v13.0.0-beta.28. You will notice if you check out cypress@develop and use 13.0.0-beta.27 for electron, everything is okay, but if you update to 13.0.0-beta.28, it regresses.

I don't know what specifically in electron causes this problem. Even looking at the diff above in electron versions, I am not sure why.

Either way, there is a fix. run and run-ct take slightly different paths. run calls run#run (which calls run#ready). run-ct skips run#run and goes straight to run#ready.

By doing this, this critical part is skipped.

app.on('window-all-closed', () => {
  debug('all BrowserWindows closed, not exiting')
})

This appears to do nothing at all on first inspection. If we look at the electron docs...

Emitted when all windows have been closed.

If you do not subscribe to this event and all windows are closed, the default behavior is to quit the app; however, if you subscribe, you control whether the app quits or not. If the user pressed Cmd + Q, or the developer called app.quit(), Electron will first try to close all the windows and then emit the will-quit event, and in this case the window-all-closed event would not be emitted.

My understanding is by calling run#ready and bypassing run#run (which basically just subscribes to window-all-closed then calls run#run, we end up invoking the default behavior of the electron browser, which is to quit the app (with exit code 0).

By subscribing, we control whether the app quits or not. We use this control to quit with a specific exit code, which is the number of failed specs.

Again, this should been a problem in the past too - I'm not clear on why it only starting causing problems after the electron 13 upgrade.

Testing

We do not have many e2e tests around run-ct. It uses 99% of the same code as e2e, but the 1% is where the bug was. I modified the e2e test harness slightly and added one that correctly catches this regression.

You can also reproduce and test the bug fix as follows:

1. in develop, create a failing spec in runner-ct/cypress/components
2. run cd packages/runner-ct && yarn cypress:run (launches CT in run mode w/ electron)
3. observe that despite the failing spec, exit code is 0
4. check out this branch
5. repeat - notice exit code is now correct (1 if you have 1 failing spec)

You can also revert the electron version 12.x in develop and you'll notice the problem goes away.

How has the user experience changed?

PR Tasks

• Have tests been added/updated?
• Has the original issue or this PR been tagged with a release in ZenHub?
• Has a PR for user-facing changes been opened in cypress-documentation?
• Have API changes been updated in the type definitions?
• 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.

[lmiller1990]

We now match run-e2e.js: https://github.com/cypress-io/cypress/blob/develop/packages/server/lib/modes/run-e2e.js#L2

[cypress]

---

Test summary

4181 • 0 • 53 • 1 • 4

---

Run details

Project	cypress
Status	Passed
Commit	fd127ff
Started	Aug 26, 2021 3:33 PM
Ended	Aug 26, 2021 3:44 PM
Duration	10:44 💡
OS	Linux Debian - 10.9
Browser	Firefox 89

View run in Cypress Dashboard ➡️

---

Flakiness

	cypress/proxy-logging-spec.ts		1
1	Proxy Logging > request logging > fetch log shows resource type, url, method, and status code and has expected snapshots and consoleProps
	commands/net_stubbing_spec.ts		1
1	network stubbing > intercepting request > can intercept utf-8 request bodies without crashing
	commands/xhr_spec.js		2
1	... > no status when request isnt forced 404
2	src/cy/commands/xhr > abort > aborts xhrs currently in flight

---

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

[flotwig]

I don't know what specifically in electron causes this problem. Even looking at the diff above in electron versions, I am not sure why.

Me either, after reviewing the diff, I can tell that it changes some window-related behavior, so maybe what triggered this bug to pop up is a change in the timing characteristics of when windows-all-closed is fired.

[cellog]

I'm trying to apply this patch in node_modules/ of my installed version of 8.3.0 to test it, and cannot find any files with the comment, or .ready(options). Where does this file live in the installed version?