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: use close event when asking the browser for its version #16312

merged 1 commit into from May 5, 2021


Copy link

@bahmutov bahmutov commented May 3, 2021

User facing changelog

  • When verifying Cypress, we now listen for the 'close' event instead of the 'exit' event in an effort to fix some situations where the browser cannot be found even though it is on the system.

Additional details

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?

Copy link

cypress-bot bot commented May 3, 2021

Thanks for taking the time to open a PR!

Copy link

cypress bot commented May 3, 2021

Test summary

4032 0 52 1Flakiness 0

Run details

Project cypress
Status Passed
Commit dbf9ea5
Started May 5, 2021 3:29 PM
Ended May 5, 2021 3:40 PM
Duration 11:17 💡
OS Linux Debian - 10.8
Browser Firefox 77

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

@bahmutov bahmutov requested a review from flotwig May 3, 2021 22:16
Copy link
Contributor Author

bahmutov commented May 3, 2021

@flotwig can you take a look at this? Seems quite a simple change that should not break stuff - and maybe solve the very annoying problem the users are reporting. I could not recreate the error in Docker or in simple Node examples though :(

Copy link

flotwig commented May 4, 2021

I see, so the difference between exit and close is that stdio streams can still be open when exit happens, but they are closed by close...

But the Node docs say:

The 'close' event is emitted when the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams.

Which makes me wonder if this will not help, since I don't think we have multiple processes sharing the same stdio streams here.

It seems to work fine at least, so we can merge it and see if it helps 🤷

@jennifer-shehane jennifer-shehane marked this pull request as ready for review May 4, 2021 16:19
@jennifer-shehane jennifer-shehane requested a review from a team as a code owner May 4, 2021 16:19
@jennifer-shehane jennifer-shehane requested review from chrisbreiding and removed request for a team May 4, 2021 16:19
Copy link

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

lg, needs a user-facing changelog and zenhub release before it can be merged

Copy link
Contributor Author

bahmutov commented May 4, 2021

kk, let's see if it helps with user problems, thanks @flotwig

Copy link

jennifer-shehane commented May 5, 2021

@bahmutov I wrote a user changelog line and tagged the PR. Going to merge after the tests pass.

@jennifer-shehane jennifer-shehane merged commit bdde38d into develop May 5, 2021
tgriesser added a commit that referenced this pull request May 7, 2021
* develop: (28 commits)
  fix: XHR event listener AUT redirect bug (#15995)
  chore: fix typo (#16345)
  chore(design-system): Added missing exports and index.ts (#16351)
  chore: Remove extra renovate.json file (#16385)
  Do not print 'uploading' stdout when --quiet mode is passed (#16271)
  fix(deps): update dependency color-string to version 1.5.5 🌟 (#16362)
  fix(deps): update dependency cypress-real-events to version 1.4.0 🌟 (#16363)
  tests: use the proper keys for selecting framework
  fix: Prevent Firefox from opening custom search when pressing / (#16372)
  fix(socket): update serialization for circular binary socket messages (#16311)
  fix: vueCli and webpack key vue@2 fix when guessing
  tests: update snapshots
  fix: add return config for vueCli and vueWebpack
  chore(deps): update dependency classnames to version 2.3.1 🌟 (#8337)
  fix: add return config for vitejs templates
  chore: release @cypress/vue-v2.2.2
  chore: release @cypress/react-v5.5.0
  fix: remove all of rollup, not supported anymore
  fix: typo in the final message (run vs run-ct)
  fix: use close event when asking the browser for its version (#16312)
Copy link

cypress-bot bot commented May 10, 2021

Released in 7.3.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v7.3.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants