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

Remove safari-only restriction for browserName capability #127

Merged
merged 1 commit into from Apr 28, 2016

Conversation

@jessehudl
Copy link
Contributor

@jessehudl jessehudl commented Apr 18, 2016

Fixes appium/appium/issues/6412
#5 added desired capabilities restraints, which restrict the allowed browserName for iOS to be only one of ['Safari', 'safari']. However, the Appium docs have long recommended using browserName as a way to specify the type of simulator/device being used, as shown in the example Grid node configuration:

          "browserName": "<e.g._iPhone5_or_iPad4>",

This means existing Selenium Grid -> Appium node configurations using browserName to specify node sessions in this way are effectively incompatible with Appium v1.5 since you now get the error:

[BaseDriver] SessionNotCreatedError: A new session could not be created. Details: The desiredCapabilities object was not valid for the following reason(s): browserName iPhone 6s is not included in the list.

Removing the specific [Ss]afari browserName restriction allows the Selenium grid to find the node properly, in which case the desired caps are passed along to the Appium server and browserName is ignored (since the app capability takes precedence). The restrictions may overall be a good thing and catch misconfigurations, but I think they should be backwards compatible with existing implementations. There is not really an alternative for Selenium Grid to be able to match nodes that I know of aside from using the catch-all applicationName capability, which is not well documented and requires modifying existing configurations.

I've tested this with Appium v1.5.1 and Selenium Grid 2.52.0. Will follow-up for appium-android-driver if this is accepted.

@jessehudl
Copy link
Contributor Author

@jessehudl jessehudl commented Apr 26, 2016

I think the build/status check is stuck, it's been pending for over a week.

@jessehudl
Copy link
Contributor Author

@jessehudl jessehudl commented Apr 27, 2016

I noticed the stuck build was cancelled, so I triggered a new build with some empty commits, I'm getting the keyboard test failing:

  87 passing (19m)
  1 pending
  1 failing

  1) testapp - keyboard typing should send backspace key:

      AssertionError: expected 'a' to equal 'ab'
      + expected - actual

      -a
      +ab

      at Suite.callee$2$0$ (test/e2e/testapp/keyboard-specs.js:79:41)
      at tryCatch (node_modules/babel-runtime/regenerator/runtime.js:67:40)
      at GeneratorFunctionPrototype.invoke [as _invoke] (node_modules/babel-runtime/regenerator/runtime.js:315:22)
      at GeneratorFunctionPrototype.prototype.(anonymous function) [as next] (node_modules/babel-runtime/regenerator/runtime.js:100:21)
      at GeneratorFunctionPrototype.invoke (node_modules/babel-runtime/regenerator/runtime.js:136:37)
      at run (node_modules/babel-runtime/node_modules/core-js/library/modules/es6.promise.js:104:47)
      at node_modules/babel-runtime/node_modules/core-js/library/modules/es6.promise.js:115:28
      at flush (node_modules/babel-runtime/node_modules/core-js/library/modules/$.microtask.js:19:5)

This change should not affect that at all. Should I push another commit to trigger another build?

@imurchie
Copy link
Contributor

@imurchie imurchie commented Apr 27, 2016

I've manually triggered the build, and am keeping an eye on it.

@imurchie
Copy link
Contributor

@imurchie imurchie commented Apr 27, 2016

I agree old ways of doing things should be supported until there is a better way. Thanks for delving into the problem and coming up with a fix!

@imurchie
Copy link
Contributor

@imurchie imurchie commented Apr 27, 2016

Build works fine now. Can you squash this into a single commit? Then I can merge.

@jessehudl jessehudl force-pushed the jessehudl:less_restrictive_browsername branch from 5454119 to 6d2ac5b Apr 27, 2016
@jessehudl
Copy link
Contributor Author

@jessehudl jessehudl commented Apr 27, 2016

@imurchie squashed into a single commit, thanks!

@jessehudl
Copy link
Contributor Author

@jessehudl jessehudl commented Apr 27, 2016

Also, I don't think this triggered a new build since the original commit is the same as the original one that was stuck/cancelled. I don't have the ability to trigger a new one (if it's needed).

This restriction broke compatibility with Selenium Grid where the browserName
capability is used to define the node's available simulator/devices.
@jessehudl jessehudl force-pushed the jessehudl:less_restrictive_browsername branch from 6d2ac5b to 04add76 Apr 28, 2016
@jessehudl
Copy link
Contributor Author

@jessehudl jessehudl commented Apr 28, 2016

I changed the commit to force a build just in case, so we don't have to keep playing tag. Hopefully it passes 🙏

@imurchie imurchie merged commit 145a7b6 into appium:master Apr 28, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@imurchie
Copy link
Contributor

@imurchie imurchie commented Apr 28, 2016

Thanks for this!

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.