[WIP] Snapshot record video #11744
[WIP] Snapshot record video #11744
Conversation
…Helper.swift" -> "snapshot.rb" communication to remove a dependency. Feedback addressed @KrauseFx
rubocop
…with Xcode 9 (fastlane#9570)' change (Only for xCode 8 for now)
* master: (96 commits) [snapshot] Fix unused result warning in helper (fastlane#10662) Typo Fix (fastlane#10661) Version bump to 2.62.1 (fastlane#10670) Support manual signingStyle (fastlane#10508) Dont count lane switches in metrics (fastlane#10659) Add missing html tag (fastlane#10657) Deprecates add_id_info_limits_tracking in favor of add_id_info_uses_idfa (fastlane#10653) [fastlane core] fix unit tests when Xcode 9.0 not installed (fastlane#10655) Add automatic commiting of mkdocs.yml for docs generation (fastlane#10578) [Client] Handle Faraday::ParsingError as part of Client#with_retry logic (fastlane#10646) Revert "[BuildWatcher] Handle parse error while waiting for build processing (fastlane#10621)" (fastlane#10647) [BuildWatcher] Handle parse error while waiting for build processing (fastlane#10621) Remove 'zip' example from update_info_plist (fastlane#10494) Make plugin details optional (fastlane#10628) fix leaking sw_vers (fastlane#10630) Update snapshot instructions (fastlane#10626) Update update instructions of snapshot (fastlane#10627) Don't add unneeded new-line when asking for user input (fastlane#10629) Forward match keychain password into cert (fastlane#10619) SW_VERS should be lowercase (fastlane#10625) ... # Conflicts: # README.md # cert/README.md # snapshot/README.md # snapshot/lib/assets/SnapshotHelper.swift
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
@laullon please will you sign the CLA again here please. I'm going to try and get this pushed through without the tcp listener. |
| # remove the '#' to clear all previously generated screenshots before creating new ones | ||
| # clear_previous_screenshots true | ||
|
|
||
| # Where should the resulting videos be stored? |
There was a problem hiding this comment.
this comment should take into account that by default no video will be create. So probably something like:
If you configure snapshot to generate of your tets, where should it be stored?
|
|
||
| # clear_previous_screenshots true # remove the '#' to clear all previously generated screenshots before creating new ones | ||
|
|
||
| # Choose which project/workspace to use |
There was a problem hiding this comment.
unrelated new feature or just new in SnapfileTemplate?
|
|
||
| require 'open3' | ||
|
|
||
| module Snapshot |
There was a problem hiding this comment.
FYI: When you rebase onto current master, this module will alredy exist in snapshot/module relative from here. (Just so you are aware and nobody is confused when reviewing - as I was)
There was a problem hiding this comment.
If not, and this is just a useless copy, one would have to make sure if there are no important changes here.
There was a problem hiding this comment.
Quick comparison shows that they are indeed identical. This is probably just an artifact (that has to be fixed before a merge).
| @@ -7,6 +7,7 @@ module Snapshot | |||
| class Options | |||
| def self.available_options | |||
| output_directory = (File.directory?("fastlane") ? "fastlane/screenshots" : "screenshots") | |||
| video_output_directory = (File.directory?("fastlane") ? "fastlane/videos" : "videos") | |||
There was a problem hiding this comment.
does this folder already exist by default?
| @@ -134,6 +137,39 @@ def execute(retries = 0, command: nil, language: nil, locale: nil, launch_args: | |||
| end) | |||
| end | |||
|
|
|||
| def start_recording(dir_name, device, name) | |||
There was a problem hiding this comment.
maybe add video to the method name to make it clearer what this is?
| def start_recording(dir_name, device, name) | ||
| device_udid = TestCommandGenerator.device_udid(device) | ||
| pid = @recording_pid[device_udid] | ||
| return unless pid.nil? |
There was a problem hiding this comment.
what exactly does this do and why?
| @@ -134,6 +137,39 @@ def execute(retries = 0, command: nil, language: nil, locale: nil, launch_args: | |||
| end) | |||
| end | |||
|
|
|||
| def start_recording(dir_name, device, name) | |||
| device_udid = TestCommandGenerator.device_udid(device) | |||
There was a problem hiding this comment.
extract into well named method as it is repeated below
| def stop_recording(device) | ||
| device_udid = TestCommandGenerator.device_udid(device) | ||
| pid = @recording_pid[device_udid] | ||
| return if pid.nil? |
There was a problem hiding this comment.
what exactly does this do and why?
| end | ||
| end | ||
|
|
||
| def stop_recording(device) |
There was a problem hiding this comment.
also add video to method name maybe?
| @@ -66,6 +67,8 @@ def take_screenshots_simultaneously | |||
| device_batches = launcher_config.devices.map { |d| [d] } | |||
| end | |||
|
|
|||
| @recording_pid = {} | |||
There was a problem hiding this comment.
should this also be in the _xcode_8 version of this file?
| stop_recording | ||
| end | ||
|
|
||
| def start_recording(dir_name, device_type, name) |
There was a problem hiding this comment.
if this is identical to above, can this be moved outside of these files somehow and unified?
| @@ -89,6 +89,7 @@ def execute(command: nil, language: nil, locale: nil, device_type: nil, launch_a | |||
| loading: "Loading...", | |||
| error: proc do |output, return_code| | |||
| ErrorHandler.handle_test_error(output, return_code) | |||
| stop_recording | |||
There was a problem hiding this comment.
should this be in the other, non _xcode_8 version of this file?
|
Hi @janpio I'd just like to mention that this pull request as it stands doesn't provide any additional functionality as it stands. It's based on PR #10121 I'm not 100% sure about your questions. However this pull request has the http listener code removed and therefore doesn't have any means of triggering the video recording. I started this PR at that point, in the hope that someone would step in and advise and contribute to it's completion. I understand that @laullon has moved on from his PR with other projects and doesn't have time to contribute further. For information purposes, I have created a new branch https://github.com/Jules2010/fastlane/tree/snapshot-record-video-with-audio which includes @laullon initial work and QuickTime Player to also record sound. I hope this can move forward. Jules. |
|
Yes, I got that, saw the other PR as well. I tried to understand what is already here and see if this is in an optimal state to solve the remaining problem - because I really like the possible functionality here! Using snapshot to record videos of the app would really be an awesome feature. Maybe @laullon can also take a look at my comments? |
|
@jules2010 How is this work coming along? Were you able to get it working for your project locally without the HTTP server? |
|
@getaaron unfortunately not, I didn’t write the code originally. But I was able to add sound via QuickTime and use ffmpeg to join the audio and video together. However my code isn’t pretty and has hardcoded paths. I did mention to @laullon on twitter but he obviously didn’t find the time to help further. |
|
If someone is looking for inspiration for a pull request without a http server, checkout my proof of concept: https://github.com/Lausbert/Snaptake |
|
@Lausbert does this add sound to the videos too? My hack uses QuickTime and merged them together. |
|
@Lausbert I really like this approach; what do you think about setting this up as a fastlane plugin? We could add a comment about it in the Snapfile and link to it in the docs. |
|
@jules2010 I haven't tested it so far, since all I needed to do in my specific case was to add a jingle later on with ffmpeg. @getaaron Sounds great, but what about the changes in snapshotHelper.swift? Did I miss anything or is it impossible to integrate them by adding a plugin? |
|
@Lausbert the simulator currently doesn’t provide sound. I filed a radar and was told it was a feature request. |
Checklist
bundle exec rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validMotivation and Context
This change add Snapshot to the ability to record videos of iOS applications, using the same method used to take screenshots.
Description
Added ability to record of video within the simulator