-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore: rename video processing events to capture/compress #26800
Conversation
@@ -515,7 +515,7 @@ export function displayVideoProcessingProgress (opts: { videoName: string, video | |||
const dur = `${humanTime.long(finished)}` | |||
|
|||
const table = terminal.table({ | |||
colWidths: [3, 21, 61, 15], | |||
colWidths: [3, 22, 60, 15], |
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.
had to offset the 2nd column by one in order to not get finished compressin...
and not get the g
truncated. I took a character from the 3rd column as we have more than ample room to store the results of the compression, but means the snapshots will be missing a space in that column (hence the regen in the system tests)
// or we've been told not to upload the video | ||
if (options.videoCompression === false || options.videoCompression === 0 || options.shouldUploadVideo === false) { | ||
debug('skipping compression') |
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.
added a debug log if this condition is met since otherwise its hard to tell. I wonder if we want to add any of these options to the telemetry span in the future to determine if this is a no-op (we can do it in this PR as well just wanted to see if others thought it was a good idea).
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.
videoname, videoCompression and compressedVideoName are added to the span now. You could add 'should upload' if you want. (down on line 685)
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 missed that. I think that should be enough for us to infer the behavior!
13 flaky tests on run #46568 ↗︎
Details:
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox
e2e/origin/commands/assertions.cy.ts • 1 flaky test • 5x-driver-firefox
cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox
commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-electron
cypress/cypress.cy.js • 3 flaky tests • 5x-driver-electron
The first 5 flaky specs are shown, see all 7 specs in Cypress Cloud. This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
- Started compressing: Compressing to 32 CRF | ||
- Finished compressing: /XXX/XXX/XXX/cypress/videos/simple.cy.js.mp4 (X second) |
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.
you have no idea how lucky you are to make this change after i turned off video compression in snapshots. lol
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.
Let's just say there were a lot LESS snapshots that needed updated 🙏🏻
548d97a
to
4191be4
Compare
4191be4
to
50bddeb
Compare
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
Additional details
The goal of this PR is to change the naming nomenclature of video output when printed to the terminal. Today, ending capture of video and compression is called "processing". This has been confusing to users because it is unclear whether the video is being saved, compressed , or uploaded to the cloud. In an effort to make this more clear, areas that are related to compression/compression failure are renamed to "compressing" nomenclature and areas related to capture/capture failures are renamed to"capture" nomenclature.
When capturing now fails, a message is displayed to the user that capturing instead of processing has failed, so it is clearer where the failure came from. This happens in FF as video recording is not currently working and is unable to capture any frames from the screencast API.
Steps to test
execute a Cypress test in run mode with video/cloud recording on
How has the user experience changed?
old terminal output
new terminal output
old terminal output with firefox failed recording
new terminal output with firefox failed recording(capturing fails instead of "processing")
PR Tasks
cypress-documentation
? (did a quick audit and didn't see anyplace in the docs where "processing" in mentioned or screenshots to terminal output need updating)type definitions
?