add custom locator strategy #268
Conversation
jlipps
requested review from
dpgraham,
imurchie,
KazuCocoa and
mykola-mokhnach
|
An alternative approach I thought about was to encode both the module name and the selector into the custom find selector. So with the code as it stands, there are two things you need to do to make this work:
This has the disadvantages of requiring a cap in addition to the element call, and allowing only one find plugin to be in use in a given session. Alternatively, we could come up with a scheme like:
Where we encode both the module and the selector into the selector field. This does away with the disadvantages above, but means that every find will be much more verbose and require client-side abstraction to manage. I'm on the fence about which is better. What do you all think? |
lib/basedriver/commands/find.js
Outdated
| * @returns {WebElement} - WebDriver element or list of elements | ||
| */ | ||
| commands.findByCustom = async function (selector, multiple) { | ||
| if (!this.opts.customFindModule) { |
There was a problem hiding this comment.
do we allow only one such module to be set at the same time or many of them?
There was a problem hiding this comment.
the way the code is written now, just one module. but see the discussion we're having about whether we want something else.
lib/basedriver/commands/find.js
Outdated
| */ | ||
| commands.findByCustom = async function (selector, multiple) { | ||
| if (!this.opts.customFindModule) { | ||
| throw new Error("Finding an element using a pluagin is currently an " + |
lib/basedriver/commands/find.js
Outdated
| if (!this.opts.customFindModule) { | ||
| throw new Error("Finding an element using a pluagin is currently an " + | ||
| "incubating feature. To use it you must manually install a " + | ||
| "plugin module inside your Appium node_modules tree, " + |
There was a problem hiding this comment.
should we explicitly say where to install it? npm might change this in the future though
lib/basedriver/commands/find.js
Outdated
| throw new Error(`Could not load your custom find module. Original error: ${err}`); | ||
| } | ||
|
|
||
| if (!_.isFunction(finder.find)) { |
There was a problem hiding this comment.
finder && !_.isFunction ... would be safer
There was a problem hiding this comment.
hmm, i guess it's possible that a module is not truthy?
lib/basedriver/commands/find.js
Outdated
| try { | ||
| finder = require(this.opts.customFindModule); | ||
| } catch (err) { | ||
| throw new Error(`Could not load your custom find module. Original error: ${err}`); |
There was a problem hiding this comment.
I'd put the actual module name instead
lib/basedriver/commands/find.js
Outdated
|
|
||
| let finder; | ||
| try { | ||
| finder = require(this.opts.customFindModule); |
There was a problem hiding this comment.
is this secure in case one adds some malicious module? do we want to check some extra stuff here?
There was a problem hiding this comment.
i'm not sure what else we can check. it's the users responsibility to vet 3rd-party plugins, which could indeed be malicious.
probably at some point we'll have a registry of 'approved' plugins that we have vetted.
lib/basedriver/commands/find.js
Outdated
|
|
||
| const customFinderLog = logger.getLogger(this.opts.customFindModule); | ||
|
|
||
| let els; |
There was a problem hiding this comment.
I'd rather change it to elements. The current one is not very readable
lib/basedriver/commands/find.js
Outdated
|
|
||
| // if we're looking for multiple elements, or if we're looking for only | ||
| // one and found it, we're done | ||
| if (els.length || multiple) { |
There was a problem hiding this comment.
!_.isEmpty() is easier to read
lib/basedriver/commands/find.js
Outdated
| throw err; | ||
| } | ||
|
|
||
| if (multiple) { |
There was a problem hiding this comment.
return multiple ? els : els[0]
Can we relax the condition by adding the caps as an array like Personally, I like below way to make managing them easier. (More explicit way)
If we'll allow only one custom module against a session, the original way, |
|
I like the idea but I'm concerned about the practice of inserting a node module into Appium's existing node_modules because that may require tampering with versions of packages and it may break the determinism of the shrinkwrap. May I suggest that the plugins be standalone node packages that sit somewhere on the filesystem, either in some permanent directory that stores plugins globally or in a Something like: // magic-finder-plugin.js
function pluginInitializer(appiumDepOne, appiumDepTwo, appiumDepThree, ...) {
...
}
export {pluginInitializer};
// finder-plugins.json
[
{name: 'magic-finder-plugin', path: '/path/to/magic-finder-plugin.js'}
]
// appium-base-driver.js
function initializeFinderPlugins() {
const plugins = require('./plugins.json');
for (let plugin of plugins) {
const {pluginInitializer} = require(plugin.path);
finderPlugin(appiumDepOne, appiumDepTwo, appiumDepThree, ...);
}
} |
|
What if we made the plugins a server-flag and when you start the server it installs the plugins (if they're not already there) |
|
Thanks for the thoughts so far @mykola-mokhnach @KazuCocoa @dpgraham. There are now two design questions on the table, each with a few proposed answers:
As for design question (1), it seems like the consensus so far is that (1a) would be potentially too restrictive, and a more flexible approach would be better so that we could choose from a number of plugins. Are there any other proposals than (1b)? As for design question (2), the proposals raise some interesting considerations. I went initially with (2a) because anything else encroaches on the territory taken up by Appium 2.0. I wanted to be careful not to design something that we would just throw away when we finally decide on an implementation for Appium 2.0. Alternatively, we could use this as an opportunity to actually move forward with the Appium 2.0 design in an area which isn't super crucial, namely this new plugin feature. We would just need to put the time into making good decisions. At a first pass, something like the As for your specific suggestion, @dpgraham:
I like this suggestion a lot, however if we are moving to a CLI installer model for Appium 2.0 ( I'm open here; just wanted to start out with something simple that advanced users could play around with while we determined whether this whole plugin thing was truly useful. Thoughts? Would love to hear @imurchie's views as well. |
|
Lgtm |
|
@dpgraham @KazuCocoa @mykola-mokhnach just pushed a change that will allow users to register multiple element finder plugins as an object, allowing for a 'shortcut' name. for example, i could register one like this: {"customFindModules": {"1": "/really/long/path/to/find/plugin/module/"}}and then in my test code: driver.findElement('-custom', '1:selector');so I can just use take a look at the code, and i'll start working on unit tests. |
jlipps
force-pushed
the
jlipps-custom-loc-strat
branch
from
7339a5e
to
515894b
Compare
|
tests pushed |
| it('should be able to use multiple find modules', async function () { | ||
| const d = new BaseDriver(); | ||
| d.opts.customFindModules = {f: CUSTOM_FIND_MODULE, g: CUSTOM_FIND_MODULE}; | ||
| await d.findElement(CUSTOM_STRATEGY, "f:foo").should.eventually.eql("bar"); |
|
thanks everyone! published as 3.9.0. once appium 2 rolls around we can merge this into whatever the ultimate plugin structure becomes. |
|
Hi @jlipps, is adding the custom locator strategy in the ruby-client on the horizon? :) |
|
let's try out the latest ruby_lib_core (2.0.4) |
|
I am able to launch the app with the below capabilities But not able to find the text box with the visible text like driver.findElement(MobileBy.custom("ai:User ID")) getting the below exception Element info: {Using=-custom, value=ai:User ID} |
|
@SoundaryaBhavanam |
The idea behind this PR is to open up the possibility of "element finding plugins", which are third-party modules that users can use for element finding functionality that probably shouldn't be maintained by the core project but which nonetheless might be useful. (For example, I'm working on an experimental locator strategy involving a machine-learning model, which probably shouldn't become part of Appium core, at least not for some time).
The idea for how this should be used is as follows:
findmethod matching the signature described in this PR. Essentially, that third party module is passed a driver object so it can mix and match Appium capabilities with its own logic.customFindModulecapability to the name of the node module.Right now, we assume that the node module is installed in Appium's tree (or can otherwise be
required). In the future, with the Appium 2.0 architecture, we could allow plugins like these to be managed via the Appium CLI.We might have other kinds of plugins in the future, beyond just element finding ones, but this is the first need I've come across.
Anyway, have a look and once everyone is a fan of the idea I'll work on tests etc.