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.

Sign up for GitHub

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

Jump to bottom

fix: use close event when asking the browser for its version #16312

Merged
merged 1 commit into from May 5, 2021
Merged

fix: use close event when asking the browser for its version #16312

merged 1 commit into from May 5, 2021

Conversation

bahmutov
Copy link
Contributor

@bahmutov bahmutov commented May 3, 2021 •
edited by jennifer-shehane

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?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented May 3, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented May 3, 2021 •
edited


Test summary

4032 • 0 • 52 • 1 • Flakiness 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
@bahmutov
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 :(

@flotwig
Copy link
Contributor

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
flotwig
flotwig approved these changes May 4, 2021
View reviewed changes
Copy link
Contributor

@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

@bahmutov
Copy link
Contributor Author

bahmutov commented May 4, 2021

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

@jennifer-shehane
Copy link
Member

jennifer-shehane commented May 5, 2021 •
edited

@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
@tgriesser
Merge branch 'develop' into tgriesser/fix/1244-redirect-bug
09abfde 
* 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)
  ...
@cypress-bot
Copy link
Contributor

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.
Reviewers

@flotwig flotwig flotwig approved these changes

@chrisbreiding chrisbreiding Awaiting requested review from chrisbreiding

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bahmutov @flotwig @jennifer-shehane