-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[rb] Initial extensibility points for Appium #12141
Changes from all commits
c9b3513
82e179d
e078018
724d7dc
5d648ad
cae37fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
# frozen_string_literal: true | ||
|
||
# Licensed to the Software Freedom Conservancy (SFC) under one | ||
# or more contributor license agreements. See the NOTICE file | ||
# distributed with this work for additional information | ||
# regarding copyright ownership. The SFC licenses this file | ||
# to you under the Apache License, Version 2.0 (the | ||
# "License"); you may not use this file except in compliance | ||
# with the License. You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, | ||
# software distributed under the License is distributed on an | ||
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
|
||
module Selenium | ||
module WebDriver | ||
module Remote | ||
class Bridge | ||
class LocatorConverter | ||
ESCAPE_CSS_REGEXP = /(['"\\#.:;,!?+<>=~*^$|%&@`{}\-\[\]()])/ | ||
UNICODE_CODE_POINT = 30 | ||
|
||
# | ||
# Converts a locator to a specification compatible one. | ||
# @param [String, Symbol] how | ||
# @param [String] what | ||
# | ||
|
||
def convert(how, what) | ||
how = SearchContext.finders[how.to_sym] || how | ||
|
||
case how | ||
when 'class name' | ||
how = 'css selector' | ||
what = ".#{escape_css(what.to_s)}" | ||
when 'id' | ||
how = 'css selector' | ||
what = "##{escape_css(what.to_s)}" | ||
when 'name' | ||
how = 'css selector' | ||
what = "*[name='#{escape_css(what.to_s)}']" | ||
end | ||
|
||
if what.is_a?(Hash) | ||
what = what.each_with_object({}) do |(h, w), hash| | ||
h, w = convert(h.to_s, w) | ||
hash[h] = w | ||
end | ||
end | ||
|
||
[how, what] | ||
end | ||
|
||
private | ||
|
||
# | ||
# Escapes invalid characters in CSS selector. | ||
# @see https://mathiasbynens.be/notes/css-escapes | ||
# | ||
|
||
def escape_css(string) | ||
string = string.gsub(ESCAPE_CSS_REGEXP) { |match| "\\#{match}" } | ||
string = "\\#{UNICODE_CODE_POINT + Integer(string[0])} #{string[1..]}" if string[0]&.match?(/[[:digit:]]/) | ||
|
||
string | ||
end | ||
end # LocatorConverter | ||
end # Bridge | ||
end # Remote | ||
end # WebDriver | ||
end # Selenium |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,10 +26,18 @@ class Common | |
CONTENT_TYPE = 'application/json' | ||
DEFAULT_HEADERS = { | ||
'Accept' => CONTENT_TYPE, | ||
'Content-Type' => "#{CONTENT_TYPE}; charset=UTF-8", | ||
'User-Agent' => "selenium/#{WebDriver::VERSION} (ruby #{Platform.os})" | ||
'Content-Type' => "#{CONTENT_TYPE}; charset=UTF-8" | ||
}.freeze | ||
|
||
class << self | ||
attr_accessor :extra_headers | ||
attr_writer :user_agent | ||
|
||
def user_agent | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, do we need all this? Watir is just subclassing the Default client and making a tweak here. https://github.com/watir/watir/blob/main/lib/watir/http_client.rb What is Appium doing now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @KazuCocoa Would implementing a custom HTTP client in Appium be better than this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, the watir way seems good. Appium overrides DEFAULT_HEADERS, b ut the watir way is sufficient for the usage. Then, this pr can drop the user_agent related change actually There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear, I'm not saying the Watir approach is "correct," if it makes more sense to do something else, Watir will do that way. 😄 Presumably we are supposed to avoid using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea. Actually, the Watir way is better than the current Appium's one at least :P Hmm, I like this pr approach, to provide a way to extend headers explicitly, than overriding the parent class and calling super There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, yes, I agree. With the approach in this PR we can just inject what the user sends regardless. Would be better to minimize subclassing. @twalpole does capybara do anything to the user agent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it doesn't |
||
@user_agent ||= "selenium/#{WebDriver::VERSION} (ruby #{Platform.os})" | ||
end | ||
end | ||
|
||
attr_writer :server_url | ||
|
||
def quit_errors | ||
|
@@ -42,7 +50,7 @@ def close | |
|
||
def call(verb, url, command_hash) | ||
url = server_url.merge(url) unless url.is_a?(URI) | ||
headers = DEFAULT_HEADERS.dup | ||
headers = common_headers.dup | ||
headers['Cache-Control'] = 'no-cache' if verb == :get | ||
|
||
if command_hash | ||
|
@@ -61,6 +69,16 @@ def call(verb, url, command_hash) | |
|
||
private | ||
|
||
def common_headers | ||
@common_headers ||= begin | ||
headers = DEFAULT_HEADERS.dup | ||
headers['User-Agent'] = Common.user_agent | ||
headers = headers.merge(Common.extra_headers || {}) | ||
|
||
headers | ||
end | ||
end | ||
|
||
def server_url | ||
return @server_url if @server_url | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we want Selenium to use this as part of
Features
modules, so we aren't subclassing#commands
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is to say, we probably should be doing the same thing with custom browser features as Appium does, either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I would refactor Selenium after Appium approves the changes.