From 004df8f42ae9fe06ac08d64841811ca39b7b3f5d Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 8 Aug 2018 20:11:44 +0200 Subject: [PATCH 1/5] Switch to flick utility usage for real device screen recording --- lib/commands/recordscreen.js | 175 +++++++++++------------ test/unit/commands/recordscreen-specs.js | 141 ------------------ 2 files changed, 86 insertions(+), 230 deletions(-) delete mode 100644 test/unit/commands/recordscreen-specs.js diff --git a/lib/commands/recordscreen.js b/lib/commands/recordscreen.js index bad1cef30..62728c034 100644 --- a/lib/commands/recordscreen.js +++ b/lib/commands/recordscreen.js @@ -1,33 +1,24 @@ import _ from 'lodash'; -import { retryInterval, waitForCondition } from 'asyncbox'; -import B from 'bluebird'; +import { waitForCondition } from 'asyncbox'; import { util, fs, tempDir } from 'appium-support'; -import { exec } from 'teen_process'; +import { exec, SubProcess } from 'teen_process'; import log from '../logger'; import { getPidUsingPattern, encodeBase64OrUpload } from '../utils'; +import path from 'path'; let commands = {}; -const RETRY_PAUSE = 1000; const MAX_RECORDING_TIME_SEC = 60 * 10; const DEFAULT_RECORDING_TIME_SEC = 60 * 3; const PROCESS_SHUTDOWN_TIMEOUT_SEC = 5; -const REAL_DEVICE_BINARY = 'xrecord'; +const REAL_DEVICE_BINARY = 'flick'; const REAL_DEVICE_PGREP_PATTERN = (udid) => `${REAL_DEVICE_BINARY}.*${udid}`; const SIMULATOR_BINARY = 'xcrun'; const SIMULATOR_PGREP_PATTERN = (udid) => `simctl io ${udid} recordVideo`; const DEFAULT_EXT = '.mp4'; -async function extractCurrentRecordingPath (pid) { - const {output} = await exec('ps', ['o', 'command', '-p', pid]); - log.debug(`Got the following output from ps: ${output}`); - const pattern = new RegExp(/[\s="'](\/.*\.mp4)/); - const matches = pattern.exec(output); - return _.isEmpty(matches) ? null : _.last(matches); -} - -async function finishScreenCapture (pid) { +async function interruptScreenCapture (pid) { try { await exec('kill', ['-2', pid]); } catch (e) { @@ -48,6 +39,18 @@ async function finishScreenCapture (pid) { return true; } +async function finishRealDeviceScreenCapture (udid, dstPath) { + const args = [ + '-p', 'ios', + '-u', udid, + '-o', path.dirname(dstPath), + '-n', path.basename(dstPath), + ]; + log.info(`Stopping screen recording by executing ${REAL_DEVICE_BINARY} ` + + `with arguments ${JSON.stringify(args)}`); + await exec(REAL_DEVICE_BINARY, args); +} + async function uploadRecordedMedia (localFile, remotePath = null, uploadOptions = {}) { try { return await encodeBase64OrUpload(localFile, remotePath, uploadOptions); @@ -74,8 +77,8 @@ async function uploadRecordedMedia (localFile, remotePath = null, uploadOptions * @property {?string} videoType - The format of the screen capture to be recorded. * Available formats: "h264", "mp4" or "fmp4". Default is "mp4". * Only works for Simulator. - * @property {?string} videoQuality - The video encoding quality (low, medium, high, photo - defaults to medium). - * Only works for real devices. + * @property {?string|number} frameRate - The framerate (in seconds) when converting screenshots to video. Default: 1. + * Only works for real devices. * @property {?boolean} forceRestart - Whether to try to catch and upload/return the currently running screen recording * (`false`, the default setting) or ignore the result of it and start a new recording * immediately. @@ -85,7 +88,7 @@ async function uploadRecordedMedia (localFile, remotePath = null, uploadOptions /** * Record the display of devices running iOS Simulator since Xcode 8.3 or real devices since iOS 8 - * (xrecord utility is required: https://github.com/WPO-Foundation/xrecord). + * (flick utility is required: https://github.com/isonic1/flick). * It records screen activity to an MPEG-4 file. Audio is not recorded with the video file. * If screen recording has been already started then the command will stop it forcefully and start a new one. * The previously recorded video file will be deleted. @@ -96,13 +99,13 @@ async function uploadRecordedMedia (localFile, remotePath = null, uploadOptions * @throws {Error} If screen recording has failed to start. */ commands.startRecordingScreen = async function (options = {}) { - const {videoType, timeLimit=DEFAULT_RECORDING_TIME_SEC, videoQuality='medium', + const {videoType, timeLimit=DEFAULT_RECORDING_TIME_SEC, frameRate, forceRestart} = options; let result = ''; if (!forceRestart) { log.info(`Checking if there is/was a previous screen recording. ` + - `Set 'forceRestart' option to 'true' if you'd like to skip this step.`); + `Set 'forceRestart' option to 'true' if you'd like to skip this step.`); result = await this.stopRecordingScreen(options); } @@ -130,20 +133,16 @@ commands.startRecordingScreen = async function (options = {}) { if (this.isRealDevice()) { binaryName = REAL_DEVICE_BINARY; if (!await fs.which(binaryName)) { - log.errorAndThrow(`'${binaryName}' binary is not found in PATH. Make sure it is present on the system. ` + - `Check https://github.com/WPO-Foundation/xrecord for more details.`); + log.errorAndThrow(`'${binaryName}' binary is not found in PATH. Make sure it is installed: 'gem install flick'. ` + + `Check https://github.com/isonic1/flick for more details.`); } args = [ - '--quicktime', - '--id', this.opts.device.udid, - '--out', localPath, - `--force` + '-a', 'start', + '-u', this.opts.device.udid, + '-p', 'ios', ]; - if (util.hasValue(timeLimit)) { - args.push('--time', `${timeLimit}`); - } - if (util.hasValue(videoQuality)) { - args.push('--quality', `${videoQuality}`); + if (util.hasValue(frameRate)) { + args.push('-r', frameRate); } } else { binaryName = SIMULATOR_BINARY; @@ -159,56 +158,40 @@ commands.startRecordingScreen = async function (options = {}) { args.push(localPath); } - // wrap in a manual Promise so we can handle errors in exec operation - return await new B(async (resolve, reject) => { - let err = null; - let timeout = Math.floor(parseFloat(timeLimit) * 1000); - if (timeout > MAX_RECORDING_TIME_SEC * 1000 || timeout <= 0) { - return reject(new Error(`The timeLimit value must be in range (0, ${MAX_RECORDING_TIME_SEC}] seconds. ` + - `The value of ${timeLimit} has been passed instead.`)); - } - log.debug(`Beginning screen recording with command: '${binaryName} ${args.join(' ')}'` + - `Will timeout in ${timeout / 1000} s`); - if (this.isRealDevice()) { - // xrecord has its owen timer, so we only use this one as a safety precaution - // although simctl has no built-in timer and we have to be precise in such case - timeout += PROCESS_SHUTDOWN_TIMEOUT_SEC * 1000 * 2; + let timeout = Math.floor(parseFloat(timeLimit) * 1000); + if (timeout > MAX_RECORDING_TIME_SEC * 1000 || timeout <= 0) { + throw new Error(`The timeLimit value must be in range (0, ${MAX_RECORDING_TIME_SEC}] seconds. ` + + `The value of ${timeLimit} has been passed instead.`); + } + log.info(`Beginning screen recording with command: '${binaryName} ${args.join(' ')}'` + + `Will timeout in ${timeout / 1000} s`); + const proc = new SubProcess(binaryName, args); + await proc.start(0); + setTimeout(async () => { + if (_.isEmpty(this._recentScreenRecordingPath)) { + return; } - // do not await here, as the call runs in the background and we check for its product - exec(binaryName, args, {timeout, killSignal: 'SIGINT'}).catch((e) => { - err = e; - }); - // there is the delay time to start recording the screen for real devices, so, wait until it is ready. - // the ready condition is - // 1. check the movie file is created - // 2. check the screen capture has been started - // - // simctl keeps the file in an internal buffer instead and only creates it when the recording is done. - if (this.isRealDevice()) { - try { - await retryInterval(10, RETRY_PAUSE, async () => { - if (err) { - return; - } + const pgrepPattern = this.isRealDevice() ? REAL_DEVICE_PGREP_PATTERN : SIMULATOR_PGREP_PATTERN; + const pid = await getPidUsingPattern(pgrepPattern(this.opts.device.udid)); + if (_.isEmpty(pid)) { + return; + } - const {size} = await fs.stat(localPath); - if (size <= 32) { - throw new Error(`Remote file '${localPath}' found but it is still too small: ${size} bytes`); - } - }); - } catch (e) { - err = e; + try { + if (this.isRealDevice()) { + await finishRealDeviceScreenCapture(this.opts.device.udid, localPath); + } else { + await proc.stop('SIGINT'); } + } catch (err) { + log.warn(`Cannot stop the screen recording after ${timeout}ms timeout. ` + + `Original error: ${err.message}`); } + }, timeout); - if (err) { - log.error(`Error recording screen: ${err.message}`); - return reject(err); - } - this._recentScreenRecordingPath = localPath; - resolve(result); - }); + this._recentScreenRecordingPath = localPath; + return result; }; /** @@ -243,33 +226,47 @@ commands.stopRecordingScreen = async function (options = {}) { const pgrepPattern = this.isRealDevice() ? REAL_DEVICE_PGREP_PATTERN : SIMULATOR_PGREP_PATTERN; const pid = await getPidUsingPattern(pgrepPattern(this.opts.device.udid)); - let localPath = this._recentScreenRecordingPath; if (_.isEmpty(pid)) { - log.info(`Screen recording is not running. There is nothing to stop.`); + log.info('Screen recording is not running. There is nothing to stop.'); } else { - localPath = localPath || await extractCurrentRecordingPath(pid); try { - if (_.isEmpty(localPath)) { + if (_.isEmpty(this._recentScreenRecordingPath)) { log.errorAndThrow(`Cannot parse the path to the file created by ` + - `screen recorder process from 'ps' output. ` + - `Did you start screen recording before?`); + `screen recorder process. ` + + `Did you start screen recording not from Appium before?`); } } finally { - if (!await finishScreenCapture(pid)) { - log.warn(`Unable to stop screen recording. Continuing anyway`); + if (this.isRealDevice() && !_.isEmpty(this._recentScreenRecordingPath)) { + try { + await finishRealDeviceScreenCapture(this.opts.device.udid, this._recentScreenRecordingPath); + } catch (err) { + log.errorAndThrow(`Cannot stop the screen recording process. Original error: ${err.message}`); + } + } else { + if (!await interruptScreenCapture(pid)) { + log.warn(`Unable to stop screen recording. Continuing anyway`); + } } } } - let result = ''; - if (!_.isEmpty(localPath)) { - try { - result = await uploadRecordedMedia(localPath, remotePath, {user, pass, method}); - } finally { - this._recentScreenRecordingPath = null; + if (_.isEmpty(this._recentScreenRecordingPath)) { + return ''; + } + + try { + if (!await fs.exists(this._recentScreenRecordingPath)) { + log.errorAndThrow(`The screen recorder utility has failed ` + + `to store the actual screen recording at '${this._recentScreenRecordingPath}'`); } + return await uploadRecordedMedia(this._recentScreenRecordingPath, remotePath, { + user, + pass, + method + }); + } finally { + this._recentScreenRecordingPath = null; } - return result; }; diff --git a/test/unit/commands/recordscreen-specs.js b/test/unit/commands/recordscreen-specs.js deleted file mode 100644 index 3ad970942..000000000 --- a/test/unit/commands/recordscreen-specs.js +++ /dev/null @@ -1,141 +0,0 @@ -import chai from 'chai'; -import chaiAsPromised from 'chai-as-promised'; -import XCUITestDriver from '../../..'; -import { withMocks } from 'appium-test-support'; -import { fs, tempDir } from 'appium-support'; -import * as utils from '../../../lib/utils'; -import * as teen_process from 'teen_process'; -import sinon from 'sinon'; -import B from 'bluebird'; -import v8 from 'v8'; - - -chai.should(); -chai.use(chaiAsPromised); - -const driver = new XCUITestDriver(); -describe('basic', withMocks({driver, fs, tempDir, utils, teen_process}, function (mocks) { - const localFile = '/path/to/local.mp4'; - const mediaContent = Buffer.from('appium'); - const udid = '1234'; - driver.opts = { - device: { - udid - } - }; - - afterEach(function () { - mocks.verify(); - }); - - describe('startRecordingScreen', function () { - beforeEach(function () { - driver._recentScreenRecordingPath = null; - }); - - it('should call simctl to start screen recording on Simulator', async function () { - mocks.driver.expects('isRealDevice').atLeast(1).returns(false); - mocks.utils.expects('getPidUsingPattern') - .atLeast(1).withExactArgs(`simctl io ${udid} recordVideo`).returns(null); - mocks.teen_process.expects('exec').once().returns(new B(() => {})); - - await driver.startRecordingScreen(); - driver._recentScreenRecordingPath.should.not.be.empty; - }); - - it('should return previous capture before starting a new recording on Simulator', async function () { - const previousPath = '/some/video.mp4'; - - mocks.driver.expects('isRealDevice').atLeast(1).returns(false); - mocks.utils.expects('getPidUsingPattern') - .atLeast(1).withExactArgs(`simctl io ${udid} recordVideo`).returns(null); - mocks.teen_process.expects('exec').returns(new B(() => {})); - mocks.fs.expects('exists').once().withExactArgs(previousPath).returns(true); - mocks.fs.expects('stat').once().returns({size: 39571}); - mocks.fs.expects('readFile').once().withExactArgs(previousPath).returns(mediaContent); - mocks.fs.expects('rimraf').once().withExactArgs(previousPath); - mocks.tempDir.expects('path').once().returns(localFile); - - driver._recentScreenRecordingPath = previousPath; - (await driver.startRecordingScreen()) - .should.be.eql(mediaContent.toString('base64')); - driver._recentScreenRecordingPath.should.be.eql(localFile); - }); - - it('should call stat multiple times until size is big enough on a real device', async function () { - mocks.driver.expects('isRealDevice').atLeast(1).returns(true); - mocks.utils.expects('getPidUsingPattern') - .atLeast(1).withExactArgs(`xrecord.*${udid}`).returns(null); - mocks.fs.expects('which').once().withExactArgs('xrecord').returns('xrecord'); - mocks.teen_process.expects('exec').returns(new B(() => {})); - let fileSizeStub = sinon.stub(fs, 'stat'); - try { - fileSizeStub - .onCall(0) - .returns({size: 31}) - .onCall(1) - .returns({size: 42}); - - await driver.startRecordingScreen(); - } finally { - fileSizeStub.restore(); - } - }); - }); - - describe('stopRecordingScreen', function () { - beforeEach(function () { - mocks.driver.expects('isRealDevice').returns(false); - }); - - it('should kill the process and get the content of the created mp4 file using ps', async function () { - const pid = '1'; - driver._recentScreenRecordingPath = null; - mocks.utils.expects('getPidUsingPattern').atLeast(1).returns(pid); - mocks.teen_process.expects('exec').withExactArgs('ps', ['o', 'command', '-p', pid]).returns({output: ` - COMMAND - xcrun simctl io ${udid} recordVideo ${localFile} - `}); - mocks.teen_process.expects('exec').withExactArgs('kill', ['-2', pid]); - mocks.teen_process.expects('exec').withExactArgs('kill', ['-0', pid]).throws(); - mocks.fs.expects('exists').once().withExactArgs(localFile).returns(true); - mocks.fs.expects('readFile').once().withExactArgs(localFile).returns(mediaContent); - mocks.fs.expects('rimraf').once().withExactArgs(localFile); - mocks.fs.expects('stat').once().withExactArgs(localFile).returns({size: 100}); - - (await driver.stopRecordingScreen()).should.eql(mediaContent.toString('base64')); - }); - - it('should use the remembered file path if present', async function () { - const pid = '1'; - driver._recentScreenRecordingPath = localFile; - mocks.utils.expects('getPidUsingPattern').atLeast(1).returns(pid); - mocks.teen_process.expects('exec').withExactArgs('kill', ['-2', pid]); - mocks.teen_process.expects('exec').withExactArgs('kill', ['-0', pid]).throws(); - mocks.fs.expects('exists').once().withExactArgs(localFile).returns(true); - mocks.fs.expects('readFile').once().withExactArgs(localFile).returns(mediaContent); - mocks.fs.expects('rimraf').once().withExactArgs(localFile); - mocks.fs.expects('stat').once().withExactArgs(localFile).returns({size: 100}); - - (await driver.stopRecordingScreen()).should.eql(mediaContent.toString('base64')); - }); - - it('should fail if the recorded file is too large', async function () { - driver._recentScreenRecordingPath = localFile; - mocks.utils.expects('getPidUsingPattern').atLeast(1).returns(null); - mocks.fs.expects('exists').once().withExactArgs(localFile).returns(true); - mocks.fs.expects('rimraf').once().withExactArgs(localFile); - mocks.fs.expects('stat').once().withExactArgs(localFile) - .returns({size: v8.getHeapStatistics().total_available_size}); - - await driver.stopRecordingScreen().should.eventually.be.rejectedWith(/is too large/); - }); - - it('should return empty string if no recording processes are running', async function () { - driver._recentScreenRecordingPath = null; - mocks.utils.expects('getPidUsingPattern').atLeast(1).returns(null); - - (await driver.stopRecordingScreen()).should.eql(''); - }); - }); -})); From b84e239bbdb35bbe7cfe5e8d05e71664783e13b9 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 8 Aug 2018 20:21:00 +0200 Subject: [PATCH 2/5] Avoid throwing in finally block --- lib/commands/recordscreen.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/commands/recordscreen.js b/lib/commands/recordscreen.js index 62728c034..53bf8bea8 100644 --- a/lib/commands/recordscreen.js +++ b/lib/commands/recordscreen.js @@ -240,7 +240,7 @@ commands.stopRecordingScreen = async function (options = {}) { try { await finishRealDeviceScreenCapture(this.opts.device.udid, this._recentScreenRecordingPath); } catch (err) { - log.errorAndThrow(`Cannot stop the screen recording process. Original error: ${err.message}`); + log.warn(`Unable to stop screen recording. Continuing anyway. Original error: ${err.message}`); } } else { if (!await interruptScreenCapture(pid)) { From fcf45d350f1add0913f27555487191a6b34a8f6b Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 8 Aug 2018 20:22:45 +0200 Subject: [PATCH 3/5] Fix params --- lib/commands/recordscreen.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/commands/recordscreen.js b/lib/commands/recordscreen.js index 53bf8bea8..b1a689552 100644 --- a/lib/commands/recordscreen.js +++ b/lib/commands/recordscreen.js @@ -41,6 +41,8 @@ async function interruptScreenCapture (pid) { async function finishRealDeviceScreenCapture (udid, dstPath) { const args = [ + 'video', + '-a', 'stop', '-p', 'ios', '-u', udid, '-o', path.dirname(dstPath), @@ -137,9 +139,10 @@ commands.startRecordingScreen = async function (options = {}) { `Check https://github.com/isonic1/flick for more details.`); } args = [ + 'video', '-a', 'start', - '-u', this.opts.device.udid, '-p', 'ios', + '-u', this.opts.device.udid, ]; if (util.hasValue(frameRate)) { args.push('-r', frameRate); From f876248c258aed86faf1e14d4cd75f3ca9758c15 Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 8 Aug 2018 20:29:00 +0200 Subject: [PATCH 4/5] Use more effective running verification --- lib/commands/recordscreen.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/lib/commands/recordscreen.js b/lib/commands/recordscreen.js index b1a689552..1cd67bb6c 100644 --- a/lib/commands/recordscreen.js +++ b/lib/commands/recordscreen.js @@ -161,23 +161,17 @@ commands.startRecordingScreen = async function (options = {}) { args.push(localPath); } - let timeout = Math.floor(parseFloat(timeLimit) * 1000); + const timeout = Math.floor(parseFloat(timeLimit) * 1000); if (timeout > MAX_RECORDING_TIME_SEC * 1000 || timeout <= 0) { throw new Error(`The timeLimit value must be in range (0, ${MAX_RECORDING_TIME_SEC}] seconds. ` + `The value of ${timeLimit} has been passed instead.`); } log.info(`Beginning screen recording with command: '${binaryName} ${args.join(' ')}'` + `Will timeout in ${timeout / 1000} s`); - const proc = new SubProcess(binaryName, args); - await proc.start(0); + const recorderProc = new SubProcess(binaryName, args); + await recorderProc.start(0); setTimeout(async () => { - if (_.isEmpty(this._recentScreenRecordingPath)) { - return; - } - - const pgrepPattern = this.isRealDevice() ? REAL_DEVICE_PGREP_PATTERN : SIMULATOR_PGREP_PATTERN; - const pid = await getPidUsingPattern(pgrepPattern(this.opts.device.udid)); - if (_.isEmpty(pid)) { + if (_.isEmpty(this._recentScreenRecordingPath) || !recorderProc.isRunning) { return; } @@ -185,7 +179,7 @@ commands.startRecordingScreen = async function (options = {}) { if (this.isRealDevice()) { await finishRealDeviceScreenCapture(this.opts.device.udid, localPath); } else { - await proc.stop('SIGINT'); + await recorderProc.stop('SIGINT'); } } catch (err) { log.warn(`Cannot stop the screen recording after ${timeout}ms timeout. ` + From 27c66b291c6a9016ec69e993eb4e2d3be57fc28c Mon Sep 17 00:00:00 2001 From: Mykola Mokhnach Date: Wed, 8 Aug 2018 20:37:39 +0200 Subject: [PATCH 5/5] Fix which usage --- lib/commands/recordscreen.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/commands/recordscreen.js b/lib/commands/recordscreen.js index 1cd67bb6c..137afa374 100644 --- a/lib/commands/recordscreen.js +++ b/lib/commands/recordscreen.js @@ -134,7 +134,9 @@ commands.startRecordingScreen = async function (options = {}) { let args; if (this.isRealDevice()) { binaryName = REAL_DEVICE_BINARY; - if (!await fs.which(binaryName)) { + try { + await fs.which(binaryName); + } catch (err) { log.errorAndThrow(`'${binaryName}' binary is not found in PATH. Make sure it is installed: 'gem install flick'. ` + `Check https://github.com/isonic1/flick for more details.`); }