fix: prevent overwriting of video files (#30673)

* fix: use getPath to prevent video file overwrite

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* chore: update changelog

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* test: add system e2e for the retain videos fix

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* Update cli/CHANGELOG.md

Co-authored-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>

* Update types for screenshots to be in util/fs

* Fix changelog entry placement

* fix extension type

* more types fixes

* fix: add required field in getPath call to satisfy ts

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* fix: sync Data interface from develop branch

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* fix: update SavedDetails type to better definition

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* update changelog

* break out type import into unique line to allow mksnapshot to work

* fix: minor comment fixes

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* fix: change videoPath fn signature as per comment

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* fix: convert the test to async/await

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

* Update CHANGELOG.md

---------

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
Co-authored-by: Jennifer Shehane <jennifer@cypress.io>
Co-authored-by: Jennifer Shehane <shehane.jennifer@gmail.com>
Co-authored-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com>
Co-authored-by: AtofStryker <bglesias@gmail.com>

cli/CHANGELOG.md

@@ -3,6 +3,10 @@
 
 _Released 5/6/2025 (PENDING)_
 
+**Bugfixes:**
+
+- Fixed an issue where the configuration setting `trashAssetsBeforeRuns=false` was ignored for assets in the `videosFolder`. These assets were incorrectly deleted before running tests with `cypress run`. Addresses [#8280](https://github.com/cypress-io/cypress/issues/8280).
+
 **Misc:**
 
 - The URL in the Cypress App no longer displays a white background when the URL is loading. Fixes [#31556](https://github.com/cypress-io/cypress/issues/31556).

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'
@@ -23,6 +23,7 @@ import chromePolicyCheck from '../util/chrome_policy_check'
 import type { SpecWithRelativeRoot, SpecFile, TestingType, OpenProjectLaunchOpts, FoundBrowser, BrowserVideoController, VideoRecording, ProcessOptions, ProtocolManagerShape, AutomationCommands } from '@packages/types'
 import type { Cfg, ProjectBase } from '../project-base'
 import type { Browser } from '../browsers/types'
+import type { Data } from '../util/fs'
 import * as printResults from '../util/print-run'
 import { telemetry } from '@packages/telemetry'
 import { CypressRunResult, createPublicBrowser, createPublicConfig, createPublicRunResults, createPublicSpec, createPublicSpecResults } from './results'
@@ -224,15 +225,30 @@ 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 (ext: string, suffix: string = '') {
+    const specPath = options.spec.relativeToCommonRoot + suffix
+    // tslint:disable-next-line
+    const data: Data = {
+      name: specPath,
+      startTime: new Date(), // needed for ts-lint
+      viewport: {
+        width: 0,
+        height: 0,
+      },
+      specName: '', // this is optional, the getPath will pick up from specPath
+      testFailure: false, //  this is only applicable for screenshot, not for video
+      testAttemptIndex: 0,
+      titles: [],
+    }
+
+    return 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('mp4', '-compressed')
 
   const outputDir = path.dirname(videoName)
 
@@ -333,6 +349,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
   }
 
@@ -949,7 +972,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.ts

@@ -1,54 +1,20 @@
 import _ from 'lodash'
 import Debug from 'debug'
 import mime from 'mime'
-import path from 'path'
 import Promise from 'bluebird'
 import dataUriToBuffer from 'data-uri-to-buffer'
 import Jimp from 'jimp'
 import sizeOf from 'image-size'
 import colorString from 'color-string'
-import sanitize from 'sanitize-filename'
 import * as plugins from './plugins'
-import { fs } from './util/fs'
+import { fs, getPath } from './util/fs'
+import type { Data, ScreenshotsFolder } from './util/fs'
 
 let debug = Debug('cypress:server:screenshot')
-const RUNNABLE_SEPARATOR = ' -- '
-const pathSeparatorRe = /[\\\/]/g
 
 // internal id incrementor
 let __ID__: string | null = null
 
-type ScreenshotsFolder = string | false | undefined
-
-interface Clip {
-  x: number
-  y: number
-  width: number
-  height: number
-}
-
-// TODO: This is likely not representative of the entire Type and should be updated
-interface Data {
-  specName: string
-  name: string
-  startTime: Date
-  viewport: {
-    width: number
-    height: number
-  }
-  titles?: string[]
-  testFailure?: boolean
-  overwrite?: boolean
-  simple?: boolean
-  current?: number
-  total?: number
-  testAttemptIndex?: number
-  appOnly?: boolean
-  hideRunnerUi?: boolean
-  clip?: Clip
-  userClip?: Clip
-}
-
 // TODO: This is likely not representative of the entire Type and should be updated
 interface Details {
   image: any
@@ -61,7 +27,10 @@ interface Details {
 interface SavedDetails {
   size?: string
   takenAt?: Date
-  dimensions?: string
+  dimensions?: {
+    width: number
+    height: number
+  }
   multipart?: any
   pixelRatio?: number
   name?: any
@@ -70,14 +39,6 @@ interface SavedDetails {
   path?: string
 }
 
-// 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
@@ -361,92 +322,6 @@ const getDimensions = function (details) {
   return pick(details.image.bitmap)
 }
 
-const ensureSafePath = function (withoutExt: string, extension: string, overwrite: Data['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
-    // @ts-expect-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: string | null | undefined) => {
-  // 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: Data, ext, screenshotsFolder: ScreenshotsFolder, overwrite: Data['overwrite']) {
-  let names
-  const specNames = (data.specName || '')
-  .split(pathSeparatorRe)
-
-  if (data.name) {
-    // @ts-expect-error
-    names = data.name.split(pathSeparatorRe).map(sanitize)
-  } else {
-    names = _
-    .chain(data.titles)
-    .map(sanitizeToString)
-    .join(RUNNABLE_SEPARATOR)
-    // @ts-expect-error - this shouldn't be necessary, but it breaks if you remove it
-    .concat([])
-    .value()
-  }
-
-  const index = names.length - 1
-
-  // append (failed) to the last name
-  if (data.testFailure) {
-    names[index] = `${names[index]} (failed)`
-  }
-
-  if (data.testAttemptIndex && data.testAttemptIndex > 0) {
-    names[index] = `${names[index]} (attempt ${data.testAttemptIndex + 1})`
-  }
-
-  let withoutExt
-
-  if (screenshotsFolder) {
-    withoutExt = path.join(screenshotsFolder, ...specNames, ...names)
-  } else {
-    withoutExt = path.join(...specNames, ...names)
-  }
-
-  return ensureSafePath(withoutExt, ext, overwrite)
-}
-
 const getPathToScreenshot = function (data: Data, details: Details, screenshotsFolder: 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,117 @@ interface PromisifiedFsExtra {
   readFileAsync: Promisified<typeof fsExtra.readFileSync>
   writeFileAsync: Promisified<typeof fsExtra.writeFileSync>
   pathExistsAsync: Promisified<typeof fsExtra.pathExistsSync>
+  outputFileAsync: Promisified<typeof fsExtra.outputFileSync>
+}
+
+interface Clip {
+  x: number
+  y: number
+  width: number
+  height: number
+}
+
+export type ScreenshotsFolder = string | false | undefined
+
+// TODO: This is likely not representative of the entire Type and should be updated
+export interface Data {
+  specName: string
+  name: string
+  startTime: Date
+  viewport: {
+    width: number
+    height: number
+  }
+  titles?: string[]
+  testFailure?: boolean
+  overwrite?: boolean
+  simple?: boolean
+  current?: number
+  total?: number
+  testAttemptIndex?: number
+  appOnly?: boolean
+  hideRunnerUi?: boolean
+  clip?: Clip
+  userClip?: Clip
 }
 
 export const fs = Bluebird.promisifyAll(fsExtra) as PromisifiedFsExtra & typeof fsExtra
+
+const ensureSafePath = async function (withoutExt: string, extension: string | null, overwrite: boolean | undefined, 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: Data, ext: string | null, screenshotsFolder: ScreenshotsFolder, overwrite: boolean | undefined): 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 && data.testAttemptIndex > 0) {
+    names[index] = `${names[index]} (attempt ${data.testAttemptIndex + 1})`
+  }
+
+  let withoutExt
+
+  if (screenshotsFolder) {
+    withoutExt = path.join(screenshotsFolder, ...specNames, ...names)
+  } else {
+    withoutExt = path.join(...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)
+  })
+})

system-tests/test/issue_8280_spec.js

@@ -0,0 +1,92 @@
+const { fs } = require('@packages/server/lib/util/fs')
+const Fixtures = require('../lib/fixtures')
+const systemTests = require('../lib/system-tests').default
+
+const PROJECT_NAME = 'issue-8280-retain-video'
+
+describe('e2e issue 8280', () => {
+  systemTests.setup()
+
+  // https://github.com/cypress-io/cypress/issues/8280
+
+  it('should retain the videos from previous runs if trashAssetsBeforeRuns=false', async function () {
+    // first run
+    await systemTests.exec(this, {
+      project: PROJECT_NAME,
+      snapshot: false,
+      expectedExitCode: 2,
+      processEnv: {
+        'CYPRESS_trashAssetsBeforeRuns': 'false',
+      },
+    })
+
+    // second run
+    await systemTests.exec(this, {
+      project: PROJECT_NAME,
+      snapshot: false,
+      expectedExitCode: 2,
+      processEnv: {
+        'CYPRESS_trashAssetsBeforeRuns': 'false',
+      },
+    })
+
+    const spec1Screenshots = await fs.readdir(Fixtures.projectPath(`${PROJECT_NAME}/cypress/screenshots/spec1.cy.js`))
+
+    expect(spec1Screenshots.length).to.eq(2)
+    expect(spec1Screenshots).to.include('spec1 -- testCase1 (failed).png')
+    expect(spec1Screenshots).to.include('spec1 -- testCase1 (failed) (1).png')
+
+    const spec2Screenshots = await fs.readdir(Fixtures.projectPath(`${PROJECT_NAME}/cypress/screenshots/spec2.cy.js`))
+
+    expect(spec2Screenshots.length).to.eq(2)
+    expect(spec2Screenshots).to.include('spec2 -- testCase1 (failed).png')
+    expect(spec2Screenshots).to.include('spec2 -- testCase1 (failed) (1).png')
+
+    const videos = await fs.readdir(Fixtures.projectPath(`${PROJECT_NAME}/cypress/videos`))
+
+    expect(videos.length).to.eq(4)
+    expect(videos).to.include('spec1.cy.js.mp4')
+    expect(videos).to.include('spec1.cy.js (1).mp4')
+    expect(videos).to.include('spec2.cy.js.mp4')
+    expect(videos).to.include('spec2.cy.js (1).mp4')
+  })
+
+  // if trash assets = true, then there will be no retention of screenshots or videos
+  it('should not retain the videos from previous runs if trashAssetsBeforeRuns=true', async function () {
+    // first run
+    await systemTests.exec(this, {
+      project: PROJECT_NAME,
+      snapshot: false,
+      expectedExitCode: 2,
+      processEnv: {
+        'CYPRESS_trashAssetsBeforeRuns': 'true',
+      },
+    })
+
+    // second run
+    await systemTests.exec(this, {
+      project: PROJECT_NAME,
+      snapshot: false,
+      expectedExitCode: 2,
+      processEnv: {
+        'CYPRESS_trashAssetsBeforeRuns': 'true',
+      },
+    })
+
+    const spec1Screenshots = await fs.readdir(Fixtures.projectPath(`${PROJECT_NAME}/cypress/screenshots/spec1.cy.js`))
+
+    expect(spec1Screenshots.length).to.eq(1)
+    expect(spec1Screenshots).to.include('spec1 -- testCase1 (failed).png')
+
+    const spec2Screenshots = await fs.readdir(Fixtures.projectPath(`${PROJECT_NAME}/cypress/screenshots/spec2.cy.js`))
+
+    expect(spec2Screenshots.length).to.eq(1)
+    expect(spec2Screenshots).to.include('spec2 -- testCase1 (failed).png')
+
+    const videos = await fs.readdir(Fixtures.projectPath(`${PROJECT_NAME}/cypress/videos`))
+
+    expect(videos.length).to.eq(2)
+    expect(videos).to.include('spec1.cy.js.mp4')
+    expect(videos).to.include('spec2.cy.js.mp4')
+  })
+})