add -image locator strategy and related logic #235

jlipps · 2018-07-15T17:58:02Z

First stab at putting the find by image logic into base driver.

I know there's no tests and docstrings, will add those. Just wanted to get the ideas here for any initial comments.

I haven't yet done extensive testing, but the happy path works for me locally.

mykola-mokhnach · 2018-07-15T17:58:42Z

lib/basedriver/commands/find.js

@@ -1,8 +1,17 @@
 import log from '../logger';
+import Jimp from 'jimp';
+import Promise from 'bluebird';


We usually call it B

mykola-mokhnach · 2018-07-15T17:59:25Z

lib/basedriver/commands/find.js

+import Jimp from 'jimp';
+import Promise from 'bluebird';
+import { errors } from '../../..';
+import { Buffer } from 'buffer';


should this be imported explicitly?

mykola-mokhnach · 2018-07-15T19:06:00Z

lib/basedriver/commands/find.js

+    // findElement(s)FromElement), this doesn't make sense, so use of the
+    // -image locator strategy won't pass the strategy validation filter as is
+    // desired
+    return await this.findByImage(selector);


I would say there's still a difference, because in case of multiple elements we need to return an empty array or throw an exception if only one element is expected

But in case of multiple elements (or anything other than strict findElement) it will throw an error saying there is no such strategy (https://github.com/appium/appium-base-driver/blob/master/lib/basedriver/commands/find.js#L19).

mykola-mokhnach · 2018-07-15T19:06:25Z

lib/basedriver/commands/find.js

@@ -47,6 +65,120 @@ commands.findElementsFromElement = async function (strategy, selector, elementId
  return await this.findElOrElsWithProcessing(strategy, selector, true, elementId);
 };

+helpers.findByImage = async function (b64Template, {checkingStaleness = false} = {}) {


-> shouldCheckStaleness

mykola-mokhnach · 2018-07-15T19:07:21Z

lib/basedriver/commands/find.js

+helpers.findByImage = async function (b64Template, {checkingStaleness = false} = {}) {
+  let threshold = DEFAULT_MATCH_THRESHOLD;
+  if (this.settings) {
+    threshold = this.settings.getSettings().imageMatchThreshold;


shouldn't we only assign it if imageMatchThreshold is defined?

mykola-mokhnach · 2018-07-15T19:11:16Z

lib/basedriver/commands/find.js

+  // someone might have sent in a template that's larger than the screen
+  // dimensions. If so let's check and cut it down to size since the algorithm
+  // will not work unless we do
+  b64Template = await this.ensureTemplateSize(b64Template, screenWidth, screenHeight);


I would rather skip this check and let the client to be responsible for template size. It might be too expensive to call getWindowSize each time the lookup is executed.

mykola-mokhnach · 2018-07-15T19:17:09Z

lib/basedriver/commands/find.js

+  // are two potential types of mismatch: aspect ratio mismatch and scale
+  // mismatch. we need to detect and fix both
+
+  const screenAR = screenWidth / screenHeight;


screen aspect ration is not going to be changed during the session, so it might be safe to cache it

i think it could be changed during session if the device orientation is changed

mykola-mokhnach · 2018-07-15T19:19:07Z

lib/basedriver/driver.js

@@ -276,6 +280,11 @@ class BaseDriver extends Protocol {
        // have an unexpected shutdown event, for example, we can cancel it from
        // outside, rejecting the current command immediately
        this.curCommandCancellable = new B((r) => { r(); }).then(() => { // eslint-disable-line promise/prefer-await-to-then
+          // if one of the args is an image element, handle it separately
+          const imgElId = getImgElFromArgs(args);


this looks a bit hacky

hmm, it was the only way I could think of to pass control to ImageElement in the case of an image element existing in the arguments. I agree it's not super pretty. Any other ideas?

mykola-mokhnach · 2018-07-15T19:22:52Z

lib/basedriver/image-element.js

+      throw new errors.StaleElementReferenceError();
+    }
+
+    if (!this.equals(newImgEl)) {


I'm not quite sure if click should mutate the element

mykola-mokhnach · 2018-07-15T19:24:16Z

lib/basedriver/image-element.js

+      this.rect = _.clone(newImgEl.rect);
+    }
+
+    // now set up the W3C touch action to click on the image by position


is this going to work for all the drivers? I'd rather add a fallback to MJSONWP

mykola-mokhnach · 2018-07-15T19:24:51Z

lib/basedriver/image-element.js

+    const imgEl = driver._imgElCache.get(imgElId);
+
+    switch (cmd) {
+      case "click":


single quotes

mykola-mokhnach · 2018-07-15T19:26:30Z

lib/basedriver/image-element.js

+  }
+}
+
+class ImageElementCache {


shall we use LRU cache implementation from a lib instead, for example https://www.npmjs.com/package/lru-cache?

mykola-mokhnach · 2018-07-15T19:27:44Z

lib/protocol/protocol.js

@@ -436,6 +437,12 @@ function driverShouldDoJwpProxy (driver, req, command) {
    }
  }

+  // if it looks like we have an image element in the body, never proxy
+  const stringBody = JSON.stringify(req.body);
+  if (stringBody && stringBody.indexOf(IMAGE_ELEMENT_PREFIX) !== -1) {


mykola-mokhnach · 2018-07-15T19:36:15Z

lib/basedriver/commands/find.js

+    return b64Screenshot;
+  }
+
+  // otherwise, if they don't match, it could spell problems for the accuracy


I would rather make this configurable from settings to make it as performant as possible by default and only do these performance-hitting transformations if explicitly asked by the client

jlipps · 2018-07-19T04:54:42Z

(note that this PR now depends on appium/appium-support#77)

mykola-mokhnach · 2018-07-19T06:11:16Z

lib/basedriver/commands/find.js

@@ -111,7 +110,8 @@ helpers.findByImage = async function (b64Template, {checkingStaleness = false} =
 };

 helpers.ensureTemplateSize = async function (b64Template, screenWidth, screenHeight) {
-  let {width: tplWidth, height: tplHeight, imgObj} = await getJimpData(b64Template);
+  let imgObj = await imageUtils.getJimpData(b64Template);


is the function called getJimpImage or getJimpData ?

oops, fixed.

mykola-mokhnach · 2018-07-19T18:20:43Z

lib/basedriver/commands/find.js

-  }
+/**
+ * @typedef {Object} FindByImageOptions
+ * @property {boolean} shouldCheckStaleness - whether this call to find an


can we also mention the default values for these properties or if any of them is required to set?

mykola-mokhnach · 2018-07-19T18:21:38Z

lib/basedriver/commands/find.js

+helpers.findByImage = async function (b64Template, {
+  shouldCheckStaleness = false,
+  multiple = false,
+} = {}) {


I assume = {} is redundant

mykola-mokhnach · 2018-07-19T18:22:16Z

lib/basedriver/commands/find.js

+  shouldCheckStaleness = false,
+  multiple = false,
+} = {}) {
+  const threshold = this.settings.getSettings().imageMatchThreshold;


does getSettings return a promise?

const { imageMatchThreshold: threshold, fixImageTemplateSize } = this.settings.getSettings();

does getSettings return a promise?

no

mykola-mokhnach · 2018-07-19T18:24:59Z

lib/basedriver/commands/find.js

+    // handle the element not found response below. In the case where get some
+    // _other_ kind of error, it means something blew up totally apart from the
+    // implicit wait timeout. We should not mask that error and instead throw
+    // it straightaway
    if (!err.match(/Condition unmet/)) {


should me match err.message instead?

mykola-mokhnach · 2018-07-19T18:29:23Z

lib/basedriver/device-settings.js

+
+  // which method to use for tapping by coordinate for image elements. the
+  // options are 'w3c' or 'mjsonwp'
+  imageElementTapStrategy: 'w3c',


shouldn't this be detected automatically?

how can it be detected automatically? we don't know which functionality the downstream driver supports. i.e., this doesn't (and shouldn't) correspond to the protocol used. it corresponds to the functionality available in the driver.

we know in which mode the downstream driver is running (w3c/jsonwp), don't we?

I'm not sure that we do. And anyway the mode doesn't matter: all we're doing is calling driver.performActions or driver.performTouch. There's no protocol involved. I'm just giving access to these methods protocol-related names because that's how they're known. But maybe strings like w3cAction and touchAction would be better.

But your comment makes me realize that this will not be proxied so in the case of WDA, we'll have to ensure the xcuitest driver has performActions on it, and manually proxy the command forward to WDA. That's not ideal.

mykola-mokhnach · 2018-07-19T18:34:28Z

lib/protocol/protocol.js

@@ -439,7 +439,7 @@ function driverShouldDoJwpProxy (driver, req, command) {

  // if it looks like we have an image element in the body, never proxy
  const stringBody = JSON.stringify(req.body);


I'm still not quite sure about that. What if any other valid request includes accidentally includes '-image' substring in its body?

it would have to include appium-image-element-. but you are right it is a risk. for a similar reason the webdriver spec does the whole element-6066-11e4-a52e-4f735466cecf thing.

one alternative is actually walking through the body recursively and seeing if we have an object with the exact form of an element. that seemed like it might be overkill. @imurchie what do you think?

having such suffix might be useful IMHO

mykola-mokhnach · 2018-07-19T18:35:10Z

lib/basedriver/image-element.js

+        actions: [
+          {type: 'pointerMove', x, y},
+          {type: 'pointerDown'},
+          {type: 'pause', duration: 500},


500 is the duration of a long tap. single tap duration is about 100-125 ms

imurchie · 2018-07-19T16:46:58Z

lib/basedriver/commands/find.js

+  }
+
+  if (!rect) {
+    throw new errors.NoSuchElementError(); // eslint-disable-line no-unsafe-finally


I presume this eslint directive is leftover from this line being in a finally block.

imurchie · 2018-07-19T16:55:33Z

lib/basedriver/driver.js

@@ -276,6 +280,11 @@ class BaseDriver extends Protocol {
        // have an unexpected shutdown event, for example, we can cancel it from
        // outside, rejecting the current command immediately
        this.curCommandCancellable = new B((r) => { r(); }).then(() => { // eslint-disable-line promise/prefer-await-to-then


I know this is existing, but can we change new B((r) -> { r(); } to B.resolve() (http://bluebirdjs.com/docs/api/promise.resolve.html)?

…mage element

jlipps · 2018-07-25T04:59:05Z

ok i've fixed all the issues reported so far and written all the unit tests that need writing. i will move on to writing functional tests in the xcui and uiauto2 drivers, so let's wait to merge this until i've at least run them locally. meanwhile i added a few significant commits since the last review so might want to have another look at it.

mykola-mokhnach · 2018-07-25T05:00:17Z

lib/protocol/protocol.js

+const IMAGE_ELEMENT_PREFIX = 'appium-image-element-';
+
+const IMG_EL_BODY_RE = new RegExp(
+  `"(${W3C_ELEMENT_KEY}|${MJSONWP_ELEMENT_KEY})": ?` +


perhaps ? -> \s*

good idea, will update

mykola-mokhnach · 2018-07-25T05:02:59Z

lib/basedriver/driver.js

@@ -408,6 +423,31 @@ class BaseDriver extends Protocol {
    return false;
  }

+  proxyRouteIsAvoided (sessionId, method, url) {
+    for (let avoidSchema of this.getProxyAvoidList(sessionId)) {
+      if (!_.isArray(avoidSchema) || avoidSchema.length !== 2) {


this will throw inside avoidSchema.length if avoidSchema is null

existing code (moved from protocol) but good point, i'll fix

actually, this is OK. if avoidSchema is not an array, the condition will short-circuit since the LHS is true, and avoidSchema.length will never be evaluated

mykola-mokhnach · 2018-07-25T05:03:41Z

lib/basedriver/driver.js

+      if (!_.includes(['GET', 'POST', 'DELETE'], avoidMethod)) {
+        throw new Error(`Unrecognized proxy avoidance method '${avoidMethod}'`);
+      }
+      if (!(avoidPathRegex instanceof RegExp)) {


perhaps _.isRegExp ?

yep, existing code but will fix

mykola-mokhnach · 2018-07-25T05:05:24Z

lib/basedriver/driver.js

+    return false;
+  }
+
+  async proxyCommand (/*url, method, body*/) {


why do we need to implement it in each driver?

each driver does proxying differently (uiauto2 calls this.uiautomator2.jwproxy for example, which isn't the case with xcui)

cannot we just call click or performActions? Why the base driver should care about proxying?

what if a driver doesn't implement performActions itself, but merely proxies requests for it to a downstream driver? for example, if uiauto2 server implemented the /actions endpoint, but uiauto2 driver did not. this is a totally valid case. we have many endpoints which are not specified explicitly in a driver and instead we rely on proxying to access the endpoint on a downstream driver.

does that make sense? maybe i'm confused but i think it's a real issue (at least in principle)

I wouldn't try to outsmart drivers. If some endpoint is not implemented there and it exists in the downstream driver then it would be better and more practical to implement the missing stuff directly there. Adding new requirements to base driver creates unnecessary complexity for the whole infrastructure.

ok. i think you have a good point. the way i am doing it now we will also require drivers to implement the proxyCommand method. so it's not less onerous than requiring them to simply implement performActions and performTouch. i'll do the same thing as i have done elsewhere and simply throw a meaningful error if these methods do not exist on the driver.

if the driver typically proxies the action/touch commands, then it will need to basically implement a one-line function to support image elements, like:

commands.performActions = function (actions) { this.proxyCommand('POST', '/actions', actions); };

In most cases this will be bypassed, but in the case of ImageElement actions, it will be called.

…up themselves to support image elements

jlipps · 2018-07-25T21:23:07Z

ok, tests have been written for xcuitest driver and uiauto2 driver (those PRs are up but won't pass until this is merged). from my perspective this is solid (at least for a beta, presumably we'll find more bugs or important behavior suggestions when it's in the wild).

jlipps · 2018-07-26T03:57:09Z

will also work on documentation, btw.

… can utilize this information

jlipps · 2018-07-27T17:07:38Z

the one build failure is a travis timeout, so I think this is good to go. Any other thoughts @imurchie @mykola-mokhnach? Otherwise I'm ready to merge.

mykola-mokhnach · 2018-07-27T17:10:48Z

lib/protocol/protocol.js

-    if (avoidMethod === req.method && avoidPathRegex.test(normalizedUrl)) {
-      return false;
-    }
+  if (driver.proxyRouteIsAvoided(req.params.sessionId, req.method, req.originalUrl)) {


it might be useful to add a docstring to this new method

jlipps · 2018-07-30T02:15:54Z

@mykola-mokhnach ok docstring added. let me know if you're happy with things as they stand now.

jlipps · 2018-07-30T15:57:26Z

@imurchie how about you?

jlipps · 2018-07-30T20:25:01Z

published in [email protected]

jlipps requested review from dpgraham, imurchie and mykola-mokhnach July 15, 2018 17:58

mykola-mokhnach reviewed Jul 15, 2018

View reviewed changes

jlipps mentioned this pull request Jul 15, 2018

Proposal for near-real-time image element interaction appium/appium#11021

Closed

mykola-mokhnach reviewed Jul 15, 2018

View reviewed changes

mykola-mokhnach reviewed Jul 19, 2018

View reviewed changes

jlipps force-pushed the jlipps-image-element branch from 6fd3908 to 44b1a06 Compare July 19, 2018 06:19

mykola-mokhnach reviewed Jul 19, 2018

View reviewed changes

imurchie reviewed Jul 20, 2018

View reviewed changes

KazuCocoa mentioned this pull request Jul 21, 2018

Add -image strategy appium/ruby_lib_core#112

Closed

3 tasks

jlipps added 2 commits July 24, 2018 21:55

tighten up logic for deciding whether to proxy based on presence of i…

ae20e84

…mage element

add find by image specs and fix issues that arose in the process

353c789

jlipps force-pushed the jlipps-image-element branch from 981fcd4 to 353c789 Compare July 25, 2018 04:56

mykola-mokhnach reviewed Jul 25, 2018

View reviewed changes

jlipps added 2 commits July 25, 2018 09:25

clean up regexes

587d8c7

don't try to be smart and proxy actions for drivers; let them set it …

2066cdb

…up themselves to support image elements

jlipps mentioned this pull request Jul 25, 2018

write functional tests for finding elements by images appium/appium-xcuitest-driver#730

Closed

change default match threshold based on experimentation

1c57023

jlipps mentioned this pull request Jul 25, 2018

fix settings proxying logic and add e2e tests for image element finding appium/appium-uiautomator2-driver#196

Merged

jlipps added 2 commits July 26, 2018 09:53

export which settings are handled by basedriver so downstream drivers…

839cea5

… can utilize this information

make sure to include a duration for the initial pointerMove action

067269c

mykola-mokhnach reviewed Jul 27, 2018

View reviewed changes

add docstring to proxyRouteIsAvoided

6c4b245

imurchie approved these changes Jul 30, 2018

View reviewed changes

jlipps merged commit ec72948 into master Jul 30, 2018

jlipps deleted the jlipps-image-element branch July 30, 2018 20:17

mykola-mokhnach mentioned this pull request Aug 3, 2018

Add support for "-image" locator type appium/java-client#990

Merged

4 tasks

KazuCocoa mentioned this pull request Aug 4, 2018

fix raising NotYetImplementedError against commands which exist in image-element #245

Merged

mykola-mokhnach mentioned this pull request Aug 4, 2018

FindByImage Support appium/appium#943

Closed

KazuCocoa mentioned this pull request Aug 5, 2018

use normal element appium/python-client#236

Merged

		@@ -439,7 +439,7 @@ function driverShouldDoJwpProxy (driver, req, command) {

		// if it looks like we have an image element in the body, never proxy
		const stringBody = JSON.stringify(req.body);

Uh oh!

add -image locator strategy and related logic #235

add -image locator strategy and related logic #235

Uh oh!

Conversation

jlipps commented Jul 15, 2018

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!

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 Jul 15, 2018 • 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 commented Jul 19, 2018

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!

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

mykola-mokhnach Jul 15, 2018 •

edited

Loading