Merge pull request #40 from jlipps/master

make appium+instruments connection more robust

app/appium.js

@@ -4,6 +4,7 @@
 var routing = require('./routing')
   , logger = require('../logger').get('appium')
   , UUID = require('uuid-js')
+  , _ = require('underscore')
   , ios = require('./ios');
 
 var Appium = function(args) {
@@ -16,6 +17,7 @@ var Appium = function(args) {
   this.active = null;
   this.device = null;
   this.sessionId = null;
+  this.desiredCapabilities = {};
   this.sessions = [];
   this.counter = -1;
   this.progress = -1;
@@ -32,7 +34,8 @@ Appium.prototype.attachTo = function(rest, cb) {
   }
 };
 
-Appium.prototype.start = function(cb) {
+Appium.prototype.start = function(desiredCaps, cb) {
+  this.desiredCapabilities = desiredCaps;
   this.sessions[++this.counter] = { sessionId: '', callback: cb };
   this.invoke();
 };
@@ -58,8 +61,24 @@ Appium.prototype.invoke = function() {
     this.device.start(function(err) {
       me.progress++;
       me.sessions[me.progress].sessionId = me.sessionId;
-      me.sessions[me.progress].callback(err);
-    });
+      me.sessions[me.progress].callback(err, me.device);
+    }, _.bind(me.onDeviceDie, me));
+  }
+};
+
+Appium.prototype.onDeviceDie = function(code, cb) {
+  var dyingSession = this.sessionId;
+  this.sessionId = null;
+  if (code !== null) {
+    this.devices = {};
+    this.device = null;
+  }
+  if (cb) {
+    if (this.active !== null) {
+      this.active = null;
+      this.invoke();
+    }
+    cb(null, {status: 0, value: null, sessionId: dyingSession});
   }
 };
 
@@ -68,29 +87,14 @@ Appium.prototype.stop = function(cb) {
     return;
   }
 
-  var dyingSession = this.sessionId
-  , me = this;
+  var me = this;
 
   logger.info('Shutting down appium session...');
   this.device.stop(function(code) {
-    me.sessionId = null;
-    if (code !== null) {
-      me.devices = {};
-    }
-    if (cb) {
-      if (me.active !== null) {
-        me.active = null;
-        me.invoke();
-      }
-      cb(null, {status: 0, value: null, sessionId: dyingSession});
-    }
+    me.onDeviceDie(code, cb);
   });
 };
 
-Appium.prototype.device = function() {
-  return this.devices[this.active];
-};
-
 module.exports = function(args) {
   return new Appium(args);
 };

app/controller.js

@@ -19,6 +19,17 @@ function getResponseHandler(req, res, validateResponse) {
   return responseHandler;
 }
 
+exports.sessionBeforeFilter = function(req, res, next) {
+  var match = new RegExp("([^/]+)").exec(req.params[0]);
+  var sessId = match ? match[1] : null;
+  // if we don't actually have a valid session, respond with an error
+  if (sessId && (!req.device || req.appium.sessionId != sessId)) {
+    res.send({sessionId: sessId, status: status.codes.NoSuchDriver, value: ''});
+  } else {
+    next();
+  }
+};
+
 exports.getStatus = function(req, res) {
   // Return a static JSON object to the client
   getResponseHandler(req, res)(null, {
@@ -30,13 +41,20 @@ exports.getStatus = function(req, res) {
 
 exports.createSession = function(req, res) {
   // we can talk to the device client from here
-  req.appium.start(function(err) {
+  var desired = req.body.desiredCapabilities;
+  req.appium.start(req.body.desiredCapabilities, function(err, instance) {
     if (err) {
       // of course we need to deal with err according to the WDJP spec.
       throw err;
     }
 
-    res.redirect("/wd/hub/session/" + req.appium.sessionId);
+    if (desired && desired.newCommandTimeout) {
+      instance.setCommandTimeout(desired.newCommandTimeout, function() {
+        res.redirect("/wd/hub/session/" + req.appium.sessionId);
+      });
+    } else {
+      res.redirect("/wd/hub/session/" + req.appium.sessionId);
+    }
   });
 };
 

app/ios.js

@@ -41,8 +41,11 @@ var IOS = function(rest, app, udid, verbose, removeTraceDir) {
   };
 };
 
-IOS.prototype.start = function(cb) {
+IOS.prototype.start = function(cb, onDie) {
   var me = this;
+  if (typeof onDie === "function") {
+    this.onStop = onDie;
+  }
 
   var onLaunch = function() {
     logger.info('Instruments launched. Starting poll loop for new commands.');
@@ -57,7 +60,7 @@ IOS.prototype.start = function(cb) {
         me.onStop(code);
       });
     } else {
-      me.onStop(code, traceDir);
+      me.onStop(code);
     }
   };
 
@@ -86,6 +89,11 @@ IOS.prototype.stop = function(cb) {
   me.progress = 0;
 };
 
+IOS.prototype.setCommandTimeout = function(secs, cb) {
+  var cmd = "waitForDataTimeout = " + parseInt(secs, 10);
+  this.proxy(cmd, cb);
+};
+
 IOS.prototype.proxy = function(command, cb) {
   // was thinking we should use a queue for commands instead of writing to a file
   this.push([command, cb]);

app/routing.js

@@ -11,6 +11,7 @@ module.exports = function(appium) {
 
   // Make appium available to all REST http requests.
   rest.all('/wd/*', inject);
+  rest.all('/wd/hub/session/*', controller.sessionBeforeFilter);
 
   rest.get('/wd/hub/status', controller.getStatus);
   rest.post('/wd/hub/session', controller.createSession);

app/test/unit/queue.js

@@ -76,7 +76,7 @@ describe('Appium', function() {
           return;
 
         mock(function() {
-          inst.start(function(err, device) {
+          inst.start({}, function(err, device) {
             var n = num;
             setTimeout(function() {
               inst.stop(function(sessionId) { doneYet(n); });

app/uiauto/bootstrap.js

@@ -15,13 +15,12 @@ target.setTimeout(1);
 
 // let server know we're alive and get first command
 var cmd = getFirstCommand();
-var noErrors = true;
 
 UIATarget.onAlert = function(){
   return true;
 };
 
-while(noErrors) {
+while(true) {
   if (cmd) {
     console.log("Got new command from instruments: " + cmd);
     var result = eval(cmd);
@@ -38,7 +37,6 @@ while(noErrors) {
     }
     cmd = sendResultAndGetNext(result);
   } else {
-    console.log("Error getting next command, shutting down :-(");
-    noErrors = false;
+    throw new Error("Error getting next command, shutting down :-(");
   }
 }

app/uiauto/lib/appiumutils.js

@@ -39,7 +39,7 @@ function getDeviceDetail() {
 /* Deactivating the app for specified duration in Seconds.
 Useful to test multi-taskig (moving app to background) */
 function deactivateApp(timeInSeconds){
- UIATarget.localTarget().deactivateAppForDuration(timeInSeconds);
+  $.backgroundApp(timeInSeconds);
 }
 
 UIAElementNil.prototype.type = function() {

app/uiauto/lib/instruments_client.js

@@ -3,14 +3,15 @@
 var system = UIATarget.localTarget().host();
 // clientPath is relative to where you run the appium server from
 var clientPath = 'instruments/client.js';
+var waitForDataTimeout = 60;
 
 var sendResultAndGetNext = function(result) {
   var args = [clientPath, '-s', '/tmp/instruments_sock'], res;
   if (typeof result !== "undefined") {
     args = args.concat(['-r', JSON.stringify(result)]);
   }
   try {
-    res = system.performTaskWithPathArgumentsTimeout('/usr/local/bin/node', args, 30);
+    res = system.performTaskWithPathArgumentsTimeout('/usr/local/bin/node', args, waitForDataTimeout);
   } catch(e) {
     console.log("Socket timed out waiting for a new command, why wasn't there one?");
     return null;

app/uiauto/lib/status.js

@@ -100,3 +100,4 @@ var codes = {
     summary: 'Target provided for a move action is out of bounds.'
   }
 };
+module.exports.codes = codes;

instruments/instruments.js

@@ -19,9 +19,7 @@ var Instruments = function(app, udid, bootstrap, template, sock, cb, exitCb) {
   this.resultHandler = this.defaultResultHandler;
   this.readyHandler = this.defaultReadyHandler;
   this.exitHandler = this.defaultExitHandler;
-  this.hasExited = false;
   this.hasConnected = false;
-  this.hasShutdown = false;
   this.traceDir = null;
   this.proc = null;
   this.debugMode = false;
@@ -89,7 +87,7 @@ Instruments.prototype.launch = function() {
   // prepare temp dir
   var tmpDir = '/tmp/' + this.guid;
   fs.mkdir(tmpDir, function(e) {
-    if(!e || (e && e.code === 'EEXIST')) {
+    if (!e || (e && e.code === 'EEXIST')) {
         self.proc = self.spawnInstruments(tmpDir);
         self.proc.stdout.on('data', function(data) {
           self.outputStreamHandler(data);
@@ -101,10 +99,9 @@ Instruments.prototype.launch = function() {
         self.proc.on('exit', function(code) {
           self.debug("Instruments exited with code " + code);
           self.exitCode = code;
-          self.hasExited = true;
-          self.doExit();
+          self.exitHandler(self.exitCode, self.traceDir);
         });
-    }else{
+    } else {
       throw e;
     }
   });
@@ -172,14 +169,10 @@ Instruments.prototype.sendCommand = function(cmd, cb) {
 
 Instruments.prototype.shutdown = function() {
   this.proc.kill();
-  this.hasShutdown = true;
-  this.doExit();
 };
 
 Instruments.prototype.doExit = function() {
-  if (this.hasShutdown && this.hasExited) {
-    this.exitHandler(this.exitCode, this.traceDir);
-  }
+  console.log("Calling exit handler");
 };
 
 
@@ -202,7 +195,7 @@ Instruments.prototype.outputStreamHandler = function(output) {
 };
 
 Instruments.prototype.errorStreamHandler = function(output) {
-  logger.error(("[INST STDERR] " + output).yellow);
+  logger.info(("[INST STDERR] " + output).yellow);
 };
 
 Instruments.prototype.lookForShutdownInfo = function(output) {

test/functional/appium.js

@@ -0,0 +1,26 @@
+/*global it:true*/
+"use strict";
+
+var should = require("should")
+    , describeWd = require("../helpers/driverblock.js").describe;
+
+describeWd('appium', function(h) {
+  it('should fail gracefully after timeout', function(done) {
+    var doSomething = function() {
+      h.driver.elementsByTagName('textField', function(err) {
+        should.exist(err);
+        done();
+      });
+    };
+    setTimeout(doSomething, 8000);
+  });
+}, undefined, undefined, undefined, {newCommandTimeout: 4});
+
+return describeWd('appium', function(h) {
+  it('should be available after previous timeout', function(done) {
+    h.driver.elementsByTagName('textField', function(err) {
+      should.not.exist(err);
+      done();
+    });
+  });
+}, undefined, undefined, undefined, {newCommandTimeout: 60});

test/helpers/driverblock.js

@@ -2,18 +2,21 @@
 "use strict";
 
 var wd = require('wd')
+  , _ = require("underscore")
   , defaultHost = '127.0.0.1'
   , defaultPort = 4723
   , defaultCaps = {
       browserName: 'iOS'
       , platform: 'Mac'
       , version: '6.0'
+      , newCommandTimeout: 60
     };
 
-var driverBlock = function(tests, host, port, caps) {
-  host = typeof host === "undefined" ? defaultHost : host;
-  port = typeof port === "undefined" ? defaultPort : port;
-  caps = typeof caps === "undefined" ? defaultCaps : caps;
+var driverBlock = function(tests, host, port, caps, extraCaps) {
+  host = typeof host === "undefined" ? _.clone(defaultHost) : host;
+  port = typeof port === "undefined" ? _.clone(defaultPort) : port;
+  caps = typeof caps === "undefined" ? _.clone(defaultCaps) : caps;
+  caps = _.extend(caps, typeof extraCaps === "undefined" ? {} : extraCaps);
   var driverHolder = {driver: null, sessionId: null};
 
   beforeEach(function(done) {
@@ -25,15 +28,20 @@ var driverBlock = function(tests, host, port, caps) {
   });
 
   afterEach(function(done) {
-    driverHolder.driver.quit(done);
+    driverHolder.driver.quit(function(err) {
+      if (err && err.status && err.status.code != 6) {
+        throw err;
+      }
+      done();
+    });
   });
 
   tests(driverHolder);
 };
 
-var describeWithDriver = function(desc, tests, host, port, caps) {
+var describeWithDriver = function(desc, tests, host, port, caps, extraCaps) {
   describe(desc, function() {
-    driverBlock(tests, host, port, caps);
+    driverBlock(tests, host, port, caps, extraCaps);
   });
 };