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

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

merged 5 commits into from Apr 7, 2019

Conversation

mykola-mokhnach
Copy link
Collaborator

Proposed changes

It is sometimes necessary to set some particular appium setting per session. This feature will allow to pass default Appium settings values in capabilities, for example

{
        platformName: 'foo',
        browserName: 'bar',
        'settings[settingName]': 'baz',
        'settings[settingName2]': 'baz2',
}

so there is no need to separately call updateSettings API after the session is created.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

nice idea! any reason you chose to put each setting as a string of the form settings[setting] rather than just adding a new cap called settings which has a value of type object? seems like that obviates the need for pullSettings entirely. might be a little harder to construct the cap client side, but every language i've seen has the possibility of complex capability values.

lib/appium.js Outdated
jsonwpCaps = _.cloneDeep(jsonwpCaps);
const jwpSettings = Object.assign({}, defaultSettings, pullSettings(jsonwpCaps));
w3cCapabilities = _.cloneDeep(w3cCapabilities);
const w3cSettings = Object.assign({}, jwpSettings, pullSettings(w3cCapabilities));
Copy link
Member

Choose a reason for hiding this comment

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

why are we merging jwp settings into w3c settings? below these seem like exclusive options

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've seen the cases where the client sends jsonwp caps only, but the request contains firstMatch/alwaysMatch props. In such case we assume the client supports W3C and merge jwp caps to w3c ones

lib/utils.js Outdated
const match = /\bsettings\[(\S+)\]$/.exec(key);
if (!match) {
if (_.isPlainObject(value)) {
Object.assign(result, pullSettings(value));
Copy link
Member

Choose a reason for hiding this comment

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

i'm having trouble imagining the need for a recursive case...?

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 might be the case for w3c caps, where they have firstMatch and alwaysMatch properties

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've changed the logic there. Now it's no more recursive

@@ -178,4 +179,65 @@ 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

lib/utils.js Show resolved Hide resolved
@mykola-mokhnach
Copy link
Collaborator Author

mykola-mokhnach commented Apr 5, 2019

any reason you chose to put each setting as a string of the form settings[setting] rather than just adding a new cap called settings which has a value of type object? seems like that obviates the need for pullSettings entirely. might be a little harder to construct the cap client side, but every language i've seen has the possibility of complex capability values.

This is not very straightforward in java/C#, where one has to create a JSONObject first and put all the necessary values there. Also, I usually don't see a need to set multiple setting values at once. The current syntax looks more natural to set a couple of setting values. In the worst case we could also add a possibility to set settings[*], if this is really needed

@@ -251,9 +251,43 @@ function getPackageVersion (pkgName) {
return pkgInfo.version;
}

/**
* Pulls the initial values of Appium settings from the given capabilities argument.
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


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) || [];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants