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

Remove restrictions on browserName and combining with app capability for Grid #139

Merged
merged 1 commit into from May 1, 2016

Conversation

Projects
None yet
3 participants
@jessehudl
Contributor

jessehudl commented Apr 28, 2016

This is the android implementation of appium/appium-ios-driver#127, which has already been merged.
#19 looks to have added these particular restrictions. browserName needs to be open since that's how the Selenium Grid matches nodes. See appium/appium-ios-driver#127 for more details.

This PR also had to remove the specific check for providing browserName and app together, which was not necessary for the iOS PR. I'm not positive what the rationale for this was, I'm assuming to try and catch mistakenly providing both. However providing both still prefers app with a custom browserName (which is necessary for the Grid). Another alternative would be to check if this server is being run as a Grid node, to do this conditionally. Perhaps @jlipps can comment since he added these changes?

I've left CHROME_BROWSERS in android-helpers.js since it appears to be used by other things.

I've tested these changes with Appium v1.5 locally and with Selenium Grid 2.52.0.

@jessehudl

This comment has been minimized.

Contributor

jessehudl commented Apr 28, 2016

Looks like I'll need to update tests to go along with this, since they explicitly test for these cases. I'd like to get some feedback if possible, before spending the time to do that.

@jlipps

This comment has been minimized.

Member

jlipps commented Apr 29, 2016

This makes sense to me. Though I'd like to keep the check and emit a warning so people know why there might be odd behavior if they use both caps. Does that sound good to you?

@imurchie

This comment has been minimized.

Member

imurchie commented Apr 29, 2016

I agree with having a log message that both are set. Otherwise this is fine.

let msg = 'The desired capabilities should not include both an app and a browser';
log.errorAndThrow(msg);
let msg = 'The desired capabilities should generally not include both an app and a browser';
log.warn(msg);

This comment has been minimized.

@jessehudl

jessehudl Apr 29, 2016

Contributor

Warn instead of throwing an error.

@@ -244,7 +244,7 @@ describe('driver', () => {
}).to.throw(/must include/);
expect(() => {
driver.validateDesiredCaps({platformName: 'Android', deviceName: 'device', browserName: 'Netscape Navigator'});
}).to.throw(/Netscape Navigator/);
}).to.throw(/must include/);

This comment has been minimized.

@jessehudl

jessehudl Apr 29, 2016

Contributor

Since validateDesiredCaps() when passing in an unknown browser no longer fails with:

The desiredCapabilities object was not valid for the following reason(s): browserName Netscape Navigator is not included in the list.

Change the test to check for the error:

The desired capabilities must include either an app, package or browser
@@ -257,16 +257,11 @@ describe('driver', () => {
driver.validateDesiredCaps({platformName: 'Android', deviceName: 'device', appPackage: 'some.app.package'});
}).to.not.throw(/must include/);
});
it('should not be sentivie to platform name casing', () => {
it('should not be sensitive to platform name casing', () => {

This comment has been minimized.

@jessehudl

jessehudl Apr 29, 2016

Contributor

Noticed this typo while I was in here.

expect(() => {
driver.validateDesiredCaps({platformName: 'Android', deviceName: 'device', app: '/path/to/some.apk', browserName: 'Chrome'});
}).to.throw(/should not include both/);
});

This comment has been minimized.

@jessehudl

jessehudl Apr 29, 2016

Contributor

Remove this test since this is a valid combination when used as a Grid node.

@jlipps

This comment has been minimized.

Member

jlipps commented Apr 29, 2016

@jessehudl LGTM now; can you please squash into one commit?

if (caps.browserName && caps.app) {
let msg = 'The desired capabilities should not include both an app and a browser';
log.errorAndThrow(msg);
let msg = 'The desired capabilities should generally not include both an app and a browser';

This comment has been minimized.

@jessehudl

jessehudl Apr 29, 2016

Contributor

Changed message to include the word "generally" since it's not necessarily bad, also make a note in the comment that it's common when using Appium with Selenium Grid.

@jessehudl

This comment has been minimized.

Contributor

jessehudl commented Apr 29, 2016

@jlipps I did 2 commits since one of the changes is unrelated to this work and just fixes a typo I noticed. Do you still want me to squash?

@jessehudl

This comment has been minimized.

Contributor

jessehudl commented Apr 29, 2016

Looks like there's an e2e test for this too, I'll squash and remove that one as well.

@jessehudl jessehudl closed this Apr 29, 2016

@jessehudl jessehudl reopened this Apr 29, 2016

@jlipps

This comment has been minimized.

Member

jlipps commented Apr 29, 2016

ok, everything looks good now. thanks! i'll let @imurchie do a pass and merge

Jesse Jarzynka
Remove restrictions on `browserName`, and having `app` capability for…
… grid

This restriction broke compatibility with Selenium Grid where the browserName
capability is used to define the node's available simulator/devices. Also
remove/modify tests that are affected.
@jessehudl

This comment has been minimized.

Contributor

jessehudl commented Apr 29, 2016

Sorry guys, I just pushed again. I decided to change the existing unit test to specifically check that this case does pass, so it doesn't break in the future.

Just curious, is SauceLabs using Selenium Grid? Since most of the Appium maintainers seem to work for Sauce, I'm wondering if it's actually getting tested in this way.

@jlipps

This comment has been minimized.

Member

jlipps commented Apr 29, 2016

@jessehudl no, Sauce does not use Selenium grid :-)

@jessehudl

This comment has been minimized.

Contributor

jessehudl commented Apr 29, 2016

@jlipps ah, secret "sauce" huh? 😬

Anyways, everything's passing and should be ready for code review, or merge 😀

@imurchie

This comment has been minimized.

Member

imurchie commented May 1, 2016

Looks good to me! Thanks for this! And thanks for taking the time to fix the typo!

@imurchie imurchie merged commit 54a836b into appium:master May 1, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
default e2e-Tests passed No test results found.
Details

@jessehudl jessehudl deleted the jessehudl:less_restrictive_browsername branch May 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment