Implement protocol handshake for Ruby bindings by p0deje · Pull Request #3809 · SeleniumHQ/selenium

p0deje · 2017-04-11T02:29:57Z

Description

~~This is currently work in progress.~~ This is done.

We need Ruby bindings to detect what protocol dialect it should talk (e.g. current IEDriver talks OSS dialect, but newer beta versions talk W3C dialect). In order to do that, we need to implement protocol handshake from Java bindings which:

See commit messages for details.

TODO

Address all new TODOs (some of them are kept for later)
Verify driver extensions are correct for all browsers
Get approval from @lmtierney and @titusfortner
Fix Travis

Tests

lmtierney · 2017-04-16T03:19:49Z

Hoping to take a look at this soon

p0deje · 2017-04-16T04:47:42Z

@lmtierney @titusfortner This is essentially done, so you can review it. I still need to fix unit tests on Travis (there should be 1 spec failing), but that will be a tiny change.

Since the changes are huge and it's only one commit, it might be better to have a call with screen sharing so I could guide you through the changes - it is hard to figure this out from the diff. If not, I'll try to write down the changes in architecture and post in this PR. Just let me know what you guys prefer.

lmtierney · 2017-04-22T02:21:33Z

+
+          def commands(command)
+            case command
+            when :status, :is_element_displayed


@titusfortner Any idea why this was here? There are plenty of commands duplicated between the two, it seems odd that these were singled out

e3fbb4a#diff-5efb39c0076ea2abd8b2446e895603b5R93

These two commands are implemented, but aren't part of w3c spec, so they reference the oss implementation. Probably not necessary to do it this way.

lmtierney · 2017-04-22T02:28:55Z

+            :implicit_timeout,
+            :page_load_timeout,
+            :script_timeout,
+          ].freeze


Spec has required capabilities of:

browser_name

browser_version

platform_name

accept_insecure_certs

page_load_strategy

proxy

set_window_rect

timeouts

unhandled_prompt_behavior

I'd like to rework the capabilities in the followup PRs, I've left a TODO for it above the constant.

lmtierney

Looks good to me, great work

lmtierney · 2017-04-22T02:30:32Z

+              opts[:platform_name] = opts.delete(:platform) if opts.key?(:platform)
+              opts[:timeouts] = {}
+              opts[:timeouts]['implicit'] = opts.delete(:implicit_timeout) if opts.key?(:implicit_timeout)
+              opts[:timeouts]['page load'] = opts.delete(:page_load_timeout) if opts.key?(:page_load_timeout)


this should actually be pageLoad per the spec, which means we currently have it wrong

let's fix it in the next PRs

lmtierney · 2017-04-22T02:37:29Z

    "lib/selenium/webdriver/ie.rb"
  ],
+  resources = [
+    { "//javascript/webdriver/atoms:getAttribute": "rb/lib/selenium/webdriver/atoms/getAttribute.js"}


Since we now have the protocol handshake, we should probably add this to common instead of including in each one. The only one that shouldn't need it in the long run would be legacy FF and it's not that much overhead

Sounds reasonable.

p0deje · 2017-04-28T09:21:58Z

It's been almost 2 weeks, so I'm going to merge it later today unless there will be any objections.

titusfortner · 2017-04-28T21:09:25Z

I'm halfway through a review, but there's a lot more code here than I have time to work through right now. I can finish up tomorrow. Thanks for your patience.

titusfortner

I'm not sure I fully get the move from Bridge -> Driver. It feels like this is adding a lot of complexity because there is more module extension and requiring of modules with class methods than just a straight class hierarchy as before. For what we need to do with capabilities classes, though, maybe this is necessary.

I guess it decouples capabilities from the Bridge classes. So the Bridge is now just the command interface to the server/driver, which is a positive move.

Not that it should be our primary concern, but I'm curious how much this is going to affect the Ruby Appium library since it does so much monkey patching of the internals.

Also, we've been telling people that if they want to customize their driver/server interaction to subclass a Bridge and make their own custom commands. So, I'm concerned that this change will completely break their implementation.

From a high level view, I don't think this is how I would have approached it, but sketching out a few alternate ideas this week I've come to the conclusion that a more elegant solution isn't obvious to me. :) As for the actual implementation of this design, I think I see how everything is working and I don't have any major issues.

Thanks for all the work on this.

titusfortner · 2017-04-28T20:39:26Z

+              opts[:url] = @launcher.url
+            end
+
+            @bridge = Remote::Bridge.handshake(opts)


I notice you forced the Edge implementation to use W3C protocol. Since this is the "Legacy Firefox" when would this get called and need to use W3C?

There are some weird things there. You've implemented Edge bridge as W3C bridge, however it responds like OSS one, so there is a bit of confusion here. By forcing W3C in Edge, I'm keeping the current behavior and implementation, though I'd like to rework this later.

In regards to legacy Firefox, there is actually no need to do this, so I'll change.

Edge was implemented as W3C because that was the format of the desired capabilities it accepted, which was what I was most concerned about. Edge Driver has from the beginning supported an interesting mix of OSS & W3C commands. Yes, this distinction won't matter once we send both formats, though it could raise some other issues if this approach expects drivers to be completely compatible with either OSS or W3C.

I've changed it so now neither Marionette::Driver nor Legacy::Driver do handshake.

titusfortner · 2017-04-28T20:40:53Z

+
+        module Bridge
+
+          # Support for geckodriver < 0.15


Unless you really want to, I'm ok with telling Ruby users to upgrade to the latest geckodriver. :)

I put this in the current bridge, @p0deje you can remove these commands

@lmtierney Sorry, I don't follow what you mean

@p0deje you can get rid of this entire bridge file. These window commands were added for backward compatibility with older geckodriver versions. Since we are fine with requiring the latest geckodriver they aren't needed anymore

titusfortner · 2017-04-28T21:04:49Z

-            execute :touch_scroll, {}, {element: element,
-                                       xoffset: x,
-                                       yoffset: y}
+          if oss_status


I'm not sure this is the correct toggle for this. Are we sure that Se4 Server isn't going to return a status value with a w3c response once it supports that? I think we might need to parse the capabilities themselves for this conditional. Of course we can make that a TODO for later if necessary.

That's how Java and .NET bindings do this, so ¯_(ツ)_/¯

p0deje · 2017-04-30T05:44:38Z

I guess it decouples capabilities from the Bridge classes. So the Bridge is now just the command interface to the server/driver, which is a positive move.

Yeah, that was the idea.

Not that it should be our primary concern, but I'm curious how much this is going to affect the Ruby Appium library since it does so much monkey patching of the internals.

I know they have already created appium/ruby_lib#558 and appium/ruby_lib#559. It would be great to hear from them here, but I don't really have time to reach them out.

Also, we've been telling people that if they want to customize their driver/server interaction to subclass a Bridge and make their own custom commands. So, I'm concerned that this change will completely break their implementation.

It's not changed, isn't it? You can still pass custom bridge so I don't see a problem here:

bridge = MyCustomBridge.new
driver = Selenium::WebDriver::Driver.new(bridge)

From a high level view, I don't think this is how I would have approached it, but sketching out a few alternate ideas this week I've come to the conclusion that a more elegant solution isn't obvious to me. :) As for the actual implementation of this design, I think I see how everything is working and I don't have any major issues.

I was attempting to also make it more consistent to other bindings. None of them have a bridge, but all of them have custom driver classes, so I actually think it is now clearer. I would have to decouple capabilities and bridge anyways and driver class seems like a best option.

p0deje · 2017-05-02T12:56:34Z

Ah, okay, awesome!

…

On Tue, May 2, 2017 at 6:42 PM Lucas Tierney ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In rb/lib/selenium/webdriver/firefox/marionette/bridge.rb <#3809 (comment)>: > +# specific language governing permissions and limitations +# under the License. + +module Selenium + module WebDriver + module Firefox + module Marionette + + # + # Extension for the usual Remote::W3C::Bridge + # @api private + # + + module Bridge + + # Support for geckodriver < 0.15 @p0deje <https://github.com/p0deje> you can get rid of this entire bridge. These window commands were added for backward compatibility with older geckodriver versions. Since we are fine with requiring the latest geckodriver they aren't needed anymore — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3809 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoo9vr02xnqKIyHbvS2Vc-noYg5k9cdks5r1ySYgaJpZM4M5i0l> .

Ruby bindings need to detect what protocol dialect it should talk using protocol handshake which: 1. Prepares capabilities for both OSS and W3C dialects. 2. Sends them in a single payload like this: { "desiredCapabilities": {...}, "capabilities": { "firstMatch": [{...}] } } 3. Based on a response, understand which dialect it needs to use. This commit adds Remote::Bridge.handshake, refactors driver-specific bridges into Driver classes which are responsible for preparing driver-specific capabilities, calling handshake and using the bridge it returns (OSS or W3C) to abstract dialect-specific communication. For Firefox, it introduces Legacy and Marionette namespaces with its own Driver classes and bridge extensions. For Edge, it uses a hacky way to force W3C dialect even though MicrosoftWebDriver responds with OSS-like body. It also namespaces remote bridges/commands/etc. into OSS and W3C modules respectively.

p0deje · 2017-05-02T16:19:03Z

All the comments are addressed, now just waiting for CI.

p0deje added the C-rb Ruby Bindings label Apr 11, 2017

p0deje force-pushed the rb-handshake branch from 6b98ef2 to 4eab6b1 Compare April 11, 2017 02:34

p0deje added D-chrome D-edge D-firefox D-IE Z-marionette Archived: Marionette browser / driver D-safari labels Apr 11, 2017

p0deje force-pushed the rb-handshake branch from 4eab6b1 to 0a5fb18 Compare April 11, 2017 02:59

p0deje changed the title ~~Implement protocol handshake in Ruby~~ Implement protocol handshake for Ruby bindings Apr 11, 2017

p0deje force-pushed the rb-handshake branch 4 times, most recently from 0630dfe to e92be5e Compare April 12, 2017 06:46

p0deje mentioned this pull request Apr 13, 2017

Support W3C implementation of IE driver #3723

Closed

p0deje force-pushed the rb-handshake branch from e92be5e to 2cb427b Compare April 16, 2017 04:41

lmtierney self-requested a review April 22, 2017 02:18

lmtierney reviewed Apr 22, 2017

View reviewed changes

lmtierney approved these changes Apr 22, 2017

View reviewed changes

p0deje force-pushed the rb-handshake branch from 2cb427b to 11f974a Compare April 22, 2017 07:25

KazuCocoa mentioned this pull request Apr 22, 2017

Update selenium-webdriver related methods appium/ruby_lib#558

Closed

p0deje force-pushed the rb-handshake branch 2 times, most recently from f6e9e7d to f45ca34 Compare April 28, 2017 09:20

p0deje force-pushed the rb-handshake branch from f45ca34 to 38efaf1 Compare April 28, 2017 09:57

titusfortner approved these changes Apr 29, 2017

View reviewed changes

p0deje force-pushed the rb-handshake branch from 38efaf1 to 0439620 Compare May 2, 2017 06:46

p0deje force-pushed the rb-handshake branch from 0439620 to a8a0034 Compare May 2, 2017 16:18

p0deje merged commit a8a0034 into master May 3, 2017

p0deje deleted the rb-handshake branch May 3, 2017 02:34

Uh oh!

Conversation

p0deje commented Apr 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Tests

Uh oh!

lmtierney commented Apr 16, 2017

Uh oh!

p0deje commented Apr 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmtierney left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0deje commented Apr 28, 2017

Uh oh!

titusfortner commented Apr 28, 2017

Uh oh!

titusfortner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmtierney May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0deje commented Apr 30, 2017

Uh oh!

p0deje commented May 2, 2017 via email

Uh oh!

p0deje commented May 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

p0deje commented Apr 11, 2017 •

edited

Loading

lmtierney May 2, 2017 •

edited

Loading