-
Notifications
You must be signed in to change notification settings - Fork 2
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
Cypress uses Chrome Devtools Protocol #53
Conversation
@@ -38,7 +38,7 @@ | |||
"test:unit": "jest", | |||
"test:playwright": "playwright test", | |||
"test:cypress": "start-server-and-test test:server 3000 test:do-cypress", | |||
"test:do-cypress": "cypress run --project tests", | |||
"test:do-cypress": "ELECTRON_EXTRA_LAUNCH_ARGS=--remote-debugging-port=8192 cypress run --project tests", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pass the ELECTRON_EXTRA_LAUNCH_ARGS
stuff so that Cypress will know what port the devtools protocol is on. Electron for some reason doesn't pass this info along (and Electron is the default browser for Cypress runs). I'll make sure this gets added to our docs as well for users.
@@ -64,7 +70,7 @@ export class Watcher { | |||
await this.client.send('Fetch.enable'); | |||
} | |||
|
|||
async idle(page: Page) { | |||
async idle(page?: Page) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, the network-idle listening doesn't happen for Cypress. This will be resolved when I add that in a future PR
tests/cypress.config.ts
Outdated
on('task', { | ||
archiveCypress, | ||
setupNetworkListener, | ||
saveArchives, | ||
}); | ||
on('before:browser:launch', async (browser, launchOptions) => { | ||
await onBeforeBrowserLaunch(browser, launchOptions); | ||
|
||
return launchOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the stuff the user will have to add to their cypress.config
file.
@@ -102,6 +103,7 @@ | |||
}, | |||
"dependencies": { | |||
"@segment/analytics-node": "^1.1.0", | |||
"chrome-remote-interface": "^0.33.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This enables us to listen to CDP. Big thanks to cypress-har-generator
plugin, which uses this lib and was a good pattern to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good! I guess we are still conversing about ways we might improve the API.
await doArchive(params); | ||
// using a single Watcher instance across all tests (for the test run) | ||
// each time a test completes, we'll save to disk whatever archives are there at that point. | ||
// This should be safe since the same resource from the same URL should be the same during the entire test run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe with the resource remapping stuff @jwir3 did?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This should be safe since the same resource from the same URL should be the same during the entire test run.
As long as this invariant holds, then it should be fine with the resource mapping stuff. If the content of the url changes, though (the response), then it could be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwir3 can you elaborate on how the resource mapping stuff would make this a problem?
I'd be surprised if the resource (at the same URL) changed during the test run, but wanted to make sure I'm not missing something here.
// We use this lifecycle hook because we need to know what host and port Chrome Devtools Protocol is listening at. | ||
export const onBeforeBrowserLaunch = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider making this part unnecessary if you are running in electron and passing in the env var?
I guess that'd complicate the DX and not save people much effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmeasday good point. We could remove the need for this lifecycle event handler if we expected everyone to always pass their port as an environment variable (I suppose we could require hostname as well, but I'd assume it's always 127.0.0.1
).
Right now people only need to pass it if they're using Electron. But that is the default, and having a bunch of "in-this-case-you-need-to-add-this" messaging would not be good DX. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, perhaps a question for DX / @byebrianwong
await doArchive(params); | ||
// using a single Watcher instance across all tests (for the test run) | ||
// each time a test completes, we'll save to disk whatever archives are there at that point. | ||
// This should be safe since the same resource from the same URL should be the same during the entire test run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This should be safe since the same resource from the same URL should be the same during the entire test run.
As long as this invariant holds, then it should be fine with the resource mapping stuff. If the content of the url changes, though (the response), then it could be problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about this comment in the PR message:
For now, Cypress doesn't listen for network idleness to wrap up the archive. It just waits for the full test timeout amount (10s)
Does that mean each test will take the full 10 seconds to run?
@tevanoff yes, it does. I'm working on a follow-on PR to use network idleness. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Are you planning on holding off on merging this until the network idle PR is ready to go as well?
@tevanoff thanks!
I'm not planning on waiting to merge for the idle stuff -- since we haven't onboarded people onto Cypress yet, I figure we're free to make changes at will still. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let's look at the env var stuff in a followup.
🚀 PR was released in |
What Changed
Cypress now uses Chrome Devtools Protocol (CDP) to archive the requested resources! This enables us to share most of the network-watching code (95%) between Playwright and Cypress.
Previously we were using the client-side
cy.intercept()
, which plays well with Cypress but would have required us to duplicate our network-watching code (and we'd have to remember to keep bugfixes and features up-to-date in both places).The approach
Like Playwright, we use the Watcher class to listen to network requests/responses via CDP, and store those on the
watcher.archive
object. However, we use one watcher instance for the entire test run, instead of one instance per test (we still write the archives to disk at the end of each test). This is because Cypress doesn't give us a way to share the watcher between the "beforeEach" and "afterEach" on the server.We still take the DOM snapshot on the client, and pass that to the server (where we add the watcher archive and send things off to be written to disk).
The API
Users will now need to add a second command to their
cypress.config.js|ts
file, as well as theon('before:browser:launch')
lifecycle method. In practice, that will look like:This is in addition to the user importing our client-side before/after hooks in their support.ts file (which they had to do before as well).
Tradeoffs/limitations
ELECTRON_EXTRA_LAUNCH_ARGS=--remote-debugging-port=<port-number> cypress run
instead of justcypress run
. I'll make sure this gets added to the docs. This limitation doesn't exist if we use the client-sideCy.intercept()
to archive. Not ideal, but other network-watching Cypress plugins have the same limitation.How to test
yarn test:playwright
locally and confirm that the tests aren't waiting for the global 10s timeout to finish (making sure they're still using the network-idle-checking)cy.intercept()
for network-watching.Change Type
maintenance
documentation
patch
minor
major
📦 Published PR as canary version:
0.0.54--canary.53.8d7bcec.0
✨ Test out this PR locally via:
npm install @chromaui/test-archiver@0.0.54--canary.53.8d7bcec.0 # or yarn add @chromaui/test-archiver@0.0.54--canary.53.8d7bcec.0