From e8a7c3f5f8a8cb82a4379670d70221df769c6a31 Mon Sep 17 00:00:00 2001 From: Yashodhan <54112038+YJDoc2@users.noreply.github.com> Date: Tue, 29 Apr 2025 19:31:48 +0530 Subject: [PATCH] fix: prevent overwriting of video files (#30673) * fix: use getPath to prevent video file overwrite Signed-off-by: Yashodhan Joshi * chore: update changelog Signed-off-by: Yashodhan Joshi * test: add system e2e for the retain videos fix Signed-off-by: Yashodhan Joshi * 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 * fix: sync Data interface from develop branch Signed-off-by: Yashodhan Joshi * fix: update SavedDetails type to better definition Signed-off-by: Yashodhan Joshi * update changelog * break out type import into unique line to allow mksnapshot to work * fix: minor comment fixes Signed-off-by: Yashodhan Joshi * fix: change videoPath fn signature as per comment Signed-off-by: Yashodhan Joshi * fix: convert the test to async/await Signed-off-by: Yashodhan Joshi * Update CHANGELOG.md --------- Signed-off-by: Yashodhan Joshi Co-authored-by: Jennifer Shehane Co-authored-by: Jennifer Shehane Co-authored-by: Mike McCready <66998419+MikeMcC399@users.noreply.github.com> Co-authored-by: AtofStryker --- cli/CHANGELOG.md | 4 + packages/server/lib/modes/run.ts | 37 ++++- packages/server/lib/screenshots.ts | 137 +----------------- packages/server/lib/util/fs.ts | 125 ++++++++++++++++ system-tests/README.md | 10 +- .../issue-8280-retain-video/cypress.config.js | 6 + .../cypress/e2e/spec1.cy.js | 13 ++ .../cypress/e2e/spec2.cy.js | 13 ++ system-tests/test/issue_8280_spec.js | 92 ++++++++++++ 9 files changed, 294 insertions(+), 143 deletions(-) create mode 100644 system-tests/projects/issue-8280-retain-video/cypress.config.js create mode 100644 system-tests/projects/issue-8280-retain-video/cypress/e2e/spec1.cy.js create mode 100644 system-tests/projects/issue-8280-retain-video/cypress/e2e/spec2.cy.js create mode 100644 system-tests/test/issue_8280_spec.js diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index bf51bc51d8..e1fc001247 100644 --- a/cli/CHANGELOG.md +++ b/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). diff --git a/packages/server/lib/modes/run.ts b/packages/server/lib/modes/run.ts index 0f97e38e97..b8af4ec865 100644 --- a/packages/server/lib/modes/run.ts +++ b/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 { +async function startVideoRecording (options: { previous?: VideoRecording, project: Project, spec: SpecWithRelativeRoot, videosFolder: string, overwrite: boolean }): Promise { 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' }) diff --git a/packages/server/lib/screenshots.ts b/packages/server/lib/screenshots.ts index 8f68189c7a..ff624162eb 100644 --- a/packages/server/lib/screenshots.ts +++ b/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)) diff --git a/packages/server/lib/util/fs.ts b/packages/server/lib/util/fs.ts index 5846acecc8..533803dbed 100644 --- a/packages/server/lib/util/fs.ts +++ b/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 any> = (...params: Parameters) => Bluebird> @@ -12,6 +26,117 @@ interface PromisifiedFsExtra { readFileAsync: Promisified writeFileAsync: Promisified pathExistsAsync: Promisified + outputFileAsync: Promisified +} + +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 { + 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) => { + // 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 { + 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) +} diff --git a/system-tests/README.md b/system-tests/README.md index 031e94c6b0..fd92f05597 100644 --- a/system-tests/README.md +++ b/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 diff --git a/system-tests/projects/issue-8280-retain-video/cypress.config.js b/system-tests/projects/issue-8280-retain-video/cypress.config.js new file mode 100644 index 0000000000..56bd22847c --- /dev/null +++ b/system-tests/projects/issue-8280-retain-video/cypress.config.js @@ -0,0 +1,6 @@ +module.exports = { + e2e: { + supportFile: false, + video: true, + }, +} diff --git a/system-tests/projects/issue-8280-retain-video/cypress/e2e/spec1.cy.js b/system-tests/projects/issue-8280-retain-video/cypress/e2e/spec1.cy.js new file mode 100644 index 0000000000..d46b2c9742 --- /dev/null +++ b/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) + }) +}) diff --git a/system-tests/projects/issue-8280-retain-video/cypress/e2e/spec2.cy.js b/system-tests/projects/issue-8280-retain-video/cypress/e2e/spec2.cy.js new file mode 100644 index 0000000000..1e54de87ca --- /dev/null +++ b/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) + }) +}) diff --git a/system-tests/test/issue_8280_spec.js b/system-tests/test/issue_8280_spec.js new file mode 100644 index 0000000000..edbccbfa76 --- /dev/null +++ b/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') + }) +})