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

Remove videoUploadOnPasses as a configuration option #26899

Closed
jennifer-shehane opened this issue May 31, 2023 · 7 comments
Closed

Remove videoUploadOnPasses as a configuration option #26899

jennifer-shehane opened this issue May 31, 2023 · 7 comments
Assignees
Labels
type: breaking change Requires a new major release version

Comments

@jennifer-shehane
Copy link
Member

What would you like?

Remove videoUploadOnPasses configuration option as a breaking change for all users.

Why is this needed?

This option itself only opens one extra configuration option, to skip compression and upload but save the video locally. Keeping the option introduces points of confusion for all users on what this option truly does and we expect it is used as a quick way to turn off compression for videos no one cares about.

This option is much less relevant with videoCompression being turned off for all users by default and videos being turned off for all users by default.

If someone wanted to skip compression/upload only for passing videos, they could delete the video moving forward. They could also copy the video elsewhere and delete the local video if they so desired to keep the video and also skip compression/upload. Overall, we think having code as configuration is better than a single, confusing config option.

Other

No response

@jennifer-shehane jennifer-shehane added the type: breaking change Requires a new major release version label May 31, 2023
@jennifer-shehane jennifer-shehane moved this from Understanding to Designing / Scoping in Cypress App Priorities May 31, 2023
@jennifer-shehane jennifer-shehane moved this from Designing / Scoping to Building in Cypress App Priorities May 31, 2023
@AtofStryker AtofStryker self-assigned this Jun 20, 2023
@jennifer-shehane
Copy link
Member Author

The code for this is done, but this has yet to be released. We'll update this issue and reference the changelog when it's released.

@github-project-automation github-project-automation bot moved this from Building to Generally Available in Cypress App Priorities Jul 5, 2023
@cdebro
Copy link

cdebro commented Aug 15, 2023

Why not just change the config option name to videoUploadOnlyOnFail, since that is what it does.

@jennifer-shehane
Copy link
Member Author

@cdebro Can you explain the use case for wanting to keep a passing video locally and preventing it from uploading to the Cloud? Where in this situation there is no extra time taken to upload to the Cloud and no storage charges incurred on the Cypress Cloud for uploaded videos.

@cdebro
Copy link

cdebro commented Aug 16, 2023

@jennifer-shehane - I think that the original post conflates a few ideas.
As I see it, there are three.

  1. Need to control video compression
  2. Need to control uploading failed videos only
  3. Case of what local storage is doing as compared to videos being pushed to the cloud

I don't think that the setting is intentionally used to toggle video compression. To me, video compression control should be an obvious setting that simply indicates that use case. e.g. videoCompression =

With that, the Cypress documentation directly indicates that using the --record flag affects uploading video to the cloud.
In that same paragraph, the flag that we are discussing here is described. It doesn't indicate anywhere in that paragraph that local storage is affected.

Here is that passage.
'When using the --record flag while running your tests, videos are processed, compressed, and uploaded to Cypress Cloud after every spec file runs, successful or not. To change this behavior to only process videos in the case that tests fail, set the videoUploadOnPasses configuration option to false.'

So, as indicated and tested, the setting is used to control the upload of video for failed tests only. It does not control the local video storage.

With that, bang for the buck comes from analyzing failed videos. In general, we don't look at passing test's videos. We do look at failing test videos as that have potential inform a solution regarding the failed state.

So, the goal is to clarify what this setting does and doesn't do in a way that retains the feature.

Cypress documentation:
'When using the --record flag while running your tests, videos are processed, compressed, and uploaded to [Cypress Cloud][edit] after every spec file runs, successful or not. '

Ok - that above part is just fine. No need to change.

Cypress documentation:
'To change this behavior to only process videos in the case that tests fail, set the videoUploadOnPasses configuration option to false.'

The above statement could be more clear. As written, it indicates that videos are only processed in that case that a test fails when set to false; meaning that both passed and failed tests are processed only in the case where a test fails. Clearly this is not the case.

Change the sentence such that it indicates that only failed test videos are processed and uploaded.
i.e. 'To change this behavior to only process and upload videos for failed test cases, set the videoUploadOnlyOnFail configuration option to true.'

Of course that above verbiage requires a logic change but it's worth it for the clarity.

Add to that sentence the following.
'This setting only affects uploaded videos. Local video storage is not affected.'

Finally, change the flag as previously mentioned, to that shown below so as to clearly indicate the behavior.
videoUploadOnlyOnFail

Control of local storage should be addressed separately and in a way that is easy and obvious to consume.

@cdebro
Copy link

cdebro commented Aug 28, 2023

@jennifer-shehane - any feedback on my comment?
#26899 (comment)

@jennifer-shehane
Copy link
Member Author

@cdebro I agree the documentation could have used more clarity upfront.

The videoUploadOnPasses configuration is being removed. If you have a use case for video that you need that cannot be met today I suggest opening a new issue describing the behavior you need Cypress to provide assuming this change will go in.

@jennifer-shehane
Copy link
Member Author

Released in Cypress 13.0.0.

@cypress-io cypress-io locked as resolved and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: breaking change Requires a new major release version
Projects
None yet
Development

No branches or pull requests

3 participants