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

Allow video to be recorded till the end of the test #4804

Merged
merged 8 commits into from
Jul 29, 2019

Conversation

bahmutov
Copy link
Contributor

@bahmutov bahmutov commented Jul 24, 2019

Adds hardcoded delay of 1 second after the end of the test run before closing the browser if there is video recording going on. This allows the last frames to be added, creating the complete video.

Reviews

Trying something new

Before

the last frame of the video

Screen Shot 2019-07-24 at 10 54 58 AM

After

Screen Shot 2019-07-24 at 10 56 01 AM

Pre-merge Tasks

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

@bahmutov bahmutov requested review from brian-mann and kuceb July 24, 2019 15:39
kuceb
kuceb previously approved these changes Jul 24, 2019
@jennifer-shehane
Copy link
Member

eek. I know this likely solves the issue, but...hardcoded 1 sec wait is kind of the opposite of Cypress philosophy. This will slow down user's runs, especially those that have 100+ spec files. Would rather isolate the cause and somehow hook into knowing when the reporter is done rendering to stop the video there.

@bahmutov
Copy link
Contributor Author

yeah, imperfect, but good enough for now to fix the immediate problem. The video recording will probably change how we capture and end the video. 1-second addition per spec is a fair price I feel.

Copy link
Member

@brian-mann brian-mann left a comment

Choose a reason for hiding this comment

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

We should only delay the end of the video if we are actually recording a video

@bahmutov
Copy link
Contributor Author

@brian-mann in the file it checks if we are recording the video, if there is a better way


    .tap =>
      # ? is this the best way to determine if we are recording a video?
      # TODO rename "end" to something meaningful, like "videoCaptureEnd"
      return unless end

@brian-mann
Copy link
Member

Def agree with your comments on making this much clearer

@kuceb kuceb requested review from kuceb and removed request for kuceb July 28, 2019 01:47
…eral cleanup

- simplify promise implementation in videoCapture.start
- extract a few methods in runMode to handle conditional video capturing
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.

Video recorded is incomplete - last frames before tests are complete are not recorded
4 participants