fix: prevent overwriting of video files

YJDoc2 commented 3 days ago

Closes https://github.com/cypress-io/cypress/issues/8280

Additional details

Why was this change necessary?

Currently multiple runs of cypress in same dir (with trashAssetsBeforeRuns=false) will keep the screenshots files across runs, but will overwrite the video file. For more details, please check the issue discussion.

What is affected by this change?

The videos dir will now retain existing video files if trashAssetsBeforeRuns=false is set

Any implementation details to explain?

Nothing much, I have moved the getPath function used for generating screenshot paths into fs.ts from screenshot.js and did some minor modifications to accommodate both screenshots and videos. There is one "hack" which I'm not sure how to fix, help appreciated :

The existing impl of getPath actually creates a file on the path to see if that has ENAMETOOLONG error.
Because of this, when getting path for compressed video file, it creates a file with -compressed suffix even if video compression is off.
For this I am removing the file manually in the video compression check, but is there any other way?

Steps to test

Use a simple cypress spec with screenshot and video recording turned on in the config. Have atleast one failing test in that.
Run cypress on it once. list all files in screenshots as well as videos
Run cypress again.
After the second run :

For current `develop` branch,

screenshot folder will have two copies of each failed test, one with name like spec1 -- testCase1 (failed).png and other with name spec1 -- testCase1 (failed) (1).png
Video folder will only have spec1.cy.js.mp4
For this branch,
screenshot folder will have two copies of each failed test, one with name like spec1 -- testCase1 (failed).png and other with name spec1 -- testCase1 (failed) (1).png
Video folder will have spec1.cy.js.mp4 and spec1.cy.js (1).mp4

How has the user experience changed?

They will keep the videos from previous runs if any.

PR Tasks

[ ] Have tests been added/updated : I need some help. Does this kind of change needs system-test, or can be done via unit test? Some help with where to add test is greatly appriciated.
[ ] Has a PR for user-facing changes been opened in cypress-documentation: This is a bug fix, I don't think this is applicable,
[ ] Have API changes been updated in the type definitions : I don't think this is applicable.

CLAassistant commented 3 days ago

All committers have signed the CLA.

cypress-app-bot commented 3 days ago

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.

YJDoc2 commented 3 days ago

Not sure why semantic CI is failing, I have added changelog entry. I don't think this will make to the next release so might have to update it later.

btw, this PR is ready for review.

MikeMcC399 commented 3 days ago

@YJDoc2

Not sure why semantic CI is failing, I have added changelog entry. I don't think this will make to the next release so might have to update it later.

The previous PR introduced an unwanted blank line 2 which is causing your failure.

https://github.com/cypress-io/cypress/blob/12df40ed8c1101c5c4053a1fe63c06fcd2809bc7/cli/CHANGELOG.md?plain=1#L1-L3

The blank line should be removed.

Edit: I suspect that more than just the blank line should be removed from the previous PR. I just commented into https://github.com/cypress-io/cypress/pull/30314#issuecomment-2499115282 about the state of the release from this other PR.

YJDoc2 commented 2 days ago

Hey @MikeMcC399 , thanks for the help! I have fixed the changelog and pushed.

cypress-io / cypress