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

chore(deps): downgrade electron to v12.0.0-beta.14 #16113

Merged
merged 1 commit into from
Apr 22, 2021
Merged

chore(deps): downgrade electron to v12.0.0-beta.14 #16113

merged 1 commit into from
Apr 22, 2021

Conversation

flotwig
Copy link
Contributor

@flotwig flotwig commented Apr 21, 2021 •
edited
Loading

Fixes #15853

User facing changelog

  • Fixed an issue causing tests to run slowly after upgrading to 7.0.0, especially when run with constrained CPU resources.
  • Downgradeded bundled Chrome version to 89.0.4328.0.

Additional details

Electron v12.0.0-beta.16 and above contain an unknown bug causing a major slowdown when video recording is enabled. For now maybe we can downgrade Electron to this last known good version, v12.0.0-beta.14

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?

@flotwig
chore(deps): downgrade electron to v12.0.0-beta.14
bef9974 
Fixes (avoids?) #15853

Electron v12.0.0-beta.16 and above contain an unknown bug causing a major slowdown when video recording is enabled. For now maybe we can downgrade Electron to this last known good version, v12.0.0-beta.14
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 21, 2021

Thanks for taking the time to open a PR!

@cypress
Copy link

cypress bot commented Apr 21, 2021


Test summary

9493 • 0 • 111 • 3 • Flakiness 0

Run details
Project cypress
Status Passed
Commit bef9974
Started Apr 21, 2021 3:30 PM
Ended Apr 21, 2021 3:41 PM
Duration 11:36 💡
OS Linux Debian - 10.8
Browser Multiple

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

@flotwig flotwig marked this pull request as ready for review April 21, 2021 16:29
@flotwig flotwig requested a review from a team as a code owner April 21, 2021 16:29
@flotwig flotwig requested review from chrisbreiding, kuceb and agg23 and removed request for a team, chrisbreiding and kuceb April 21, 2021 16:29
@agg23
Copy link
Contributor

agg23 commented Apr 21, 2021

Can you post time -p measurements for Chrome and Electron headless before and after this change?

@flotwig
Copy link
Contributor Author

flotwig commented Apr 21, 2021

Can you post time -p measurements for Chrome and Electron headless before and after this change?

The Electron version has no impact on the Chrome times. If you upgrade Chrome to a version 89.0.4389 or higher, you can reproduce this in Chrome, regardless of Electron version. I haven't been able to find the Chromium bug for this yet.

I ran actions.spec.js using --browser electron inside of Docker with --cpus=2. The Electron measurements were:

Electron Version Cypress Test Duration real user sys Good?
11.2.3 0:16 0:25 0:28 0:04 ✔️
12.0.0-beta.14 0:17 0:25 0:29 0:04 ✔️
12.0.0-beta.16 0:58 1:13 1:50 0:31 ❌
12.0.4 0:47 1:01 1:31 0:24 ❌

@agg23
Copy link
Contributor

agg23 commented Apr 21, 2021

The Electron version has no impact on the Chrome times. If you upgrade Chrome to a version 89.0.4389 or higher, you can reproduce this in Chrome, regardless of Electron version.

Did you experimentally confirm this? Intellectually I know that should be the case, but some of the results I was seeing was that Chrome results were changing as I changed Electron versions.

@flotwig
Copy link
Contributor Author

flotwig commented Apr 21, 2021

Did you experimentally confirm this? Intellectually I know that should be the case, but some of the results I was seeing was that Chrome results were changing as I changed Electron versions.

Yeah, here are my results running with Electron 12.0.4 but in --browser chrome:

Chrome Version Cypress Test Duration real user sys Good?
88.0.4324.182 0:33 1:02 1:38 0:11 ✔️
88.0.4324.182 0:37 1:16 2:07 0:12 ✔️
89.0.4389.72 1:09 1:58 3:21 0:27 ❌
89.0.4389.72 1:48 2:51 4:58 0:38 ❌
90.0.4430.72 2:00 3:28 6:08 0:42 ❌

@agg23
Copy link
Contributor

agg23 commented Apr 21, 2021

Do we know why Chromium is that much heavier than Electron? Over double the time taken, for a browser that should be pretty similar to what Electron exposes

@flotwig
Copy link
Contributor Author

flotwig commented Apr 21, 2021 •
edited
Loading

I'm not sure. Some of the difference is because Chrome has to be spawned as an independent process, I think it's faster for Electron to do new BrowserWindow than it is for us to do child_process.spawn('chrome')

Also, I think Electron's debugger module might be faster than using CDP over a websocket, like we have to do for Chrome.

@flotwig flotwig merged commit 7b55863 into develop Apr 22, 2021
@cypress-bot cypress-bot bot mentioned this pull request Apr 22, 2021
Closed
@flotwig flotwig mentioned this pull request Apr 22, 2021
Closed
@lmiller1990 lmiller1990 mentioned this pull request Apr 23, 2021
Closed
@flotwig flotwig mentioned this pull request Apr 23, 2021
Merged
5 tasks
tgriesser added a commit that referenced this pull request Apr 26, 2021
@tgriesser
Merge branch 'develop' into tgriesser/fix/3975-redirect-bug
88b2589 
* develop: (49 commits)
  fix: `cy.type()` should not change the value attr of button-like inputs. (#16154)
  fix: properly detect `cy.intercept(url, routeMatcher, handler)` overload (#16167)
  fix: consider multiple routes when looking for aliases (#16180)
  fix: pass contextIsolation: true (#16165)
  chore: fix failing "should handle aborted requests" test (#16170)
  feat(issue-3741): added keyboard support for folder (#15648)
  fix(webpack-dev-server): remove hard dependency on html-webpack-plugin v4  (#16108)
  chore(deps): downgrade electron to v12.0.0-beta.14 (#16113)
  fix: accept absolute paths in vite dev server (#16148)
  fix: cy.then shows wrong type when collection of HTMLElement's is provided (#15869)
  fix: do not treat utf8 requests as binary (#15946)
  chore: fix types
  docs: fix broken links for @cypress/react example recipes (#15674)
  update circle yml
  ignore undefined beforeEach
  fix: make vite-dev-server work on windows (#16103)
  chore: add triple slash reference
  chore: remove conflicting types
  chore: rebuild yarn lock
  resolve conflicts in master(fe0b63c) and develop
  ...
@effrenus
Copy link

effrenus commented May 24, 2021 •
edited
Loading

@flotwig Is it possible the problem because of this change: https://github.com/chromium/chromium/blob/d7da0240cae77824d1eda25745c4022757499131/content/browser/devtools/devtools_video_consumer.cc#L25?
Some details: https://www.notion.so/effrenus/Cypress-screencast-frame-rate-62dfe13d2efa46ebaf9a90957b3ee32a

//cc @jennifer-shehane, maybe related to #16498

@flotwig
Copy link
Contributor Author

flotwig commented May 24, 2021

@effrenus possibly, I have noticed that Electron 12.0.0-beta.16 sends roughly twice as many frames per minute as 12.0.0-beta.14. Good find.

@jennifer-shehane jennifer-shehane mentioned this pull request May 25, 2021
Closed
@jennifer-shehane jennifer-shehane mentioned this pull request Jun 16, 2021
Closed
@jennifer-shehane
Copy link
Member

This PR introduced a bug where the menu bar cannot be selected in Windows: #16323

@jennifer-shehane
Copy link
Member

This PR also introduced a performance issue with some cy.visits: #16671

@flotwig flotwig deleted the electron-v12.0.0-beta.14 branch January 24, 2022 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agg23 agg23 agg23 approved these changes

@chrisbreiding chrisbreiding chrisbreiding approved these changes

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

Successfully merging this pull request may close these issues.

[7.0.0] Performance regression from 6.8 (over 3x slower in our test suite)
5 participants
@flotwig @agg23 @effrenus @jennifer-shehane @chrisbreiding