fix: electron slowness within cy origin #21445

AtofStryker · 2022-05-11T23:38:25Z

Closes Investigate cy.origin being significantly slow in 10.x than in 9.x in electron 94 #21375

User facing changelog

Additional details

After merging cy.origin into the 10.0-release branch, it was noticed that certain cy.origin tests were performing incredibly slow in electron. It turns out the root cause of this was the spec bridge iframe being beneath the Spec Runner / Reporter / AUT, which for some reason was causing electron to slow down drastically. In 9.x, the spec bridge overlays the Reporter / AUT due to how the UI layout is structured. That structured changed in 10.x, which made this problem visible. We were able to reproduce this issue in 9.x by attaching the spec bridge after the body element, instead of within it. To fix this in 10.x, we need to set the spec bridge to be within the viewport by set top and left to 0. This is verified by a new system test added to make sure the spec bridge isn't shifted south of the AUT.

CURRENT in `9.x` Spec Bridge overlaying AUT (no performance issue visible)

`9.x` Spec Bridge beneath AUT (performance issues present)

`10.x` THIS PR Spec Bridge overlaying AUT (no performance issue visible)

CURRENT `10.x` Spec Bridge beneath AUT (performance issues present)

How has the user experience changed?

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)
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 · 2022-05-11T23:38:28Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

AtofStryker · 2022-05-11T23:49:08Z

packages/runner/src/iframe/iframe.scss

@@ -94,12 +94,4 @@
    transform-origin: 50% 0;
    width: 100%;
  }
-}
-
-.spec-bridge-iframe {


these styles were still being injected, but it doesn't really make sense for the style to live here any longer, but inside the app, package.

lmiller1990 · 2022-05-12T01:41:20Z

Not sure how you figured out the styling was the problem - why does the style of the spec bridge so heavily impact performance? Any theory?

system-tests/projects/e2e/cypress/e2e/spec_bridge.cy.ts

chrisbreiding · 2022-05-12T12:59:47Z

system-tests/projects/e2e/cypress/e2e/spec_bridge.cy.ts

@@ -0,0 +1,20 @@
+it('visits foobar.com and types foobar inside an input', () => {


Any reason this can't be a driver test instead of system test?

Not in particular. Main reason I didn't add it to the driver tests is the main test is really to test additional styles to the app package and behavior. I originally started with a cy-in-cy test inside the app package, but had issues configuring the host, plus it only ran in chrome and regressions would likely be a lot more obvious in electron. So I just moved it to a system test 🤷 .

I'd prefer that this and the other ones be driver tests.

In general, it's better to use regular Cypress tests (driver tests, etc) if possible and reserve system tests for when it's not possible to write regular Cypress tests, like when the tests need specific project configuration, need dependencies the driver does not have, or when testing certain failures. It's much easier to iterate on driver tests as well as debug any failures, plus the results go to the Dashboard and utilize retries and config we already have set up for them.

It's alright if the tests cover more than driver behavior. A lot of the driver tests are true e2e tests. Since this issue is specific to cy.origin(), I'd say the driver is as appropriate a place as any for the tests.

chrisbreiding · 2022-05-12T13:18:09Z

system-tests/projects/e2e/cypress/e2e/spec_bridge.cy.ts

+    expect(specBridgeIframe.offsetTop).to.equal(currentBody.offsetTop)
+    expect(specBridgeIframe.offsetLeft).to.equal(currentBody.offsetLeft)
+    expect(specBridgeIframe.clientWidth).to.equal(currentBody.clientWidth)
+    expect(specBridgeIframe.clientHeight).to.equal(currentBody.clientHeight)


I know this PR is fixing a performance issue, but I believe the original reason the spec bridge needed to have the same dimensions as the AUT is for the sake of screenshots having the correct dimensions.

I wonder if there was a behavioral regression as well that this fixes, but it doesn't look like we have test coverage around screenshot dimensions for screenshots taken within cy.origin(), so it didn't result in a failing test. If so, I think asserting on the screenshot behavior would be a good idea. It would also be better since it would assert on the behavior instead of the implementation details.

Agreed. I am going to look into seeing if we can add in a few screenshot tests that assert on the behavior of the screensize within cy.origin for a few different use cases.

It's unlikely there was a behavioral regression since the size of the frame was still correct.

I don't think a few system tests could hurt to verify the screenshot in case there are some weird layout issues that occur in future.

@chrisbreiding how is something like 70e3251? It's based on the system tests that currently exist for snapshotting to just verify the dimensions of clipped, element, and full page snapshots.

Looks good! Those are exactly what I had in mind.

Like I said in my other comment, I think they should be moved into the driver's tests. They would fit in well in origin/commands/screenshot.spec.ts. You'll just need to copy over the check:screenshot:size task or share it some manner.

Looks good! Those are exactly what I had in mind.

Like I said in my other comment, I think they should be moved into the driver's tests. They would fit in well in origin/commands/screenshot.spec.ts. You'll just need to copy over the check:screenshot:size task or share it some manner.

@chrisbreiding I got everything moved over into the driver and now looks to be working properly.

AtofStryker · 2022-05-12T14:28:53Z

Not sure how you figured out the styling was the problem - why does the style of the spec bridge so heavily impact performance? Any theory?

@lmiller1990 I got lucky. @chrisbreiding brought up yesterday that maybe the performance was related to the iframe size. When I set the spec bridge iframe to have a width and height of 0, cy.origin starting performing incredibly quick. But the spec bridge iframe in 9.x has a full height/width of the application body in order to take cy.screenshot correctly in cy.origin. With that info, when comparing the two between 9.x and 10.x, I noticed the positioning was off in 10.x, and fixing the positioning fixed the performance issue.

I am unsure why this is happening. My guess is electron trying to render contents outside the viewport is causing a massive slowdown, but it seems weird we wouldn't see this behavior in other chromium browsers. I think this might be worth opening an issue to electron itself.

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

cypress · 2022-05-12T21:45:35Z

Test summary

37424 • 0 • 454 • 0 • 9

Run details


Project	cypress
Status	Passed
Commit	`eb64e96`
Started	May 17, 2022 3:28 PM
Ended	May 17, 2022 3:47 PM
Duration	19:43 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	commands/actions/click.cy.js	1
1	... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
	commands/net_stubbing.cy.ts	2
1	... > doesn't fail test if network error occurs retrieving response and response is not intercepted
2	network stubbing > intercepting request > can delay and throttle a StaticResponse
	commands/navigation.cy.js	1
1	src/cy/commands/navigation > #visit > window immediately resolves and doesn't reload when visiting the same URL with hashes
	next.cy.ts	1
1	Working with next-12 > should detect new spec
This comment includes only the first 5 flaky tests. See all 9 flaky tests in the 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

…ots within cy.origin

…ypress-io/cypress into bugfix/fix-electron-slowness-cy-origin

lmiller1990 · 2022-05-13T02:23:09Z

Not sure how you figured out the styling was the problem - why does the style of the spec bridge so heavily impact performance? Any theory?

@lmiller1990 I got lucky. @chrisbreiding brought up yesterday that maybe the performance was related to the iframe size. When I set the spec bridge iframe to have a width and height of 0, cy.origin starting performing incredibly quick. But the spec bridge iframe in 9.x has a full height/width of the application body in order to take cy.screenshot correctly in cy.origin. With that info, when comparing the two between 9.x and 10.x, I noticed the positioning was off in 10.x, and fixing the positioning fixed the performance issue.

I am unsure why this is happening. My guess is electron trying to render contents outside the viewport is causing a massive slowdown, but it seems weird we wouldn't see this behavior in other chromium browsers. I think this might be worth opening an issue to electron itself.

Interesting... there are definitely a number of differences in electron and chrome, even if they are using the same internals, more or less, specifically with frames. Here's another one: #15932

lmiller1990

Good to see some regression tests added. Thanks for this. I wonder if we need a more aggressive comment explaining the CSS you added is perf-critical - right now it reads like "this is some style" not "this is critical for perf, don't mess with it".

AtofStryker · 2022-05-13T03:40:28Z

Good to see some regression tests added. Thanks for this. I wonder if we need a more aggressive comment explaining the CSS you added is perf-critical - right now it reads like "this is some style" not "this is critical for perf, don't mess with it".

@lmiller1990 I'll add a bit more explanation in the main.scss to explain the styles and why they are needed, and to not change them unless the impacts are known.

…ypress-io/cypress into bugfix/fix-electron-slowness-cy-origin

…fix/fix-electron-slowness-cy-origin

…issing in CI

…ypress-io/cypress into bugfix/fix-electron-slowness-cy-origin

… longer need it

AtofStryker · 2022-05-16T18:02:00Z

Good to see some regression tests added. Thanks for this. I wonder if we need a more aggressive comment explaining the CSS you added is perf-critical - right now it reads like "this is some style" not "this is critical for perf, don't mess with it".

@lmiller1990 I added some more comments in e26ae8b

…en in cypress cypress-io/cypress#21445

lmiller1990 · 2022-05-17T02:37:30Z

Looks good to me.

AtofStryker added 3 commits May 11, 2022 19:19

fix iframe performance issues in electron

06fe5b1

reduce 1000ms to 300ms for cross:origin:release:html

7e31364

reduce shouldWithTimeout from 3000ms to 250ms

ec2ea63

AtofStryker changed the title ~~Bugfix/fix electron slowness cy origin~~ Bugfix: fix electron slowness cy origin May 11, 2022

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

05a9a99

AtofStryker changed the title ~~Bugfix: fix electron slowness cy origin~~ fix: electron slowness within cy origin May 11, 2022

AtofStryker requested review from mschile, emilyrohrbough, chrisbreiding and mjhenkes May 11, 2022 23:44

AtofStryker marked this pull request as ready for review May 11, 2022 23:44

AtofStryker requested review from a team as code owners May 11, 2022 23:44

AtofStryker removed the request for review from a team May 11, 2022 23:44

AtofStryker commented May 11, 2022

View reviewed changes

chrisbreiding requested changes May 12, 2022

View reviewed changes

AtofStryker and others added 4 commits May 12, 2022 10:29

Update system-tests/projects/e2e/cypress/e2e/spec_bridge.cy.ts

cf12f0f

Co-authored-by: Chris Breiding <chrisbreiding@users.noreply.github.com>

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

be1fc4f

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

90bd99f

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

017ebff

AtofStryker added 2 commits May 12, 2022 18:08

add some regression tests around screenshot size when taking screensh…

70e3251

…ots within cy.origin

Merge branch 'bugfix/fix-electron-slowness-cy-origin' of github.com:c…

cf856bf

…ypress-io/cypress into bugfix/fix-electron-slowness-cy-origin

AtofStryker requested a review from chrisbreiding May 12, 2022 22:10

lmiller1990 approved these changes May 13, 2022

View reviewed changes

AtofStryker added 8 commits May 13, 2022 13:23

Merge branch 'bugfix/fix-electron-slowness-cy-origin' of github.com:c…

80837d9

…ypress-io/cypress into bugfix/fix-electron-slowness-cy-origin

Merge branch '10.0-release' of github.com:cypress-io/cypress into bug…

eb89971

…fix/fix-electron-slowness-cy-origin

add additional comments to spec bridge iframe scss for caution

e26ae8b

resolve snapshots directory correctly in open vs run mode

7cf3afe

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

9f318ab

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

7690f0e

revert this commit to see if assets are being tampered with or just m…

2c98438

…issing in CI

Merge branch 'bugfix/fix-electron-slowness-cy-origin' of github.com:c…

dbbf06a

…ypress-io/cypress into bugfix/fix-electron-slowness-cy-origin

AtofStryker force-pushed the bugfix/fix-electron-slowness-cy-origin branch 3 times, most recently from 5d17f52 to bffe6e7 Compare May 13, 2022 22:29

fix check:screenshot:size to always point to correct screenshot path

0fe394f

AtofStryker force-pushed the bugfix/fix-electron-slowness-cy-origin branch from bffe6e7 to 0fe394f Compare May 16, 2022 16:59

AtofStryker added 2 commits May 16, 2022 13:02

remove isCypressRunMode from cypress config as internal flag as we no…

739897f

… longer need it

add check:screenshot:size to task.cy.js registered plugin assertion

1a89e94

AtofStryker requested review from mjhenkes and chrisbreiding May 16, 2022 17:59

AtofStryker added 2 commits May 16, 2022 14:38

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

5707d36

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

b89e336

AtofStryker added a commit to AtofStryker/electron-iframe-performance-reprod that referenced this pull request May 16, 2022

an attempt to reproduce issues with iframes out of the viewport as se…

7af0057

…en in cypress cypress-io/cypress#21445

chrisbreiding approved these changes May 17, 2022

View reviewed changes

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

4f4a074

mjhenkes approved these changes May 17, 2022

View reviewed changes

AtofStryker added 2 commits May 17, 2022 10:41

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

fcc910c

Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin

eb64e96

AtofStryker merged commit 045ad04 into 10.0-release May 17, 2022

AtofStryker deleted the bugfix/fix-electron-slowness-cy-origin branch May 17, 2022 16:03

cypress-bot bot mentioned this pull request May 17, 2022

Investigate cy.origin being significantly slow in 10.x than in 9.x in electron 94 #21375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: electron slowness within cy origin #21445

fix: electron slowness within cy origin #21445

AtofStryker commented May 11, 2022 •

edited

Loading

cypress-bot bot commented May 11, 2022

AtofStryker May 11, 2022

lmiller1990 commented May 12, 2022 •

edited

Loading

chrisbreiding May 12, 2022

AtofStryker May 12, 2022

chrisbreiding May 13, 2022

chrisbreiding May 12, 2022

AtofStryker May 12, 2022

mschile May 12, 2022

AtofStryker May 12, 2022

AtofStryker May 12, 2022

chrisbreiding May 13, 2022

AtofStryker May 16, 2022

AtofStryker commented May 12, 2022

cypress bot commented May 12, 2022 •

edited

Loading

lmiller1990 commented May 13, 2022

lmiller1990 left a comment

AtofStryker commented May 13, 2022

AtofStryker commented May 16, 2022 •

edited

Loading

lmiller1990 commented May 17, 2022

		@@ -0,0 +1,20 @@
		it('visits foobar.com and types foobar inside an input', () => {

fix: electron slowness within cy origin #21445

fix: electron slowness within cy origin #21445

Conversation

AtofStryker commented May 11, 2022 • edited Loading

User facing changelog

Additional details

CURRENT in 9.x Spec Bridge overlaying AUT (no performance issue visible)

9.x Spec Bridge beneath AUT (performance issues present)

10.x THIS PR Spec Bridge overlaying AUT (no performance issue visible)

CURRENT 10.x Spec Bridge beneath AUT (performance issues present)

How has the user experience changed?

PR Tasks

cypress-bot bot commented May 11, 2022

Choose a reason for hiding this comment

lmiller1990 commented May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtofStryker commented May 12, 2022

cypress bot commented May 12, 2022 • edited Loading

Test summary

Run details

Flakiness

lmiller1990 commented May 13, 2022

lmiller1990 left a comment

Choose a reason for hiding this comment

AtofStryker commented May 13, 2022

AtofStryker commented May 16, 2022 • edited Loading

lmiller1990 commented May 17, 2022

AtofStryker commented May 11, 2022 •

edited

Loading

CURRENT in `9.x` Spec Bridge overlaying AUT (no performance issue visible)

`9.x` Spec Bridge beneath AUT (performance issues present)

`10.x` THIS PR Spec Bridge overlaying AUT (no performance issue visible)

CURRENT `10.x` Spec Bridge beneath AUT (performance issues present)

lmiller1990 commented May 12, 2022 •

edited

Loading

cypress bot commented May 12, 2022 •

edited

Loading

AtofStryker commented May 16, 2022 •

edited

Loading