fix: call screencastOpts as a function for starting a screencast on electron. #23001
Conversation
This file contains 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
|
Thanks for taking the time to open a PR!
|
marktnoonan
reviewed
Jul 28, 2022
Co-authored-by: Mark Noonan <mark@cypress.io>
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Flakiness
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 |
||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -784,6 +784,8 @@ const createRunAndRecordSpecs = (options = {}) => { | |||
| }) | |||
|
|
|||
| if (response === responseDidFail) { | |||
| debug('`responseDidFail` equals `response`, allowing browser to hang until it is killed: Response %o', { responseDidFail }) | |||
There was a problem hiding this comment.
why not just kill the process?
There was a problem hiding this comment.
This seems like the safest (easiest) option. I don't have context for why this was setup this way, but now we will have logs when/ if it happens.
emilyrohrbough
approved these changes
Jul 28, 2022
mschile
approved these changes
Jul 28, 2022
chrisbreiding
approved these changes
Jul 29, 2022
htekdev
pushed a commit
to htekdev/cypress
that referenced
this pull request
Aug 13, 2022
…lectron. (cypress-io#23001) Co-authored-by: Mark Noonan <mark@cypress.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
(Do we want to close this issue? I think there is more than one hang reported in there but this was the only reproducible example)
User facing changelog
Fixes a hang when sending video to the dashboard from the electron browser.
Additional details
This issue was introduced in version 8.6.0 with this change: #18392
screencastOptswas converted to a function, but not all instances were called as a function. In the electron browser we passed the function as the options object, which ended up causing a hang when running test.Based on my testing I'm confident in this fix, but I'm looking for suggestions about how to write a test for it.
I've also added an additional debug log to for future debugging to see if we allow the browser to hang when recording.
Steps to test
This has been tested via CI. A user was kind enough to help me get a re-creatable example setup for me to test against. Using that I was able to recreate the hang and discover the offending commit.
The hang was introduced in build 107 (which uses cypress built from this commit: b4b3d67 ) and corrected in build 109 (which uses cypress build from this commit: 3f7fc20). 108 was additional logging.
https://app.circleci.com/pipelines/github/mjhenkes/jcontent?branch=master
How has the user experience changed?
This specific hang will no longer occur in CI.
PR Tasks
cypress-documentation?type definitions?