Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent overwriting of video files #30673

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

YJDoc2
Copy link

@YJDoc2 YJDoc2 commented Nov 25, 2024

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 : Added system test
  • 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
Copy link

CLAassistant commented Nov 25, 2024

CLA assistant check
All committers have signed the CLA.

@cypress-app-bot
Copy link
Collaborator

@YJDoc2
Copy link
Author

YJDoc2 commented Nov 25, 2024

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
Copy link
Contributor

MikeMcC399 commented Nov 25, 2024

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

<!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->
## 13.16.1

The blank line should be removed.

Signed-off-by: Yashodhan Joshi <[email protected]>
@YJDoc2
Copy link
Author

YJDoc2 commented Nov 26, 2024

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

@jennifer-shehane
Copy link
Member

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

@jennifer-shehane jennifer-shehane self-requested a review December 2, 2024 15:47
@YJDoc2
Copy link
Author

YJDoc2 commented Dec 10, 2024

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
Copy link
Member

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

@AtofStryker AtofStryker self-requested a review December 10, 2024 15:20
@@ -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).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- 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).
- Fixed an issue where the configuration setting `trashAssetsBeforeRuns=false` was ignored for assets in the `videosfolder` and these assets were incorrectly deleted before running tests with `cypress run`. Addresses [#8280](https://github.com/cypress-io/cypress/issues/8280).
  1. Orient the Changelog text towards the configuration description in https://docs.cypress.io/app/references/configuration#Screenshots and use a similar style for the description as used in other Changelog entries
  2. Only list the issue ID, not the PR ID as well (see https://github.com/cypress-io/cypress/blob/develop/guides/writing-the-cypress-changelog.md#writing-guidelines).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trashAssetsBeforeRuns=false keeps all versions of screenshots but not of videos
5 participants