Move to pinging WDA status for readiness #327

imurchie · 2016-12-19T19:23:36Z

We currently use the technique of looking at the ios logs (sim or device) and waiting for the marker that WDA writes on startup. This means we need to open and manage the logs, make sure we are looking at the right entries, etc. It is all fraught, as the issue tracker can attest.

This PR strips out all log handling, and instead pings the WDA server until it gets a valid status back, or times out. This is possible since (A) on simulators the WDA server is always on localhost, and (B) on real devices we use iproxy, which maps the request over USB, so we also end up talking to localhost. In neither case do we need to actual server url, which is the only information that is available in the logs but not the status.

Tests are faster, and locally work without issue.

Two new caps are introduced:

useNewWDA - forces uninstall of WDA before launch. This clears up some problems with hanging processes and "too many instances running" errors.
wdaLaunchTimeout - ms to wait trying to connect.

@mykola-mokhnach Can you try this out for your setup?

mykola-mokhnach · 2016-12-19T19:36:04Z

lib/webdriveragent.js

+      }
      await exec('/bin/bash', ['Scripts/bootstrap.sh', '-d'], {cwd: this.bootstrapPath});
+    } catch (err) {
+      log.warn('Carthage not found. Install using `brew install carthage`');


mykola-mokhnach · 2016-12-19T19:39:51Z

lib/webdriveragent.js

-        if (!agentUrl) {
-          log.errorAndThrow(new Error('No url detected from WebDriverAgent'));
+        try {
+          let {stdout} = await exec('curl', curlArgs);


Executing terminal commands is usually slower than using built-in instruments. How about sending HEAD http request with timeout to the WDA URL and checking the response code?

I agree, why don't we use request-promise instead of curl?

I couldn't get it to work when I was originally trying. It would always return a blank response, while I could use the terminal to curl to it no problem.

I'll give it another shot.

I was thinking about something similar to this example:
http://stackoverflow.com/questions/5922842/getting-http-headers-with-node-js

For reasons unknown, request-promise is working now. I must have done something wrong that I couldn't see before.

mykola-mokhnach · 2016-12-19T19:41:17Z

lib/webdriveragent.js

-      }
+      return (async () => {
+        try {
+          await this.iproxy.start(5000);


probably, we can move the timeout value to a separate constant?

mykola-mokhnach · 2016-12-19T19:41:55Z

lib/webdriveragent.js

+          resolve();
+        } catch (err) {
+          log.error(`Error starting iproxy: '${err.message}'`);
+          reject('Unable to start iproxy. Is it installed?');


can we show the expected iproxy path in the error message

mykola-mokhnach · 2016-12-19T19:43:39Z

lib/webdriveragent.js

-    this.url = url.parse(agentUrl);
-
-    this.url.hostname = 'localhost';
+    this.url = url.parse(`http://localhost:8100`);


can the URL be moved to a constant?

jlipps

+1 to all of @mykola-mokhnach's comments, and added my own. Great approach.

jlipps · 2016-12-19T19:54:19Z

lib/webdriveragent.js

-        if (!agentUrl) {
-          log.errorAndThrow(new Error('No url detected from WebDriverAgent'));
+        try {
+          let {stdout} = await exec('curl', curlArgs);


I agree, why don't we use request-promise instead of curl?

jlipps · 2016-12-19T19:55:05Z

lib/webdriveragent.js

+          resolve();
+        } catch (err) {
+          log.error(`Error starting iproxy: '${err.message}'`);
+          reject('Unable to start iproxy. Is it installed?');


probably a good practice to reject with an Error instead of a string?

jlipps · 2016-12-19T19:55:31Z

lib/webdriveragent.js

+        } catch (err) {
+          let msg = `Unable to start WebDriverAgent: ${err}`;
+          log.error(msg);
+          reject(msg);


probably a good idea to reject with an error instead of a string?

mykola-mokhnach · 2016-12-19T20:23:20Z

lib/webdriveragent.js

+      if (!await fs.hasAccess(`${this.bootstrapPath}/Carthage`)) {
+        log.debug('Running WebDriverAgent bootstrap script to install dependencies');
+      } else {
+        log.debug('Running WebDriverAgent bootstrap script to update dependencies');


do we really need to execute carthage again?

This, I think, should fix the errors with certain dependencies not being there. I'm happy to not do this, and make people manually do it when problems arise.

@imurchie is this only a problem when people are running appium from source and receive wda updates?

@jlipps No. I think it is mostly a problem of people upgrading on top of an older version. As in, just doing an npm install without npm uninstall.

Then it would be better to just ask them to do this action manually in an error message. Otherwise executing carthage for each WDA build may greatly slow down automated tests.

imurchie · 2016-12-20T12:57:31Z

All comments addressed in code.

mykola-mokhnach · 2016-12-20T13:10:05Z

README.md

 |`preventWDAAttachments`|Sets read only permissons to Attachments subfolder of WebDriverAgent root inside Xcode's DerivedData. This is necessary to prevent XCTest framework from creating tons of unnecessary screenshots and logs, which are impossible to shutdown using programming interfaces provided by Apple.|Setting the capability to `true` will set Posix permissions of the folder to `555` and `false` will reset them back to `755`|
 |`webDriverAgentUrl`|If provided, Appium will connect to an existing WebDriverAgent instance at this URL instead of starting a new one.|e.g., `http://localhost:8100`|
-
+|`useNewWDA`|If `true`, forces uninstall of any existing WebDriverAgent app on device. This can provide stability in some situations. Defaults to `false`.|e.g., `true`|


I think it might be useful to give at least one example of such situation

I'll work on documenting an example. I don't have the details of the problem right now.

Does this require manual git pull of webdriveragent source into the wedriveragent under the xcuittestdriver node module ?

No. It just uninstalls the app from the device on session creation.

mykola-mokhnach · 2016-12-20T13:11:12Z

lib/desired-caps.js

  },
  wdaLocalPort: {
-    isNumber: true
+    isNumber: true,


there are no trailing commas for other parameters. Do we follow any rule on this?

I don't think we have a rule, per se. And this one is not doing anything (since we won't be adding to the object).

mykola-mokhnach · 2016-12-20T13:11:58Z

lib/driver.js

+    try {
      await this.wda.launch(sessionId, realDevice);
+    } catch (err) {
+      if (tries > 2) throw err;


this magic number might be moved to constants

mykola-mokhnach · 2016-12-20T13:12:31Z

lib/webdriveragent.js

-const AGENT_STARTED_REGEX = /ServerURLHere->(.*)<-ServerURLHere/;
 const REAL_DEVICE_LOGGER_PATH = 'idevicesyslog';
 const WDA_BUNDLE_ID = 'com.apple.test.WebDriverAgentRunner-Runner';
+const WDA_LAUNCH_TIMEOUT = 60 * 1000;


mykola-mokhnach · 2016-12-20T13:14:29Z

lib/webdriveragent.js

-    await this.deviceLogs.start(startDetector);
-    log.info(`WebDriverAgent started at url '${agentUrl}'`);
+      });
+      let startupTime = Date.now() - startTime;


one recommends to use process.hrtime for such purpose instead of Date()

http://stackoverflow.com/questions/38097264/node-js-timing-process-hrtime-vs-new-date-gettime

shortly said - process.hrtime has been initially designed to measure intervals with maximum possible precision. And Date is a general class to represent calendar dates in apps.

jlipps

one small remark then LGTM

jlipps · 2016-12-21T18:27:22Z

lib/webdriveragent.js

+          let msg = `Unable to start WebDriverAgent: ${err}`;
+          log.error(msg);
+          reject(new Error(msg));
+          return;


not sure what this return is doing

Not a whole lot. There must have been something after the catch block at one point.

jlipps · 2016-12-21T23:19:40Z

(I resolved conflicts using the GitHub UI. Pretty cool.)

rkavalap · 2016-12-22T00:43:02Z

package.json

    "lodash": "^4.0.0",
    "node-simctl": "^3.1.0",
+    "request": "^2.79.0",
+    "request-promise": "^2.0.1",


@imurchie request-promise is at version 4.1.1

Weird. That was just the result of npm -S request-promise, so it should have gotten the latest.

imurchie · 2016-12-22T12:32:16Z

@jlipps: So, the GitHub UI conflict resolver isn't so good. Alas.

imurchie mentioned this pull request Dec 19, 2016

Possible solution for multiple issues about idevicedate usage #326

Closed

mykola-mokhnach reviewed Dec 19, 2016

View reviewed changes

jlipps requested changes Dec 19, 2016

View reviewed changes

mykola-mokhnach reviewed Dec 19, 2016

View reviewed changes

imurchie force-pushed the isaac-real-device branch from f5754cc to 38026f8 Compare December 20, 2016 12:57

mykola-mokhnach reviewed Dec 20, 2016

View reviewed changes

mykola-mokhnach requested changes Dec 20, 2016

View reviewed changes

imurchie force-pushed the isaac-real-device branch from 38026f8 to 66f25a1 Compare December 20, 2016 14:45

jlipps reviewed Dec 21, 2016

View reviewed changes

imurchie force-pushed the isaac-real-device branch from 66f25a1 to e740021 Compare December 21, 2016 18:36

jlipps approved these changes Dec 21, 2016

View reviewed changes

rkavalap reviewed Dec 22, 2016

View reviewed changes

Move to pinging WDA for readiness

c0b48dd

imurchie force-pushed the isaac-real-device branch from ffc7f99 to c0b48dd Compare December 22, 2016 12:26

imurchie merged commit 95886f1 into master Dec 22, 2016

imurchie deleted the isaac-real-device branch December 22, 2016 13:15

Uh oh!

Move to pinging WDA status for readiness #327

Move to pinging WDA status for readiness #327

Uh oh!

Conversation

imurchie commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mykola-mokhnach Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlipps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imurchie commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlipps left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlipps commented Dec 21, 2016

mykola-mokhnach Dec 19, 2016 •

edited

Loading