fix: prevent overwriting of video files

cli/CHANGELOG.md

@@ -6,6 +6,7 @@ _Released 12/17/2024 (PENDING)_
 **Bugfixes:**
 
 - Fixed an issue where targets may hang if `Network.enable` is not implemented for the target. Addresses [#29876](https://github.com/cypress-io/cypress/issues/29876).
+- Prevent overwriting of video files across multiple runs. Addresses [#8280](https://github.com/cypress-io/cypress/issues/8280). Addressed in [#30673](https://github.com/cypress-io/cypress/pull/30673).
 - Updated Firefox `userChrome.css` to correctly hide the toolbox during headless mode. Addresses [#30721](https://github.com/cypress-io/cypress/issues/30721).
 
 ## 13.16.1

packages/server/lib/modes/run.ts

@@ -13,7 +13,7 @@ import Reporter from '../reporter'
 import browserUtils from '../browsers'
 import { openProject } from '../open_project'
 import * as videoCapture from '../video_capture'
-import { fs } from '../util/fs'
+import { fs, getPath } from '../util/fs'
 import runEvents from '../plugins/run_events'
 import env from '../util/env'
 import trash from '../util/trash'
@@ -224,15 +224,24 @@ async function trashAssets (config: Cfg) {
   }
 }
 
-async function startVideoRecording (options: { previous?: VideoRecording, project: Project, spec: SpecWithRelativeRoot, videosFolder: string }): Promise<VideoRecording> {
+async function startVideoRecording (options: { previous?: VideoRecording, project: Project, spec: SpecWithRelativeRoot, videosFolder: string, overwrite: boolean }): Promise<VideoRecording> {
   if (!options.videosFolder) throw new Error('Missing videoFolder for recording')
 
-  function videoPath (suffix: string) {
-    return path.join(options.videosFolder, options.spec.relativeToCommonRoot + suffix)
+  async function videoPath (suffix: string, ext: string) {
+    const specPath = options.spec.relativeToCommonRoot + suffix
+    const data = {
+      name: specPath,
+      testFailure: false,
+      testAttemptIndex: 0,
+      titles: [],
+    }
+
+    // getPath returns a Promise!!!
+    return await getPath(data, ext, options.videosFolder, options.overwrite)
   }
 
-  const videoName = videoPath('.mp4')
-  const compressedVideoName = videoPath('-compressed.mp4')
+  const videoName = await videoPath('', 'mp4')
+  const compressedVideoName = await videoPath('-compressed', 'mp4')
 
   const outputDir = path.dirname(videoName)
 
@@ -333,6 +342,13 @@ async function compressRecording (options: { quiet: boolean, videoCompression: n
   if (options.videoCompression === false || options.videoCompression === 0) {
     debug('skipping compression')
 
+    // the getSafePath used to get the compressedVideoName creates the file
+    // in order to check if the path is safe or not. So here, if the compressed
+    // file exists, we remove it as compression is not enabled
+    if (fs.existsSync(options.processOptions.compressedVideoName)) {
+      await fs.remove(options.processOptions.compressedVideoName)
+    }
+
     return
   }
 
@@ -945,7 +961,7 @@ async function runSpec (config, spec: SpecWithRelativeRoot, options: { project:
   async function getVideoRecording () {
     if (!options.video) return undefined
 
-    const opts = { project, spec, videosFolder: options.videosFolder }
+    const opts = { project, spec, videosFolder: options.videosFolder, overwrite: options.config.trashAssetsBeforeRuns }
 
     telemetry.startSpan({ name: 'video:capture' })
 

packages/server/lib/screenshots.js

@@ -1,30 +1,17 @@
 const _ = require('lodash')
 const mime = require('mime')
-const path = require('path')
 const Promise = require('bluebird')
 const dataUriToBuffer = require('data-uri-to-buffer')
 const Jimp = require('jimp')
 const sizeOf = require('image-size')
 const colorString = require('color-string')
-const sanitize = require('sanitize-filename')
 let debug = require('debug')('cypress:server:screenshot')
 const plugins = require('./plugins')
-const { fs } = require('./util/fs')
-
-const RUNNABLE_SEPARATOR = ' -- '
-const pathSeparatorRe = /[\\\/]/g
+const { fs, getPath } = require('./util/fs')
 
 // internal id incrementor
 let __ID__ = null
 
-// many filesystems limit filename length to 255 bytes/characters, so truncate the filename to
-// the smallest common denominator of safe filenames, which is 255 bytes. when ENAMETOOLONG
-// errors are encountered, `maxSafeBytes` will be decremented to at most `MIN_PREFIX_BYTES`, at
-// which point the latest ENAMETOOLONG error will be emitted.
-// @see https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits
-let maxSafeBytes = Number(process.env.CYPRESS_MAX_SAFE_FILENAME_BYTES) || 254
-const MIN_PREFIX_BYTES = 64
-
 // TODO: when we parallelize these builds we'll need
 // a semaphore to access the file system when we write
 // screenshots since its possible two screenshots with
@@ -293,83 +280,6 @@ const getDimensions = function (details) {
   return pick(details.image.bitmap)
 }
 
-const ensureSafePath = function (withoutExt, extension, overwrite, num = 0) {
-  const suffix = `${(num && !overwrite) ? ` (${num})` : ''}.${extension}`
-
-  const maxSafePrefixBytes = maxSafeBytes - suffix.length
-  const filenameBuf = Buffer.from(path.basename(withoutExt))
-
-  if (filenameBuf.byteLength > maxSafePrefixBytes) {
-    const truncated = filenameBuf.slice(0, maxSafePrefixBytes).toString()
-
-    withoutExt = path.join(path.dirname(withoutExt), truncated)
-  }
-
-  const fullPath = [withoutExt, suffix].join('')
-
-  debug('ensureSafePath %o', { withoutExt, extension, num, maxSafeBytes, maxSafePrefixBytes })
-
-  return fs.pathExists(fullPath)
-  .then((found) => {
-    if (found && !overwrite) {
-      return ensureSafePath(withoutExt, extension, overwrite, num + 1)
-    }
-
-    // path does not exist, attempt to create it to check for an ENAMETOOLONG error
-    return fs.outputFileAsync(fullPath, '')
-    .then(() => fullPath)
-    .catch((err) => {
-      debug('received error when testing path %o', { err, fullPath, maxSafePrefixBytes, maxSafeBytes })
-
-      if (err.code === 'ENAMETOOLONG' && maxSafePrefixBytes >= MIN_PREFIX_BYTES) {
-        maxSafeBytes -= 1
-
-        return ensureSafePath(withoutExt, extension, overwrite, num)
-      }
-
-      throw err
-    })
-  })
-}
-
-const sanitizeToString = (title) => {
-  // test titles may be values which aren't strings like
-  // null or undefined - so convert before trying to sanitize
-  return sanitize(_.toString(title))
-}
-
-const getPath = function (data, ext, screenshotsFolder, overwrite) {
-  let names
-  const specNames = (data.specName || '')
-  .split(pathSeparatorRe)
-
-  if (data.name) {
-    names = data.name.split(pathSeparatorRe).map(sanitize)
-  } else {
-    names = _
-    .chain(data.titles)
-    .map(sanitizeToString)
-    .join(RUNNABLE_SEPARATOR)
-    .concat([])
-    .value()
-  }
-
-  const index = names.length - 1
-
-  // append (failed) to the last name
-  if (data.testFailure) {
-    names[index] = `${names[index]} (failed)`
-  }
-
-  if (data.testAttemptIndex > 0) {
-    names[index] = `${names[index]} (attempt ${data.testAttemptIndex + 1})`
-  }
-
-  const withoutExt = path.join(screenshotsFolder, ...specNames, ...names)
-
-  return ensureSafePath(withoutExt, ext, overwrite)
-}
-
 const getPathToScreenshot = function (data, details, screenshotsFolder) {
   const ext = mime.getExtension(getType(details))
 

packages/server/lib/util/fs.ts

@@ -2,6 +2,20 @@
 
 import Bluebird from 'bluebird'
 import fsExtra from 'fs-extra'
+import sanitize from 'sanitize-filename'
+import path from 'path'
+import _ from 'lodash'
+
+const RUNNABLE_SEPARATOR = ' -- '
+const pathSeparatorRe = /[\\\/]/g
+
+// many filesystems limit filename length to 255 bytes/characters, so truncate the filename to
+// the smallest common denominator of safe filenames, which is 255 bytes. when ENAMETOOLONG
+// errors are encountered, `maxSafeBytes` will be decremented to at most `MIN_PREFIX_BYTES`, at
+// which point the latest ENAMETOOLONG error will be emitted.
+// @see https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits
+let maxSafeBytes = Number(process.env.CYPRESS_MAX_SAFE_FILENAME_BYTES) || 254
+const MIN_PREFIX_BYTES = 64
 
 type Promisified<T extends (...args: any) => any>
   = (...params: Parameters<T>) => Bluebird<ReturnType<T>>
@@ -12,6 +26,88 @@ interface PromisifiedFsExtra {
   readFileAsync: Promisified<typeof fsExtra.readFileSync>
   writeFileAsync: Promisified<typeof fsExtra.writeFileSync>
   pathExistsAsync: Promisified<typeof fsExtra.pathExistsSync>
+  outputFileAsync: Promisified<typeof fsExtra.outputFileSync>
 }
 
+type PathOptions = {
+  specName?: string
+  name: string
+  testFailure: boolean
+  testAttemptIndex: number
+  titles: Array<string>
+};
+
 export const fs = Bluebird.promisifyAll(fsExtra) as PromisifiedFsExtra & typeof fsExtra
+
+const ensureSafePath = async function (withoutExt: string, extension: string, overwrite: boolean, num: number = 0): Promise<string> {
+  const suffix = `${(num && !overwrite) ? ` (${num})` : ''}.${extension}`
+
+  const maxSafePrefixBytes = maxSafeBytes - suffix.length
+  const filenameBuf = Buffer.from(path.basename(withoutExt))
+
+  if (filenameBuf.byteLength > maxSafePrefixBytes) {
+    const truncated = filenameBuf.slice(0, maxSafePrefixBytes).toString()
+
+    withoutExt = path.join(path.dirname(withoutExt), truncated)
+  }
+
+  const fullPath = [withoutExt, suffix].join('')
+
+  return fs.pathExists(fullPath)
+  .then((found) => {
+    if (found && !overwrite) {
+      return ensureSafePath(withoutExt, extension, overwrite, num + 1)
+    }
+
+    // path does not exist, attempt to create it to check for an ENAMETOOLONG error
+    return fs.outputFileAsync(fullPath, '')
+    .then(() => fullPath)
+    .catch((err) => {
+      if (err.code === 'ENAMETOOLONG' && maxSafePrefixBytes >= MIN_PREFIX_BYTES) {
+        maxSafeBytes -= 1
+
+        return ensureSafePath(withoutExt, extension, overwrite, num)
+      }
+
+      throw err
+    })
+  })
+}
+
+const sanitizeToString = (title: any, idx: number, arr: Array<string>) => {
+  // test titles may be values which aren't strings like
+  // null or undefined - so convert before trying to sanitize
+  return sanitize(_.toString(title))
+}
+
+export const getPath = async function (data: PathOptions, ext: string, screenshotsFolder: string, overwrite: boolean): Promise<string> {
+  let names
+  const specNames = (data.specName || '')
+  .split(pathSeparatorRe)
+
+  if (data.name) {
+    names = data.name.split(pathSeparatorRe).map(sanitizeToString)
+  } else {
+    // we put this in array so to match with type of the if branch above
+    names = [_
+    .chain(data.titles)
+    .map(sanitizeToString)
+    .join(RUNNABLE_SEPARATOR)
+    .value()]
+  }
+
+  const index = names.length - 1
+
+  // append '(failed)' to the last name
+  if (data.testFailure) {
+    names[index] = `${names[index]} (failed)`
+  }
+
+  if (data.testAttemptIndex > 0) {
+    names[index] = `${names[index]} (attempt ${data.testAttemptIndex + 1})`
+  }
+
+  const withoutExt = path.join(screenshotsFolder, ...specNames, ...names)
+
+  return await ensureSafePath(withoutExt, ext, overwrite)
+}

system-tests/README.md

@@ -10,23 +10,23 @@ These tests run in CI in Electron, Chrome, Firefox, and WebKit under the `system
 ## Running System Tests
 
 ```bash
-yarn test # runs all tests
+yarn test-system # runs all tests
 ## or use globbing to find spec in folders as defined in "glob-in-dir" param in package.json
-yarn test screenshot*element # runs screenshot_element_capture_spec.js
-yarn test screenshot # runs screenshot_element_capture_spec.js, screenshot_fullpage_capture_spec.js, ..., etc.
+yarn test-system screenshot*element # runs screenshot_element_capture_spec.js
+yarn test-system screenshot # runs screenshot_element_capture_spec.js, screenshot_fullpage_capture_spec.js, ..., etc.
 
 ```
 
 To keep the browser open after a spec run (for easier debugging and iterating on specs), you can pass the `--no-exit` flag to the test command. Live reloading due to spec changes should also work:
 
 ```sh
-yarn test go_spec.js --browser chrome --no-exit
+yarn test-system go_spec.js --browser chrome --no-exit
 ```
 
 To debug the Cypress process under test, you can pass `--cypress-inspect-brk`:
 
 ```sh
-yarn test go_spec.js --browser chrome --no-exit --cypress-inspect-brk
+yarn test-system go_spec.js --browser chrome --no-exit --cypress-inspect-brk
 ```
 
 ## Developing Tests

system-tests/projects/issue-8280-retain-video/cypress.config.js

@@ -0,0 +1,6 @@
+module.exports = {
+  e2e: {
+    supportFile: false,
+    video: true,
+  },
+}

system-tests/projects/issue-8280-retain-video/cypress/e2e/spec1.cy.js

@@ -0,0 +1,13 @@
+// here the delays  are just so there is something in the screenshots and recordings.
+
+describe('spec1', () => {
+  it('testCase1', () => {
+    cy.wait(500)
+    assert(false)
+  })
+
+  it('testCase2', () => {
+    cy.wait(500)
+    assert(true)
+  })
+})

system-tests/projects/issue-8280-retain-video/cypress/e2e/spec2.cy.js

@@ -0,0 +1,13 @@
+// here the delays  are just so there is something in the screenshots and recordings.
+
+describe('spec2', () => {
+  it('testCase1', () => {
+    cy.wait(500)
+    assert(false)
+  })
+
+  it('testCase2', () => {
+    cy.wait(500)
+    assert(true)
+  })
+})