fix: prevent overwriting of video files

[YJDoc2]

• 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

• [x] Have tests been added/updated : Added system test
• [x] Has a PR for user-facing changes been opened in cypress-documentation: This is a bug fix, I don't think this is applicable,
• [x] Have API changes been updated in the type definitions : I don't think this is applicable.

[CLAassistant]

All committers have signed the CLA.

[cypress-app-bot]

• 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]

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]

@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]

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

[jennifer-shehane]

@YJDoc2 Thanks for the contribution. Can you write a test that verifies the change in behavior?

[YJDoc2]

Hey @jennifer-shehane , I have added a system test for this change. Also I synced the branch using develop-merge, if you want me to rebase instead, I'll do it and push. Also there is some code repetition in the test code, but because it is isolated and not reusable outside this, I have kept it. Let me know if you want me to refactor it as well.

Thanks :)

[jennifer-shehane]

@YJDoc2 Thanks! We'll give it a look over as soon as we can.

[YJDoc2]

Hey I'm facing some issues while syncing my branch with develop. There were some conflicts because develop has changed the screenshot.js to screenshot.ts , but even after fixing those, the pre-commit hook is failing eslint on files which are from the develop branch (i.e. I have only changed screenshot.ts file, and it is passing the eslint).

cc: @jennifer-shehane

[jennifer-shehane]

@YJDoc2 We haven't forgotten about this PR! We're working hard on getting Cypress 14 out first because we don't want to add any more scope to that release, so this will go in a release after that if approved.

We can take a look at the conflicts - indeed a lot changed.

[YJDoc2]

We're working hard on getting Cypress 14 out first because we don't want to add any more scope to that release, so this will go in a release after that if approved.

I didn't know that, makes sense to wait for release before adding this then.

We can take a look at the conflicts - indeed a lot changed.

Yep, I'm still interested in following this PR, so when the release is done and you all have bandwidth for this, ping me and we can figure out the problem together. I'll pause any update on this in the meantime, as because the pre-commit hook itself is failing, I cannot push any updates at all.

Thanks for your response and follow-up!

[jennifer-shehane]

I have the conflicts fixed in my branch, but I need to get these eslint fixes in first because the repo is in a weird place without that. https://github.com/cypress-io/cypress/pull/30776/commits/d9550adee8438aba90e9a3d6fe1225b40aa136de

[jennifer-shehane]

K, I've got a cleaner merge on this PR now

[jennifer-shehane]

@YJDoc2 There are some conflicts with your solution and our types. Could you take a look? You're providing a data object with many missing properties which makes the other code fail in many places.

Run yarn check-ts --concurrency=1

[YJDoc2]

Hey, thanks for following up!

@YJDoc2 There are some conflicts with your solution and our types. Could you take a look? You're providing a data object with many missing properties which makes the other code fail in many places.

Yes, I'll take a look and fix it 👍

[jennifer-shehane]

@YJDoc2 Hi, any intention to pick this back up?

[YJDoc2]

Hey @jennifer-shehane , yes. I got caught up in some other stuff so wasn't able to do this, but will try to get this done on this weekend. Thanks for the ping!

[YJDoc2]

Hey @jennifer-shehane , so I was taking a look at the lint fails, and I noticed that several of fails are not related to my original changes which I did in my first commit on this PR https://github.com/cypress-io/cypress/pull/30673/commits/fd492ce39f2f0c78d26509baf3dbbbf805f3e7f2 .

The main intention for this PR was to fix the video overwrite issue. If you take a look at my first commit, the way I did it was in the startVideoRecording function, in videoPath function , instead of just doing path.join I used getPath function which ensures that given path does not exist, and if it does appends (1) or such to create a new path. Along with there there were minor changes in packages/server/lib/modes/run.ts to accommodate this.

The other main change I had to do was move the getPath function from screenshot.jsto fs.ts so it can be used from a ts file. This was basically a cut-paste without any changes.

However, in the current state of the branch, where the screenshot.js was changed to screenshot.ts and so on, there are lint changes that erroring not related to my original changes. For example :

lib/automation/screenshot.ts:16:52 - error TS2345: Argument of type '{ size: never; takenAt: Date; dimensions: Pick<any, "width" | "height">; multipart: any; pixelRatio: any; name: string; specName: string | undefined; testFailure: boolean | undefined; path: string; }' is not assignable to parameter of type 'SavedDetails'.
  Types of property 'dimensions' are incompatible.
    Type 'Pick<any, "width" | "height">' is not assignable to type 'string'.
16           return screenshots.afterScreenshot(data, savedDetails)

This is from commit 5 years ago, I haven't changed this file.

lib/browsers/firefox.ts:638:11 - error TS2322: Type 'true' is not assignable to type 'string | number | undefined'.
638           'moz:debuggerAddress': true,

Same, haven't changed this file (this is also present in develop branch)

lib/screenshots.ts:195:43 - error TS18048: 'data.viewport' is possibly 'undefined'.
195   const pixelRatio = image.bitmap.width / data.viewport.width

This is the issue with the Data interface that was introduced, which does not have 100% correct type defs.

The thing is I'm only providing the Data object for getPath function, and I can be sure that everything that that function needs is there in the object, but aside from that , the rest of the code which I haven't changed also needs type-fixing after introducing the data-type.

I think fixing the Data type should be handled separately, what I think might be better would be that I close and open another PR on latest develop branch, and re-implement the changes I originally did here. At that point we can be fairly certain what changes I have done and the type issues that go with it v/s fixing the Data type in this branch itself.

wdyt?

[jennifer-shehane]

@YJDoc2 The getPath function was updated with types so that it is now erroring if required args are not passed to it. Since you are calling the getPath function without those required types, I would expect that call to be updated to be handled properly.

[YJDoc2]

Hey @jennifer-shehane , ok I'll update the function call on getPath to properly pass params and pass the type check.

[jennifer-shehane]

@YJDoc2 Do you still have time to address this?

[YJDoc2]

Hey @jennifer-shehane I have updated the call of getPath I had introduced to pass the param correctly, so that ts lint does not error on it anymore. Also fixed another lint by updating the type in the branch. Apart from that as I mentioned before, the lints which are failing in this branch are also failing in the develop branch.

Another thing to mention, the e2e tests I had added were passing when I added them, however now they are failing with ts related errors in the auth module. Do you know anything about it? The exact error is -

Cannot use import statement outside a module
/......../cypress/packages/server/lib/modes/index.ts:1
import { setCtx } from '@packages/data-context'
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at wrapSafe (node:internal/modules/cjs/loader:1385:21)

/....../cypress/packages/server/lib/makeDataContext.ts:1
import { DataContext, getCtx, clearCtx, setCtx } from '@packages/data-context'

/......../cypress/packages/server/lib/cloud/auth.ts:222
  .catch((err: Error) => {
             ^

Thanks :)

[jennifer-shehane]

@YJDoc2 We won't have time to dedicate to updating this PR at the moment as we're focused on other things. We'll likely need to close it if the issues can't be resolved.

[YJDoc2]

We'll likely need to close it if the issues can't be resolved.

Ok, I needed some help with the auth error in test ; but if you don't have available time to look at this, give me maybe till this weekend, I'll try to see if I can do something.

Apart from that, there is nothing blocking for this, right?

[YJDoc2]

Hey, is there any issue with snapshot builing on develop branch? I tried to clean the workspace and re-install everything, but the snapshot generation is failing . There is no specific error but the last step in log is metadata generation. Because snapshot generation is failing, I cannot run the tests to fix them. Please let me know if there is any know issue with snapshot generation .

[AtofStryker]

@YJDoc2 going to make a note to take a look at this tomorrow

[AtofStryker]

Hi @YJDoc2. Looks like mksnapshot wasn't happy with trying to due type imports in the same place as actual imports (I had to turn the error debug logs on to see it)

I broke this out into a separate line, updated the branch with develop, and moved the changelog to the next release. Let me know if you are still running into issues!

[YJDoc2]

@AtofStryker , thanks a lot of the fixes, they worked like a charm! Snapshot generation and my tests are both working now. Thanks a lot :)

@jennifer-shehane , so as of the latest commit,

• yarn check-ts --concurrency=1 is passing without any error on my system
• two tests I have added in the pr for testing the changes in this PR are passing

Apart from that, anything else I need to do? let me know. Thanks :)

[cypress-bot[bot]]

Released in 14.3.3.

This comment thread has been locked. If you are still experiencing this issue after upgrading to Cypress v14.3.3, please open a new issue.