chore(webkit): add video recording #23579

flotwig · 2022-08-27T03:14:42Z

For Safari (WebKit) Support #6422

User facing changelog

n/a

Additional details

Moves browser-specific video recording logic out of run.ts and in to the browsers/* drivers.
- This happened in e3b1979
- Browsers now control the video recording more closely, and only expose a small API to run.ts (start/end/restart control)
Adds WebKit video recording.
- This necessitated the above refactor - WebKit only supports recording an entire Page session at a time, and it streams to disk, so we can't use the ffmpeg method we regularly use with WK.
Adds a webkit-system-tests job. 60 .its are skipped in this PR since they don't work yet. It breaks down like this:
- 6 skipped due to needing multidomain support
- 8 skipped due to missing before:browser:launch support
- 22 skipped due to broken stack traces
- 24 other
But importantly for this PR, the video_compression system-test spec is unskipped and passing in WebKit
Depends on refactor: move more of video capture into browser automations #23587

Steps to test

Run a test project that records video in WebKit, for example:

yarn cypress:run --project ../cypress-test-tiny --browser webkit --config video=true

(the project needs to have playwright-webkit installed)

Video should be recorded to the videosFolder after doing this. No guarantee that the project won't fail for other reasons since WebKit is WIP.

How has the user experience changed?

PR Tasks

Have tests been added/updated?
[na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?

cypress-bot · 2022-08-27T03:14:44Z

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.

cypress · 2022-08-27T03:34:27Z

Test summary

39674 • 0 • 3345 • 0 • 3

Run details


Project	cypress
Status	Passed
Commit	`edff645`
Started	Sep 6, 2022 4:48 PM
Ended	Sep 6, 2022 5:03 PM
Duration	15:04 💡
OS	Linux Debian - 11.3
Browser	Multiple

View run in Cypress Dashboard ➡️

Flakiness

	angular.cy.ts	1
1	Working with angular-14 > should mount a passing test
	e2e/origin/events.cy.ts	2
1	cy.origin > window:before:load event
2	cy.origin > post window load events > window:unload

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

…owserOpts

flotwig · 2022-08-31T16:08:19Z

packages/server/test/integration/cypress_spec.js

@@ -1024,7 +1024,6 @@ describe('lib/cypress', () => {
              browser: 'electron',
              foo: 'bar',
              onNewWindow: sinon.match.func,
-              writeVideoFrame: sinon.match.func,


Removed this unit assertion since it's complex to mock now and is covered by other Electron tests that record video.

flotwig · 2022-08-31T18:30:25Z

packages/types/src/video.ts

+export type VideoRecording = {
+  api: RunModeVideoApi
+  controller?: BrowserVideoController
+}


I'm super open to input on these APIs, especially around naming and structure. I know it's currently not the smoothest experience. They're structured the way they are to avoid a complete refactor of this part of run.ts, where two complex, asynchronous code paths that are interdependent require different parts of the BrowserVideoController at different times:

cypress/packages/server/lib/modes/run.ts

Lines 848 to 879 in 4ad2279

const [results] = await Promise.all([

waitForTestsToFinishRunning({

spec,

config,

project,

estimated,

screenshots,

videoRecording,

exit: options.exit,

testingType: options.testingType,

videoCompression: options.videoCompression,

videoUploadOnPasses: options.videoUploadOnPasses,

quiet: options.quiet,

shouldKeepTabOpen: !isLastSpec,

}),

waitForBrowserToConnect({

spec,

project,

browser,

screenshots,

onError,

videoRecording,

socketId: options.socketId,

webSecurity: options.webSecurity,

projectRoot: options.projectRoot,

testingType: options.testingType,

isFirstSpec,

experimentalSingleTabRunMode: config.experimentalSingleTabRunMode,

shouldLaunchNewTab: !isFirstSpec,

}),

])

weyert · 2022-08-31T22:00:44Z

Nice, great job, that’s a nice approach to add video capturing for Webkit :)

rachelruderman · 2022-09-01T18:04:21Z

@flotwig can you update the PR description with Steps to Test? 🙏

flotwig · 2022-09-01T18:06:25Z

can you update the PR description with Steps to Test? pray

@rachelruderman done

rachelruderman

Tried and tested!! Great job!!

lmiller1990 · 2022-09-06T01:11:02Z

I did not review this one yet (was not assigned, but can take a look this week) - but I did test it out as part of #23662 and the recording feature is working as expected, at least based on a few tests. I did not do more in depth testing yet.

Should we be able to see recordings in the dashboard from WK with this PR @flotwig? My understanding is WK is only installed during local dev, so I guess not?

tbiethman · 2022-09-06T13:43:11Z

packages/server/lib/browsers/firefox.ts

@@ -529,7 +536,7 @@ export async function open (browser: Browser, url: string, options: BrowserLaunc
    // user can overwrite this default with these env vars or --height, --width arguments
    MOZ_HEADLESS_WIDTH: '1280',
    MOZ_HEADLESS_HEIGHT: '721',
-  }) as LaunchedBrowser & { browserCriClient: BrowserCriClient}
+  }) as unknown as BrowserInstance


I haven't seen this before, tricky.

Yeah... I don't know the "right" way to change the type of an object, type-safely, without using class ... extends .... If there even is a way without as unknown. Would be nice to eventually remove the need for this.

This one seems a quick fix #23693

I think what you are referring to is something like

function foo<T extends SomeBaseClass>(arg: T): T { // }

Now as long as arg extends SomeBaseClass you should get type safety (error if you don't extend the base class).

Maybe like this?

https://www.typescriptlang.org/play?#code/IYIwzgLgTsDGEAJYBthjAgagUwBYEsVsEBvAKAUoVEhngQHNtF8ATBACgEoEAuBWvgB2DMmKoIA7rmzZkYfkICuAWxDYoFKrAD2Q2kvg6oHabPmLV6qD3ISJEAmAB0ZuRgC8Ume62UAvmSBZChoGADCwFAI2AAeENhCrBg4BESkfghsCF4A5LBRuZmJDMLY-Lly2PBQhLkIAD4IuQAOzFA6yEWZuvrQhhDGHCVlFVU1dY3NbdCdubaZEmBKbSYALFyLVI74LiNCxF772Jn+CEFioegIAEL4ANbEcQlJKXiEyMR2VNl5IA-YIoSFQ6VhyCogFSxepNXIgpRCCDAYQw5odYCsboSXoGIwmEFg5AQqGouE6BFIlFTXLozELexUZarDgAJk2DMoOxcBLkOQQPOQp3OwQAZgj4Pg9IwoLIIAAeAAqMXiiWSWHeRAAfBwWsAAJ7IHQY-gK+lIPRgTrYZyGhgcAAGCr1bSyGAAJCRdQajaxnGwzpJ8I4EB6vYaMa4fPIzso1BoEDoRd5zC57ZtggwZcwOAdJAhIiZKp8JrB5ptM7Kc9g83dHhwyRTkUIy2QgA

Also works with interface and plain objects ... not just limited to class.

packages/types/src/video.ts

flotwig · 2022-09-06T15:52:32Z

Should we be able to see recordings in the dashboard from WK with this PR? My understanding is WK is only installed during local dev, so I guess not?

@lmiller1990 in the monorepo, it's a devDependency, we run most of our tests using the dev version of Cypress so we do have video: https://dashboard.cypress.io/projects/ypt4pf/runs/38732/test-results/abbf0e35-9475-4a03-a446-743cb06c5e22/video

flotwig added 12 commits August 25, 2022 17:10

factor console logging out of run.ts

297e41b

fix print-run

173de9f

minimize diff

e493600

chore(server): convert browsers/index to typescript

beb3f2d

fix tests

4957a83

update stubbed tests

c255548

convert electron.js to .ts

b01e8c0

Suggestions from code review

ce15192

Clean up new type errors

8bd49f3

electron.connectToExisting can be sync

8e8f62a

more type errors for the type god

501895a

Suggestions from code review

5e74399

flotwig force-pushed the webkit-video branch 3 times, most recently from 0375b0a to 7886a3d Compare August 27, 2022 16:53

refactor: move more of video capture into browser automations

d25b9e0

flotwig force-pushed the webkit-video branch from 7886a3d to d25b9e0 Compare August 27, 2022 17:16

unit tests

9b8cfbe

flotwig mentioned this pull request Aug 29, 2022

refactor: move more of video capture into browser automations #23587

Merged

2 tasks

Base automatically changed from browser-types to develop August 29, 2022 19:47

flotwig added 2 commits August 29, 2022 15:51

Merge remote-tracking branch 'origin/develop' into refactor-defaultBr…

a7f6d4b

…owserOpts

refactor: move videoCapture to browsers

e3b1979

flotwig changed the base branch from develop to refactor-defaultBrowserOpts August 30, 2022 01:01

fix snapshots

c3f6915

flotwig mentioned this pull request Aug 30, 2022

Safari (WebKit) Support #6422

Closed

21 tasks

flotwig added 2 commits August 30, 2022 16:36

fix multi-spec videos?

ce0a27f

webkit video recording works!

e41dd70

flotwig force-pushed the webkit-video branch from 5137354 to 9ececdb Compare August 30, 2022 21:47

flotwig requested a review from tbiethman August 31, 2022 15:40

flotwig added 2 commits August 31, 2022 11:49

fix more tests

115a07c

minimize diff, fix ff

4ad2279

flotwig commented Aug 31, 2022

View reviewed changes

flotwig added 3 commits August 31, 2022 12:26

fix unit tests

a449c8f

fix tests

2468f9f

cleanup

d38ae27

flotwig requested a review from BlueWinds August 31, 2022 17:52

flotwig marked this pull request as ready for review August 31, 2022 18:29

flotwig commented Aug 31, 2022

View reviewed changes

Base automatically changed from refactor-defaultBrowserOpts to develop August 31, 2022 18:55

flotwig force-pushed the webkit-video branch from a1efcbf to a8c42cf Compare September 1, 2022 03:51

Merge remote-tracking branch 'origin/develop' into webkit-video

47adf21

flotwig force-pushed the webkit-video branch from a8c42cf to 47adf21 Compare September 1, 2022 14:29

This was referenced Sep 1, 2022

chore(webkit): add before:browser:launch and download support #23662

Merged

feat(webkit): fix multidomain driver tests in WebKit #23442

Merged

rachelruderman approved these changes Sep 2, 2022

View reviewed changes

clean up wantsWrite logic

22ddb94

tbiethman reviewed Sep 6, 2022

View reviewed changes

packages/types/src/video.ts Outdated Show resolved Hide resolved

tbiethman approved these changes Sep 6, 2022

View reviewed changes

flotwig added 2 commits September 6, 2022 11:56

Clean up setVideoController/newFfmpegVideoController naming

45e9b8f

Merge branch 'develop' into webkit-video

edff645

flotwig merged commit dc9e9dc into develop Sep 6, 2022

flotwig deleted the webkit-video branch September 6, 2022 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(webkit): add video recording #23579

chore(webkit): add video recording #23579

flotwig commented Aug 27, 2022 •

edited

Loading

cypress-bot bot commented Aug 27, 2022

cypress bot commented Aug 27, 2022 •

edited

Loading

flotwig Aug 31, 2022

flotwig Aug 31, 2022

weyert commented Aug 31, 2022 •

edited

Loading

rachelruderman commented Sep 1, 2022

flotwig commented Sep 1, 2022

rachelruderman left a comment

lmiller1990 commented Sep 6, 2022

tbiethman Sep 6, 2022

flotwig Sep 6, 2022

lmiller1990 Sep 7, 2022

lmiller1990 Sep 7, 2022 •

edited

Loading

flotwig commented Sep 6, 2022

	const [results] = await Promise.all([
	waitForTestsToFinishRunning({
	spec,
	config,
	project,
	estimated,
	screenshots,
	videoRecording,
	exit: options.exit,
	testingType: options.testingType,
	videoCompression: options.videoCompression,
	videoUploadOnPasses: options.videoUploadOnPasses,
	quiet: options.quiet,
	shouldKeepTabOpen: !isLastSpec,
	}),

	waitForBrowserToConnect({
	spec,
	project,
	browser,
	screenshots,
	onError,
	videoRecording,
	socketId: options.socketId,
	webSecurity: options.webSecurity,
	projectRoot: options.projectRoot,
	testingType: options.testingType,
	isFirstSpec,
	experimentalSingleTabRunMode: config.experimentalSingleTabRunMode,
	shouldLaunchNewTab: !isFirstSpec,
	}),
	])

chore(webkit): add video recording #23579

chore(webkit): add video recording #23579

Conversation

flotwig commented Aug 27, 2022 • edited Loading

User facing changelog

Additional details

Steps to test

How has the user experience changed?

PR Tasks

cypress-bot bot commented Aug 27, 2022

cypress bot commented Aug 27, 2022 • edited Loading

Test summary

Run details

Flakiness

flotwig Aug 31, 2022

Choose a reason for hiding this comment

flotwig Aug 31, 2022

Choose a reason for hiding this comment

weyert commented Aug 31, 2022 • edited Loading

rachelruderman commented Sep 1, 2022

flotwig commented Sep 1, 2022

rachelruderman left a comment

Choose a reason for hiding this comment

lmiller1990 commented Sep 6, 2022

tbiethman Sep 6, 2022

Choose a reason for hiding this comment

flotwig Sep 6, 2022

Choose a reason for hiding this comment

lmiller1990 Sep 7, 2022

Choose a reason for hiding this comment

lmiller1990 Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

flotwig commented Sep 6, 2022

flotwig commented Aug 27, 2022 •

edited

Loading

cypress bot commented Aug 27, 2022 •

edited

Loading

weyert commented Aug 31, 2022 •

edited

Loading

lmiller1990 Sep 7, 2022 •

edited

Loading