fix: prevent timing out on short/skipped videos (#28643)

Author: emilyrohrbough

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).

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,26 +574,14 @@ 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 })
     await Bluebird.delay(DELAY_TO_LET_VIDEO_FINISH_MS)
     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

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)

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)
+    })
+  })
 })

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<void>
+  endVideoCapture: (waitForMoreFrames: boolean) => Promise<void>
   /**
    * Timestamp of when the video capture started - used for chapter timestamps.
    */