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.

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

fix: electron slowness within cy origin #21445

Merged
merged 29 commits into from
May 17, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
06fe5b1
fix iframe performance issues in electron
AtofStryker May 11, 2022
7e31364
reduce 1000ms to 300ms for cross:origin:release:html
AtofStryker May 11, 2022
ec2ea63
reduce shouldWithTimeout from 3000ms to 250ms
AtofStryker May 11, 2022
05a9a99
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 11, 2022
cf12f0f
Update system-tests/projects/e2e/cypress/e2e/spec_bridge.cy.ts
AtofStryker May 12, 2022
be1fc4f
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 12, 2022
90bd99f
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 12, 2022
017ebff
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 12, 2022
70e3251
add some regression tests around screenshot size when taking screensh…
AtofStryker May 12, 2022
cf856bf
Merge branch 'bugfix/fix-electron-slowness-cy-origin' of github.com:c…
AtofStryker May 12, 2022
f784d00
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 13, 2022
0ff55b1
move screenshot e2e tests from system-tests into the driver
AtofStryker May 13, 2022
83b5746
move spec bridge system test into driver test
AtofStryker May 13, 2022
80837d9
Merge branch 'bugfix/fix-electron-slowness-cy-origin' of github.com:c…
AtofStryker May 13, 2022
eb89971
Merge branch '10.0-release' of github.com:cypress-io/cypress into bug…
AtofStryker May 13, 2022
e26ae8b
add additional comments to spec bridge iframe scss for caution
AtofStryker May 13, 2022
7cf3afe
resolve snapshots directory correctly in open vs run mode
AtofStryker May 13, 2022
9f318ab
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 13, 2022
7690f0e
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 13, 2022
2c98438
revert this commit to see if assets are being tampered with or just m…
AtofStryker May 13, 2022
dbbf06a
Merge branch 'bugfix/fix-electron-slowness-cy-origin' of github.com:c…
AtofStryker May 13, 2022
0fe394f
fix check:screenshot:size to always point to correct screenshot path
AtofStryker May 16, 2022
739897f
remove isCypressRunMode from cypress config as internal flag as we no…
AtofStryker May 16, 2022
1a89e94
add check:screenshot:size to task.cy.js registered plugin assertion
AtofStryker May 16, 2022
5707d36
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 16, 2022
b89e336
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 16, 2022
4f4a074
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 17, 2022
fcc910c
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 17, 2022
eb64e96
Merge branch '10.0-release' into bugfix/fix-electron-slowness-cy-origin
AtofStryker May 17, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/app/src/main.scss
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,14 @@ html {
height: 0;
}
}

// used to style the spec bridge needed for cy.origin.
iframe.spec-bridge-iframe {
border: none;
height: 100%;
position: fixed;
visibility: hidden;
width: 100%;
top: 0;
left: 0;
}
5 changes: 1 addition & 4 deletions packages/driver/cypress/support/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,7 @@ const getAllFn = (...aliases) => {
)
}

// FIXME: currently increasing the timeout here from 250ms to 3 second
// due to some unknown performance issues with logs coming back in from the primary in the 10.0-release branch.
// this timeout should be reduced in the future once these performance issues are addressed.
const shouldWithTimeout = (cb, timeout = 3000) => {
const shouldWithTimeout = (cb, timeout = 250) => {
Copy link
Member

@mjhenkes mjhenkes May 13, 2022

Choose a reason for hiding this comment

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

🎉

cy.wrap({}, { timeout }).should(cb)
}

Expand Down
2 changes: 1 addition & 1 deletion packages/driver/src/cy/commands/origin/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export default (Commands, Cypress: Cypress.Cypress, cy: Cypress.cy, state: Cypre
// not have a cy.origin for the intermediary origin.
timeoutId = setTimeout(() => {
Cypress.backend('cross:origin:release:html')
}, 1000)
}, 300)
})

Commands.addAll({
Expand Down
10 changes: 1 addition & 9 deletions packages/runner/src/iframe/iframe.scss
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,4 @@
transform-origin: 50% 0;
width: 100%;
}
}

.spec-bridge-iframe {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

border: none;
height: 100%;
position: fixed;
visibility: hidden;
width: 100%;
}
}
59 changes: 59 additions & 0 deletions system-tests/__snapshots__/spec_bridge_in_viewport_spec.ts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
exports['e2e spec bridge in viewport / Overlays the reporter/AUT and is not positioned off screen, leading to potential performance impacts with cy.origin'] = `

====================================================================================================

(Run Starting)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Cypress: 1.2.3 │
│ Browser: FooBrowser 88 │
│ Specs: 1 found (spec_bridge.cy.ts) │
│ Searched: cypress/e2e/spec_bridge.cy.ts │
│ Experiments: experimentalSessionAndOrigin=true │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


────────────────────────────────────────────────────────────────────────────────────────────────────

Running: spec_bridge.cy.ts (1 of 1)


✓ visits foobar.com and types foobar inside an input

1 passing


(Results)

┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 1 │
│ Passing: 1 │
│ Failing: 0 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: true │
│ Duration: X seconds │
│ Spec Ran: spec_bridge.cy.ts │
└────────────────────────────────────────────────────────────────────────────────────────────────┘


(Video)

- Started processing: Compressing to 32 CRF
- Finished processing: /XXX/XXX/XXX/cypress/videos/spec_bridge.cy.ts.mp4 (X second)


====================================================================================================

(Run Finished)


Spec Tests Passing Failing Pending Skipped
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✔ spec_bridge.cy.ts XX:XX 1 1 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✔ All specs passed! XX:XX 1 1 - - -


`
20 changes: 20 additions & 0 deletions system-tests/projects/e2e/cypress/e2e/spec_bridge.cy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
it('visits foobar.com and types foobar inside an input', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🤷 .

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

cy.visit('primary_origin.html')
cy.get('[data-cy="cross_origin_secondary_link"]').click()

cy.origin('http://foobar.com:4466', () => {
cy.get('[data-cy="text-input"]').type('foobar')
})

// ensure stability
cy.then(() => {
AtofStryker marked this conversation as resolved.
Show resolved Hide resolved
const specBridgeIframe: HTMLIFrameElement = window.top.document.querySelector('.spec-bridge-iframe')
const currentBody = window.top.document.querySelector('body')

// make sure the spec bridge overlays the reporter/AUT and is not off the screen
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

})
})
1 change: 1 addition & 0 deletions system-tests/projects/e2e/secondary_origin.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
<p data-cy="dom-check">From a secondary origin</p>
<p data-cy="cypress-check"></p>
<p data-cy="window-before-load"></p>
<input data-cy="text-input" type="text"/>
<form>
<button type="submit">Submit</button>
</form>
Expand Down
38 changes: 38 additions & 0 deletions system-tests/test/spec_bridge_in_viewport_spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import path from 'path'
import systemTests from '../lib/system-tests'
import Fixtures from '../lib/fixtures'

const e2ePath = Fixtures.projectPath('e2e')

const PORT = 3500
const onServer = function (app) {
app.get('/secondary_origin.html', (_, res) => {
res.sendFile(path.join(e2ePath, `secondary_origin.html`))
})
}

describe('e2e spec bridge in viewport', () => {
systemTests.setup({
servers: [{
port: 4466,
onServer,
}],
settings: {
hosts: {
'*.foobar.com': '127.0.0.1',
},
},
})

systemTests.it('Overlays the reporter/AUT and is not positioned off screen, leading to potential performance impacts with cy.origin', {
// keep the port the same to prevent issues with the snapshot
port: PORT,
spec: 'spec_bridge.cy.ts',
snapshot: true,
browser: 'electron',
expectedExitCode: 0,
config: {
experimentalSessionAndOrigin: true,
},
})
})