fix: preserve orphaned process electron#29515
Merged
Merged
Conversation
da1febb to
a900563
Compare
11 flaky tests on run #55401 ↗︎
Details:
|
|||||||||||||||||||||||||||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| ... > dismisses the banner for a specified time |
Test Replay
Screenshots
|
|
e2e/origin/navigation.cy.ts • 1 flaky test • 5x-driver-firefox
| Test | Artifacts | |||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| ... > establishes foobar spec bridge |
| |||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| ... > throws when foo cannot resolve |
Test Replay
|
|
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-electron
| Test | Artifacts | |
|---|---|---|
| ... > stops waiting when an fetch request is canceled |
Test Replay
|
|
commands/querying/querying.cy.js • 1 flaky test • 5x-driver-electron
| Test | Artifacts | |
|---|---|---|
| ... > throws when alias property isnt just a digit |
Test Replay
|
|
The first 5 flaky specs are shown, see all 9 specs in Cypress Cloud.
Review all test suite changes for PR #29515 ↗︎
a900563 to
6bdf6ed
Compare
cacieprins
approved these changes
May 17, 2024
|
|
||
| if (isOrphanedBrowserProcess) { | ||
| debug(`killing process because launch attempt: ${thisLaunchAttempt} does not match current launch attempt: ${launchAttempt}`) | ||
| await kill({ instance: _instance, isOrphanedBrowserProcess, nullOut: false }) |
Collaborator
There was a problem hiding this comment.
Is nullOut (or rather !nullOut) serving the same purpose as isOrphanedBrowserProcess?
Collaborator
Author
There was a problem hiding this comment.
I don't think so. I think the goal of nullOut is to set the current browser instance to null, which we don't want to do since we want the active instance to be set, but the _instance inside the closure to be cleaned up.
ryanthemanuel
approved these changes
May 17, 2024
Contributor
|
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Additional details
This PR fixes an issue where orphaned processes were killing the whole electron browser CRI client, when in actually we want to preserve this until the electron process is terminated.
Unique to our electron setup, launching electron produces a window and not a separate browser instance. Since orphaned processes (or in this case, windows/targets) need to be cleaned up, we want to only clean up those items related to the process/window and not the overall launched instance.
see #28397 (comment) for more in depth details, as well as how we reproduced the issue
Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?