Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a possibility to parse initial values for Appium settings from capabilities #12440

Merged
merged 5 commits into from Apr 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 29 additions & 4 deletions lib/appium.js
Expand Up @@ -4,7 +4,9 @@ import { getBuildInfo, updateBuildInfo, APPIUM_VER } from './config';
import { BaseDriver, errors, isSessionCommand } from 'appium-base-driver';
import B from 'bluebird';
import AsyncLock from 'async-lock';
import { inspectObject, parseCapsForInnerDriver, getPackageVersion } from './utils';
import {
inspectObject, parseCapsForInnerDriver, getPackageVersion,
pullSettings } from './utils';
import semver from 'semver';


Expand Down Expand Up @@ -246,10 +248,23 @@ class AppiumDriver extends BaseDriver {
* @return {Array} Unique session ID and capabilities
*/
async createSession (jsonwpCaps, reqCaps, w3cCapabilities) {
const {defaultCapabilities} = this.args;
const defaultCapabilities = _.cloneDeep(this.args.defaultCapabilities);
const defaultSettings = pullSettings(defaultCapabilities);
jsonwpCaps = _.cloneDeep(jsonwpCaps);
const jwpSettings = Object.assign({}, defaultSettings, pullSettings(jsonwpCaps));
w3cCapabilities = _.cloneDeep(w3cCapabilities);
// It is possible that the client only provides caps using JSONWP standard,
// although firstMatch/alwaysMatch properties are still present.
// In such case we assume the client understands W3C protocol and merge the given
// JSONWP caps to W3C caps
const w3cSettings = Object.assign({}, jwpSettings);
Object.assign(w3cSettings, pullSettings((w3cCapabilities || {}).alwaysMatch || {}));
for (const firstMatchEntry of ((w3cCapabilities || {}).firstMatch || [])) {
Object.assign(w3cSettings, pullSettings(firstMatchEntry));
}

let protocol;
let innerSessionId, dCaps;

try {
// Parse the caps into a format that the InnerDriver will accept
const parsedCaps = parseCapsForInnerDriver(
Expand Down Expand Up @@ -321,12 +336,22 @@ class AppiumDriver extends BaseDriver {
// unexpectedly shuts down
this.attachUnexpectedShutdownHandler(d, innerSessionId);


log.info(`New ${InnerDriver.name} session created successfully, session ` +
`${innerSessionId} added to master session list`);

// set the New Command Timeout for the inner driver
d.startNewCommandTimeout();

// apply initial values to Appium settings (if provided)
if (d.isW3CProtocol() && !_.isEmpty(w3cSettings)) {
log.info(`Applying the initial values to Appium settings parsed from W3C caps: ` +
JSON.stringify(w3cSettings));
await d.updateSettings(w3cSettings);
} else if (d.isMjsonwpProtocol() && !_.isEmpty(jwpSettings)) {
log.info(`Applying the initial values to Appium settings parsed from MJSONWP caps: ` +
JSON.stringify(jwpSettings));
await d.updateSettings(jwpSettings);
}
} catch (error) {
return {
protocol,
Expand Down
36 changes: 35 additions & 1 deletion lib/utils.js
Expand Up @@ -251,9 +251,43 @@ function getPackageVersion (pkgName) {
return pkgInfo.version;
}

/**
* Pulls the initial values of Appium settings from the given capabilities argument.
mykola-mokhnach marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like a link to http://appium.io/docs/en/advanced-concepts/settings/index.html so we know what "Settings" are.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see below, the link is already there

* Each setting item must satisfy the following format:
* `setting[setting_name]: setting_value`
* The capabilities argument itself gets mutated, so it does not contain parsed
* settings anymore to avoid further parsing issues.
* Check
* https://github.com/appium/appium/blob/master/docs/en/advanced-concepts/settings.md
* for more details on the available settings.
*
* @param {?Object} caps - Capabilities dictionary. It is mutated if
* one or more settings have been pulled from it
* @returns {Object} - An empty dictionary if the given caps contains no
* setting items or a dictionary containing parsed Appium setting names along with
* their values.
*/
function pullSettings (caps) {
if (!_.isPlainObject(caps) || _.isEmpty(caps)) {
return {};
}

const result = {};
for (const [key, value] of _.toPairs(caps)) {
const match = /\bsettings\[(\S+)\]$/.exec(key);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe const [, settingName] = /\bsettings\[(\S+)\]$/.exec(key);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will throw if match is not an array

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we still have a variable called settingName to make it more clear what's being parsed.

const [, settingName] = /\bsettings\[(\S+)\]$/.exec(key) || [];

if (!match) {
continue;
}

result[match[1]] = value;
delete caps[key];
}
return result;
}

const rootDir = findRoot(__dirname);

export {
inspectObject, parseCapsForInnerDriver, insertAppiumPrefixes, rootDir,
getPackageVersion,
getPackageVersion, pullSettings,
};
58 changes: 57 additions & 1 deletion test/utils-specs.js
@@ -1,6 +1,7 @@
import chai from 'chai';
import chaiAsPromised from 'chai-as-promised';
import { parseCapsForInnerDriver, insertAppiumPrefixes } from '../lib/utils';
import {
parseCapsForInnerDriver, insertAppiumPrefixes, pullSettings } from '../lib/utils';
import { BASE_CAPS, W3C_CAPS } from './helpers';
import _ from 'lodash';

Expand Down Expand Up @@ -178,4 +179,59 @@ describe('utils', function () {
});
});
});

describe('pullSettings()', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about a test for a setting that has an object as a value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, will add

it('should pull settings from caps', function () {
const caps = {
platformName: 'foo',
browserName: 'bar',
'settings[settingName]': 'baz',
'settings[settingName2]': 'baz2',
};
const settings = pullSettings(caps);
settings.should.eql({
settingName: 'baz',
settingName2: 'baz2',
});
caps.should.eql({
platformName: 'foo',
browserName: 'bar',
});
});
it('should pull settings dict if object values are present in caps', function () {
const caps = {
platformName: 'foo',
browserName: 'bar',
'settings[settingName]': {key: 'baz'},
};
const settings = pullSettings(caps);
settings.should.eql({
settingName: {key: 'baz'},
});
caps.should.eql({
platformName: 'foo',
browserName: 'bar',
});
});
it('should pull empty dict if no settings are present in caps', function () {
const caps = {
platformName: 'foo',
browserName: 'bar',
'setting[settingName]': 'baz',
};
const settings = pullSettings(caps);
settings.should.eql({});
caps.should.eql({
platformName: 'foo',
browserName: 'bar',
'setting[settingName]': 'baz',
});
});
it('should pull empty dict if caps are empty', function () {
const caps = {};
const settings = pullSettings(caps);
settings.should.eql({});
caps.should.eql({});
});
});
});