refactor: move more of video capture into browser automations #23587
Conversation
|
Thanks for taking the time to open a PR!
|
|
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 |
||||||||||||||||||||||||||||||||||
| async startVideoRecording (writeVideoFrame: WriteVideoFrame, screencastOpts?) { | ||
| this.onFn('Page.screencastFrame', async (e) => { | ||
| writeVideoFrame(Buffer.from(e.data, 'base64')) | ||
| await this.sendDebuggerCommandFn('Page.screencastFrameAck', { sessionId: e.sessionId }) |
There was a problem hiding this comment.
async function() { await Promise() } is mostly equivalent to function() { return Promise() }. You only need to await things if you want flow control to return to this function after a promise completes (eg, to return a different value than the inner promise resolves to).
Same with the outer startVideoRecording as well.
There was a problem hiding this comment.
I see what you mean, but I prefer it this way, since I don't want to "return" any value. A synchronous function wouldn't return on this line either. Absent of a coding style guide, there's a mix of both ways in the codebase.
| return automation.use(cdpAutomation) | ||
| automation.use(cdpAutomation) | ||
|
|
||
| return cdpAutomation |
There was a problem hiding this comment.
(for comparison, this is a good use of async/await, because we want to call a method on cdpAutomation after the first promise resolves)
|
|
||
| await pageCriClient.send('Page.enable') | ||
|
|
||
| await options.onInitializeNewBrowserTab?.() | ||
| await options['onInitializeNewBrowserTab']?.() |
There was a problem hiding this comment.
It bypasses type checks on options, otherwise you get: Property 'onInitializeNewBrowserTab' does not exist on type 'BrowserLaunchOpts | BrowserNewTabOpts'. since it's only on one of the two.
Open to suggestions on a cleaner way to do this... await (options as BrowserNewTabOpts).onInitializeNewBrowserTab?.() ??
| onFocus () { | ||
| if (options.show) { | ||
| if (!options.browser.isHeadless) { |
There was a problem hiding this comment.
options.show seems clearer to me?
There was a problem hiding this comment.
options.show was coming from the run.ts defaultBrowserOptions which was removed in this PR. Here, we only have isHeadless, and I guess we have defaults.show here as well, which is also defined off of isHeadless. I figured it was clearer to use isHeadless again than to use defaults.show (which is actually just isHeadless)
Side note - the data flow and variable naming in this launcher is off the rails. It's due for a major refactor...
User facing changelog
n/a
Additional details
run.tsand in to the browser launchers themselves, in preparation for chore(webkit): add video recording #23579Steps to test
How has the user experience changed?
PR Tasks
cypress-documentation?type definitions?