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: Plugin error when sending on disconnected IPC channel #21011

Merged
merged 3 commits into from Apr 13, 2022
Merged

fix: Plugin error when sending on disconnected IPC channel #21011

merged 3 commits into from Apr 13, 2022

Conversation

tomudding
Copy link
Contributor

@tomudding tomudding commented Apr 9, 2022 •
edited by emilyrohrbough

User facing changelog

Fixed an issue where an ambiguous error message was incorrectly shown to the user when Cypress was interrupted during an active test. Fixed #21010.

Additional details

Interrupting Cypress during an active test would cause the file:preprocessor child process' IPC channel to already be disconnected while handling preprocessor:close. This would lead to an error as it is no longer possibly to send message. That could cause confusion due to the ambiguous error message produced.

This fix check that the process is still connected before sending messages on the IPC channel.

How has the user experience changed?

No more ambiguous error when interrupting the Cypress process.

PR Tasks

  • Have tests been added/updated?
  • 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?

@tomudding tomudding requested a review from a team as a code owner April 9, 2022 12:40
@tomudding tomudding requested review from jennifer-shehane and removed request for a team April 9, 2022 12:40
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 9, 2022

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@tomudding
Copy link
Contributor Author

I have added a test case for the new behaviour (similar to the test for killed processes). However, lib/plugins/util #wrapIpc #send sends event through the process still fails. I assume this is because the connected property is not defined in the theProcess object, but I am not sure how it would have worked before then as killed is also not defined.

Let me know what you think.

@jennifer-shehane jennifer-shehane removed their request for review April 11, 2022 14:35
jennifer-shehane
jennifer-shehane previously requested changes Apr 11, 2022
View reviewed changes
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

@tomudding There's a failing test in the test file you touched, could you look at the failure to see if this is related? Thanks.

@tomudding
Copy link
Contributor Author

@tomudding There's a failing test in the test file you touched, could you look at the failure to see if this is related? Thanks.

Yes I am aware, the reason is that the connected is undefined in the test theProcess object. Hence it fails for the generic test (as !undefined == true). Should I just add this property to the theProcess object in beforeEach?

@rockhold rockhold requested a review from BlueWinds April 11, 2022 15:37
@jennifer-shehane jennifer-shehane dismissed their stale review April 11, 2022 15:42

Dismissing my previous review so that other reviewers can look at this.

@BlueWinds
Copy link
Contributor

@tomudding There's a failing test in the test file you touched, could you look at the failure to see if this is related? Thanks.

Yes I am aware, the reason is that the connected is undefined in the test theProcess object. Hence it fails for the generic test (as !undefined == true). Should I just add this property to the theProcess object in beforeEach?

Yeah, adding it to the mocked process object seems correct. Looks good to me, just rerunning the flaky test so we can get everything green.

@BlueWinds
Copy link
Contributor

These bogus error messages when cancelling tests have bothered me, but never enough to take the time to fix it - thanks for taking the time!

@jennifer-shehane jennifer-shehane merged commit 57dbc17 into cypress-io:develop Apr 13, 2022
@cypress-bot cypress-bot bot mentioned this pull request Apr 13, 2022
Closed
tgriesser added a commit that referenced this pull request Apr 20, 2022
@tgriesser
Merge branch '10.0-release' into tgriesser/chore/UNIFY-1256-auto-merg…
e1a4b3d 
…e-config

* 10.0-release:
  fix: make error on integration folder point to e2e (#20853)
  fix(unify): Update Cypress Dashboard Service Link in Login Modal (#21084)
  chore: fix windows node_modules install step (#21098)
  fix: webpack integration tests for app w webpack-dev-server-fresh (#21093)
  fix: move non spec files on migration (#21054)
  Bumping electron version in root
  chore(deps): Bumping electron dependency to 15.5.1 (#21072)
  fix: resolves correctly specPattern (#21057)
  feat: replace reconfigure button on settings page with link to config doc (#21077)
  feat(launchpad): update CT setup and config scaffolding (#20893)
  fix: cy.type('{enter}') on <input> elements submits the form correctly after Firefox 98. (#21042)
  chore: making the npm deps for vue, react, and vue2 use 0.0.0-dev (#21081)
  fix(cli): show additional mitigation steps for max path length error (#21047)
  fix: Plugin error when sending on disconnected IPC channel (#21011)
  chore: add internal types for cy.session command (#21028)
  chore: Update Chrome (stable) to 100.0.4896.88 (#21043)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BlueWinds BlueWinds BlueWinds approved these changes

@emilyrohrbough emilyrohrbough emilyrohrbough approved these changes

@jennifer-shehane jennifer-shehane jennifer-shehane left review comments

Assignees

@tomudding tomudding

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@tomudding @CLAassistant @BlueWinds @jennifer-shehane @emilyrohrbough