Skip to content

fix: starting cdp screencast when video:false #15985

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

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

kuceb
Copy link
Contributor

@kuceb kuceb commented Apr 14, 2021

This regression was introduced due to requirement for component testing to switch writeVideoFrame callbacks when the specs switch without closing the browser.

other questions about CT video recording

  • for CT, is a new video saved for each spec? Does the video compression happen between each spec while the browser is open?
  • when is the video uploaded when recording to the dashboard, after each spec?

User facing changelog

Bug fix: fixed issue causing decreased performance in chromium browsers due to requesting screencast frames when video is disabled

Additional details

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue or this PR been tagged with a release in ZenHub?

@kuceb kuceb requested a review from a team as a code owner April 14, 2021 15:16
@kuceb kuceb requested review from chrisbreiding and jennifer-shehane and removed request for a team April 14, 2021 15:16
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Apr 14, 2021

Thanks for taking the time to open a PR!

@kuceb kuceb changed the title fix starting cdp screencast when video:false fix: starting cdp screencast when video:false Apr 14, 2021
@cypress
Copy link

cypress bot commented Apr 14, 2021



Test summary

9489 0 111 3Flakiness 0


Run details

Project cypress
Status Passed
Commit 55155b7
Started Apr 14, 2021 3:36 PM
Ended Apr 14, 2021 3:48 PM
Duration 12:09 💡
OS Linux Debian - 10.8
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Contributor

@chrisbreiding chrisbreiding left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@brian-mann
Copy link
Member

CT should cut/slice the video for each spec and upload the video to the dashboard. The only difference between e2e + CT should be that the browser is not closed in between each spec.

Please make sure that's the case before this PR is merged.

Copy link
Contributor

@elevatebart elevatebart left a comment

Choose a reason for hiding this comment

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

I confirm it works for CT.
I am curious to understand what my first implementation did wrong.

@kuceb
Copy link
Contributor Author

kuceb commented Apr 16, 2021

@elevatebart we use the existence of writeVideoFrame as a boolean check whether or not to start video. So if it's falsey we know not to start cdp.

@kuceb kuceb merged commit 04e854e into develop Apr 16, 2021
tgriesser added a commit that referenced this pull request Apr 19, 2021
* develop:
  fix flaky e2e record passes test (#16043)
  chore: switch lolex to new name @sinonjs/fake-timers (#15595)
  add pwa example (#15970)
  fix starting cdp screencast when video:false (#15985)
  fix(deps): update dependency ansi_up to version 5.x 🌟 (#15440)
  fix: run-ct does not hang on windows anymore (#16022)
  docs: in vite-dev-server example don't require the config (#15866)
@emilyrohrbough emilyrohrbough deleted the fix-screencast-with-video-false branch August 1, 2024 13:51
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.

cdp startScreencast is sent when video:false
4 participants