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

[snapshot] Ability to record video #10121

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
@laullon

laullon commented Aug 23, 2017

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

This change add Snapshot to the ability to record videos of iOS applications, using the same method used to take screenshots.
There is a bug open for this: #8759
This was tested using:

  • Xcode 8.3
  • Xcode 9.0 (Beta 6)

Description

I have created a light/simple HTTP server that is used by SnapshotHelper.swift to send the startRecording and stopRecording commands to snapshot.rb that will execute\kill the xcrun simctl io booted recordVideo

laullon added some commits Aug 10, 2017

Replace a "file watcher" for a "simple http server" for the "Snapshot…
…Helper.swift" -> "snapshot.rb" communication to remove a dependency.

Feedback addressed @KrauseFx
@googlebot

This comment has been minimized.

googlebot commented Aug 23, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@googlebot googlebot added the cla: no label Aug 23, 2017

@laullon

This comment has been minimized.

laullon commented Aug 23, 2017

I signed it!

@googlebot

This comment has been minimized.

googlebot commented Aug 23, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Aug 23, 2017

@laullon

This comment has been minimized.

laullon commented Aug 23, 2017

I need to do some changes to apply recent changes on Snapshot

Back to work after '[snapshot] Add support for concurrent simulators …
…with Xcode 9 (#9570)'  change

(Only for xCode 8 for now)

@laullon laullon changed the title from Snapshot - Ability to record video to [snapshot] Ability to record video Aug 23, 2017

@laullon

This comment has been minimized.

laullon commented Aug 23, 2017

done, all is working now

@joshdholtz

This is super cool! Requested one change to add method doc on execute for new parameter.

Also, is there anyway to add tests for this? 😇

@@ -30,7 +30,7 @@ def which(cmd)
# @param prefix [Array] An array containing a prefix + block which might get applied to the output

This comment has been minimized.

@joshdholtz

joshdholtz Aug 23, 2017

Contributor

Can we get the pidCreated added to this comment/doc here? ^ 😇

This comment has been minimized.

@laullon

This comment has been minimized.

@laullon
@KrauseFx

This comment has been minimized.

Member

KrauseFx commented Sep 6, 2017

Hey @laullon, that's so awesome, thanks for your contribution, I love the idea to use snapshot to create videos. Could you tell us a little more about why we need a HTTP server to communicate? I thought it should be possible to do that without a web server.

@brunomunizaf

This comment has been minimized.

brunomunizaf commented Sep 7, 2017

I need this PR to be approved so much... Really appreciate the work @laullon !

@laullon

This comment has been minimized.

laullon commented Sep 7, 2017

I added the "HTTP Server" because I need a way to communicate the XCTests running on the simulator and the Snapshot process, so it knows when start and stop the recording. I did it that way because:

  1. Is very simple to implement on the Snapshot side, just a TCPServer listening on a random port (for security)
  2. On the simulator side is very simple to send the command, just a simple URLSession
  3. Supports multiples clients (which came handy for Xcode 9 multi simulator support)

This doesn't represent a security problem because this server only support 2 get URLs, any other URL or HTTP command is ignored, any HTTP header is also ignored, and it listen on a different port every time.

@KrauseFx

This comment has been minimized.

Member

KrauseFx commented Sep 7, 2017

Yeah, I'm not too worried about the security in this case, but about the complexity that this will add to the code base

@laullon

This comment has been minimized.

laullon commented Sep 11, 2017

yeah, "HTTP Server" sounds complex, but is really very simple...

Anyway, if you have a better way in mind let me know, I'll try it.

@ChristianSteffens

This comment has been minimized.

Contributor

ChristianSteffens commented Sep 19, 2017

This is a great addition to snapshot - especially since Apple is pushing towards the usage of videos in the AppStore.

What is the current position on this PR? Does is need some more testing or is the http server a problem? The http server might be handy for further additions / actions to control the simulator.

@KrauseFx

This comment has been minimized.

Member

KrauseFx commented Sep 19, 2017

Yeah, I thought it's possible using

xcrun simctl io booted recordVideo
@laullon

This comment has been minimized.

laullon commented Sep 19, 2017

I use "xcrun simctl io" to record the video, but I don't use the "booted" option because in Xcode 9 you can have multiple simulator running, so it had to called with a divide ID.

@KrauseFx

This comment has been minimized.

Member

KrauseFx commented Sep 19, 2017

Yeah, but that should still work, right? First, launch up a simulator, get its UDID, and then use that to record the video. For video recording in snapshot (or a new action/tool?), I feel like it would totally be okay to require Xcode 9 to use the feature 👍

@laullon

This comment has been minimized.

laullon commented Sep 19, 2017

That's what I do... and It works on Xcode 8 and 9

sendCommand(commnad: "stopRecording")
}
func sendCommand(commnad: String,args: String = "") {

This comment has been minimized.

@ffittschen

ffittschen Oct 1, 2017

Contributor

There is a typo in the attribute name: commnad should be command

This comment has been minimized.

@laullon
# @param error [Block] A block that's called if an error occurs
# @param prefix [Array] An array containing a prefix + block which might get applied to the output
# @param loading [String] A loading string that is shown before the first output
# @return [String] All the output as string
def execute(command: nil, print_all: false, print_command: true, error: nil, prefix: nil, loading: nil)
def execute(command: nil, print_all: false, print_command: true, pidCreated: nil, error: nil, prefix: nil, loading: nil)

This comment has been minimized.

@ffittschen

ffittschen Oct 1, 2017

Contributor

The other attributes of this function all use snake_case instead of camelCase. This should be consistent

This comment has been minimized.

@laullon
@ffittschen

This comment has been minimized.

Contributor

ffittschen commented Oct 2, 2017

I agree with @KrauseFx on this. There should be a simpler way to pass the command from Xcode to fastlane other than using a http server. Maybe it just has to try it from another direction: don't start to record from inside of the UITest, but start the relevant parts of the UITest from outside of Xcode (before/after starting to record).

I could imagine something like snapshot record_videos:true will internally look similar to this execution of scan

scan(only_testing: "TestBundleA/TestSuiteVideo")

directly followed this part of your code

Thread.new do
    FastlaneCore::CommandExecutor.execute(command: "xcrun simctl io booted recordVideo #{language_folder}/#{name}.mp4",
                                          print_all: true,
                                          print_command: true,
                                          loading: "Recording video...",
                                          pidCreated: proc do |pid|
                                            @recording_pid = pid
                                          end,
                                          error: proc do |output, return_code|
                                            ErrorHandler.handle_test_error(output, return_code)
                                            UI.error "Caught error... #{return_code}"
                                            UI.error "Caught output... #{output}"
                                          end) 
end

laullon added some commits Oct 23, 2017

Merge branch 'master' into snapshot-record-video
* master: (96 commits)
  [snapshot] Fix unused result warning in helper (#10662)
  Typo Fix (#10661)
  Version bump to 2.62.1 (#10670)
  Support manual signingStyle (#10508)
  Dont count lane switches in metrics (#10659)
  Add missing html tag (#10657)
  Deprecates add_id_info_limits_tracking in favor of add_id_info_uses_idfa (#10653)
  [fastlane core] fix unit tests when Xcode 9.0 not installed (#10655)
  Add automatic commiting of mkdocs.yml for docs generation (#10578)
  [Client] Handle Faraday::ParsingError as part of Client#with_retry logic (#10646)
  Revert "[BuildWatcher] Handle parse error while waiting for build processing (#10621)" (#10647)
  [BuildWatcher] Handle parse error while waiting for build processing (#10621)
  Remove 'zip' example from update_info_plist (#10494)
  Make plugin details optional (#10628)
  fix leaking sw_vers (#10630)
  Update snapshot instructions (#10626)
  Update update instructions of snapshot (#10627)
  Don't add unneeded new-line when asking for user input (#10629)
  Forward match keychain password into cert (#10619)
  SW_VERS should be lowercase (#10625)
  ...

# Conflicts:
#	README.md
#	cert/README.md
#	snapshot/README.md
#	snapshot/lib/assets/SnapshotHelper.swift
@ohayon

This comment has been minimized.

Member

ohayon commented Dec 4, 2017

Hey folks! I am just going through some older PRs and checking out the status of them. I wanted to bump this in case there was any more discussion to be had and see if we still thought this should get merged in. @laullon did you have a chance to consider the feedback of the http server? 🚀

@KrauseFx

This comment has been minimized.

Member

KrauseFx commented Dec 17, 2017

Thanks everyone, I'm gonna go ahead and close the PR for now. I think we should be able to implement video recording feature in snapshot without the need of setting up a local HTTP server 👍

Thanks for everyone's contribution on this, and sorry for having to close this PR at this time

@Jules2010

This comment has been minimized.

Jules2010 commented Feb 5, 2018

Any ideas how to get rid of the 1 second of black screen at the start of the video :(

Edit: this could be several seconds. Also it delayed my first view controller in the video. I’m pretty sure the snapshot erase simulator was the cause.

@laullon any chance you can merge in recent changes from master? I tried on another branch and couldn’t get it to work, I reckon your more familiar with your changes.

One suggestion I do have is that the success of the snapshot still requires a screenshot, also putting the video in the results page would be good.

However this all works for me, good job.

For anyone else reading, I was confused over the http listener stuff but this is all contained within the swift / fastlane, you don’t need to do anything in your app to use this.

@laullon

This comment has been minimized.

laullon commented Feb 5, 2018

I'm not working on this right now... don't have time to it...
But there is another PR in progress: #11744

@Jules2010

This comment has been minimized.

Jules2010 commented Feb 5, 2018

Yeah thats me :)

@laullon

This comment has been minimized.

laullon commented Feb 5, 2018

ohhh!!! right ;)

@Jules2010 Jules2010 referenced this pull request Feb 17, 2018

Open

[WIP] Snapshot record video #11744

4 of 4 tasks complete

@fastlane fastlane locked and limited conversation to collaborators Apr 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.