Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct bad propagation of exit signals #7755

Merged
merged 2 commits into from Jun 22, 2020
Merged

fix: correct bad propagation of exit signals #7755

merged 2 commits into from Jun 22, 2020

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Jun 18, 2020

User facing changelog

  • Fixed an issue where Cypress could exit successfully even with failing tests under certain conditions.

Additional details

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue been tagged with a release in ZenHub?
  • [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
Copy link
Contributor

cypress-bot bot commented Jun 18, 2020

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Jun 18, 2020



Test summary

7644 0 119 0


Run details

Project cypress
Status Passed
Commit da32a8f
Started Jun 22, 2020 5:33 PM
Ended Jun 22, 2020 5:40 PM
Duration 06:41 💡
OS Linux Debian - 10.1
Browser Multiple

View run in Cypress Dashboard ➡️


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 flotwig marked this pull request as ready for review June 19, 2020 15:12
@flotwig flotwig requested review from a team, brian-mann and kuceb and removed request for a team June 19, 2020 15:12
Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to only change when we fork the process based on the usage of NODE_OPTIONS which to me does not seem to reflect the reason this issue was opened (it mentions the use of global mode).

How would changing the util/node_options affect global mode?

@flotwig
Copy link
Contributor Author

flotwig commented Jun 22, 2020

@brian-mann util/node_options relaunches the Electron app if it is launched without the correct NODE_OPTIONS. The CLI always launches the Electron app with the correct options, so in released versions of Cypress, Electron will only practically be relaunched via util/node_options if launched in global mode (since the user will presumably not pass the expected NODE_OPTIONS).

@flotwig
Copy link
Contributor Author

flotwig commented Jun 22, 2020

Other situations where the binary is launched directly (scripts, our own e2e tests...) will also benefit from this fix, which is why the changelog mentions "certain conditions" and not "global mode" explicitly.

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is fixing a userland bug, it needs tests. you can unit test this specific function if you'd like if its too difficult to recreate its usage in the real world.

we have some existing test patterns for testing cp.spawn methods you could look for in the CLI, etc.

@flotwig flotwig requested a review from brian-mann June 22, 2020 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit usage of on('exit') to propagate exits + fix buggy usages
2 participants