From 4b1df3574157461cd082a717fd5e9f3ac5cc16f1 Mon Sep 17 00:00:00 2001 From: Emily Rohrbough Date: Thu, 11 Jan 2024 09:56:31 -0600 Subject: [PATCH] fix: prevent timing out on short/skipped videos (#28643) --- cli/CHANGELOG.md | 1 + packages/server/lib/modes/run.ts | 28 ++++---- packages/server/lib/video_capture.ts | 37 ++++++---- .../test/integration/video_capture_spec.ts | 71 +++++++++++++------ packages/types/src/video.ts | 2 +- 5 files changed, 86 insertions(+), 53 deletions(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index f30474a96120..5f7792792011 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -5,6 +5,7 @@ _Released 1/16/2024 (PENDING)_ **Bugfixes:** +- No longer wait for additional frames when recording a video for a spec that was skipped by the Cloud due to Auto Cancellation. Fixes [#27898](https://github.com/cypress-io/cypress/issues/27898). - Now `node_modules` will not be ignored if a project path or a provided path to spec files contains it. Fixes [#23616](https://github.com/cypress-io/cypress/issues/23616). - Updated display of assertions and commands with a URL argument to escape markdown formatting so that values are displayed as is and assertion values display as bold. Fixes [#24960](https://github.com/cypress-io/cypress/issues/24960) and [#28100](https://github.com/cypress-io/cypress/issues/28100). - When generating assertions via Cypress Studio, the preview of the generated assertions now correctly displays the past tense of 'expected' instead of 'expect'. Fixed in [#28593](https://github.com/cypress-io/cypress/pull/28593). diff --git a/packages/server/lib/modes/run.ts b/packages/server/lib/modes/run.ts index 77719b283b4d..746bdac2c29f 100644 --- a/packages/server/lib/modes/run.ts +++ b/packages/server/lib/modes/run.ts @@ -554,6 +554,18 @@ async function waitForTestsToFinishRunning (options: { project: Project, screens debug('received project end') + _.defaults(results, { + error: null, + hooks: null, + tests: null, + video: null, + screenshots: null, + reporterStats: null, + }) + + // Cypress Cloud told us to skip this spec + const skippedSpec = results.skippedSpec + // https://github.com/cypress-io/cypress/issues/2370 // delay 1 second if we're recording a video to give // the browser padding to render the final frames @@ -562,7 +574,7 @@ async function waitForTestsToFinishRunning (options: { project: Project, screens debug('received videoController %o', { videoController }) - if (videoController) { + if (videoController && !skippedSpec) { const span = telemetry.startSpan({ name: 'video:capture:delayToLetFinish' }) debug('delaying to extend video %o', { DELAY_TO_LET_VIDEO_FINISH_MS }) @@ -570,18 +582,6 @@ async function waitForTestsToFinishRunning (options: { project: Project, screens span?.end() } - _.defaults(results, { - error: null, - hooks: null, - tests: null, - video: null, - screenshots: null, - reporterStats: null, - }) - - // Cypress Cloud told us to skip this spec - const skippedSpec = results.skippedSpec - if (screenshots) { results.screenshots = screenshots } @@ -603,7 +603,7 @@ async function waitForTestsToFinishRunning (options: { project: Project, screens } try { - await videoController.endVideoCapture() + await videoController.endVideoCapture(!skippedSpec) debug('ended video capture') } catch (err) { videoCaptureFailed = true diff --git a/packages/server/lib/video_capture.ts b/packages/server/lib/video_capture.ts index 8375e0f5cb4d..87ee0c9256e7 100644 --- a/packages/server/lib/video_capture.ts +++ b/packages/server/lib/video_capture.ts @@ -114,35 +114,36 @@ export type StartOptions = { // If set, expect input frames as webm chunks. webmInput?: boolean // Callback for asynchronous errors in video capturing/compression. - onError?: (err: Error, stdout: string, stderr: string) => void + onError?: (err: Error) => void } export function start (options: StartOptions) { const pt = new stream.PassThrough() const ended = deferredPromise() - let done = false + let doneCapturing = false let wantsWrite = true - let skippedChunksCount = 0 - let writtenChunksCount = 0 + let skippedFramesCount = 0 + let writtenFramesCount = 0 _.defaults(options, { onError () {}, }) - const endVideoCapture = function (waitForMoreChunksTimeout = 3000) { - debugFrames('frames written:', writtenChunksCount) + const endVideoCapture = function (waitForMoreFrames = true) { + debugFrames('frames written:', writtenFramesCount) // in some cases (webm) ffmpeg will crash if fewer than 2 buffers are // written to the stream, so we don't end capture until we get at least 2 - if (writtenChunksCount < 2) { + if (writtenFramesCount < 2 && waitForMoreFrames) { return new Bluebird((resolve) => { pt.once('data', resolve) }) .then(() => endVideoCapture()) - .timeout(waitForMoreChunksTimeout) + .timeout(3000) + .catch(() => endVideoCapture(false)) } - done = true + doneCapturing = true pt.end() @@ -158,7 +159,7 @@ export function start (options: StartOptions) { // our stream yet because paint // events can linger beyond // finishing the actual video - if (done) { + if (doneCapturing) { return } @@ -185,7 +186,7 @@ export function start (options: StartOptions) { lengths[data.length] = true } - writtenChunksCount++ + writtenFramesCount++ debugFrames('writing video frame') @@ -200,9 +201,9 @@ export function start (options: StartOptions) { }) } } else { - skippedChunksCount += 1 + skippedFramesCount += 1 - return debugFrames('skipping video frame %o', { skipped: skippedChunksCount }) + return debugFrames('skipping video frame %o', { skipped: skippedFramesCount }) } } @@ -228,8 +229,14 @@ export function start (options: StartOptions) { }).on('error', (err, stdout, stderr) => { debug('capture errored: %o', { error: err.message, stdout, stderr }) - // bubble errors up - options.onError?.(err, stdout, stderr) + if (err.message.includes('ffmpeg exited with code 1: pipe:0')) { + err.message = 'Insufficient frames captured to create video.' + } + + // bubble errors up if occurs before endCapture is called + if (!doneCapturing) { + options.onError?.(err) + } // reject the ended promise return ended.reject(err) diff --git a/packages/server/test/integration/video_capture_spec.ts b/packages/server/test/integration/video_capture_spec.ts index 451879aa09c2..f572dcd3c338 100644 --- a/packages/server/test/integration/video_capture_spec.ts +++ b/packages/server/test/integration/video_capture_spec.ts @@ -4,6 +4,10 @@ import path from 'path' import fse from 'fs-extra' import os from 'os' +const image1Path = path.join(__dirname, '..', '..', '..', 'icons', 'assets', 'cypress.iconset', 'icon_16x16.png') +const image2Path = path.join(__dirname, '..', '..', '..', 'icons', 'assets', 'cypress.iconset', 'icon_32x32.png') +const image3Path = path.join(__dirname, '..', '..', '..', 'icons', 'assets', 'cypress.iconset', 'icon_128x128.png') + async function startSpiedVideoCapture (videoName, options = {}) { const props = await videoCapture.start({ videoName, ...options }) @@ -28,7 +32,7 @@ Output file #0 does not contain any stream\n` } describe('Video Capture', () => { - context('#start', () => { + context('#start.writeVideoFrame', () => { let tmpFilename beforeEach(() => { @@ -63,27 +67,6 @@ describe('Video Capture', () => { await expect(endVideoCapture()).rejectedWith(END_OF_FILE_ERROR) }) - it('will eventually timeout on single frame write', async () => { - const { writeVideoFrameAsBuffer, endVideoCapture } = await startSpiedVideoCapture(tmpFilename) - - writeVideoFrameAsBuffer('foo') - - await expect(endVideoCapture(1)).be.rejectedWith('operation timed out') - }) - - // https://github.com/cypress-io/cypress/issues/6408 - it('waits for at least 2 stream writes before ending', async () => { - const { writeVideoFrameAsBuffer, endVideoCapture, END_OF_FILE_ERROR } = await startSpiedVideoCapture(tmpFilename) - - writeVideoFrameAsBuffer('foo') - - const endVideoCaptureResult = endVideoCapture() - - writeVideoFrameAsBuffer('foobar') - - await expect(endVideoCaptureResult).rejectedWith(END_OF_FILE_ERROR) - }) - // https://github.com/cypress-io/cypress/issues/16648 context('deduping frames', async () => { it('does not dedupe when not webminput', async () => { @@ -94,7 +77,6 @@ describe('Video Capture', () => { writeVideoFrameAsBuffer('foo') writeVideoFrameAsBuffer('foo') expect(_pt.write).callCount(4) - // await expect(endVideoCapture()).rejectedWith(END_OF_FILE_ERROR) }) }) @@ -108,4 +90,47 @@ describe('Video Capture', () => { expect(_pt.write).calledOnce }) }) + + context('#start.endVideoCapture', () => { + let tmpFilename + + beforeEach(() => { + tmpFilename = path.join(fse.mkdtempSync(path.join(os.tmpdir(), 'cy-video-')), 'video.mp4') + }) + + it('ends immediately if more than two frames written', async () => { + const { writeVideoFrame, endVideoCapture } = await startSpiedVideoCapture(tmpFilename) + + writeVideoFrame(fse.readFileSync(image1Path)) + writeVideoFrame(fse.readFileSync(image2Path)) + writeVideoFrame(fse.readFileSync(image3Path)) + + const waitForMoreFrames = false + + await endVideoCapture(waitForMoreFrames) + }) + + // https://github.com/cypress-io/cypress/issues/6408 + it('waits for at least 2 stream writes before ending if spec not skipped by the cloud', async () => { + const { writeVideoFrame, endVideoCapture } = await startSpiedVideoCapture(tmpFilename) + + writeVideoFrame(fse.readFileSync(image1Path)) + + const waitForMoreFrames = true + const endVideoCaptureResult = endVideoCapture(waitForMoreFrames) + + writeVideoFrame(fse.readFileSync(image2Path)) + + await endVideoCaptureResult + }) + + it('ends immediately if less than two frames have been written and spec is skipped by the cloud', async () => { + const { writeVideoFrame, endVideoCapture } = await startSpiedVideoCapture(tmpFilename) + + writeVideoFrame(fse.readFileSync(image1Path)) + const waitForMoreFrames = false + + await endVideoCapture(waitForMoreFrames) + }) + }) }) diff --git a/packages/types/src/video.ts b/packages/types/src/video.ts index 7bc3fcf63688..2f665c4a7ab4 100644 --- a/packages/types/src/video.ts +++ b/packages/types/src/video.ts @@ -25,7 +25,7 @@ export type BrowserVideoController = { /** * A function that resolves once the video is fully captured and flushed to disk. */ - endVideoCapture: () => Promise + endVideoCapture: (waitForMoreFrames: boolean) => Promise /** * Timestamp of when the video capture started - used for chapter timestamps. */