-
Notifications
You must be signed in to change notification settings - Fork 110
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
Check if we already have the latest/desired version before checking for updates. #49
Changes from all commits
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 |
---|---|---|
|
@@ -16,7 +16,13 @@ def current_version | |
end | ||
|
||
def latest_version | ||
raise StandardError, 'Can not reach site' unless site_available? | ||
unless site_available? | ||
cur_ver = current_version | ||
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. Not as big of a deal since they aren't network calls, but some paths through this code are also calling |
||
raise StandardError, update_failed_msg if cur_ver.nil? # Site is down and no existing binary | ||
|
||
Webdrivers.logger.warn "Can not reach update site. Using existing #{file_name} #{cur_ver}" | ||
return cur_ver | ||
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. This might not be the right logic here... We might want to throw an error instead of a warning if the driver & browser versions are technically incompatible. 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. Yeah, makes sense. I'm going to go ahead and refactor more code than I'd originally anticipated. Will make a list of all cases and then check them off one by one. |
||
end | ||
|
||
# Versions before 70 do not have a LATEST_RELEASE file | ||
return Gem::Version.new('2.46') if release_version < '70.0.3538' | ||
|
@@ -89,7 +95,7 @@ def chrome_on_windows | |
|
||
# Workaround for Google Chrome when using Jruby on Windows. | ||
# @see https://github.com/titusfortner/webdrivers/issues/41 | ||
if RUBY_PLATFORM == 'java' && platform == 'win' | ||
if RUBY_PLATFORM == 'java' | ||
ver = 'powershell (Get-Item -Path ((Get-ItemProperty "HKLM:\\Software\\Microsoft' \ | ||
"\\Windows\\CurrentVersion\\App` Paths\\chrome.exe\").\\'(default)\\'))" \ | ||
'.VersionInfo.ProductVersion' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,40 +7,52 @@ class << self | |
attr_accessor :version | ||
|
||
def update | ||
if correct_binary? # Already have desired or latest version | ||
Webdrivers.logger.debug 'Expected webdriver version found' | ||
return binary | ||
end | ||
|
||
# If site is down | ||
unless site_available? | ||
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. We've already checked for site availability, no need to make another call to get the same info. Let's memoize this method. |
||
return current_version.nil? ? nil : binary | ||
# No existing binary and we can't download | ||
raise StandardError, update_failed_msg if current_version.nil? | ||
|
||
# Use existing binary | ||
Webdrivers.logger.error "Can not reach update site. Using existing #{file_name} #{current_version}" | ||
return binary | ||
end | ||
|
||
# Newer not specified or latest not found, so use existing | ||
return binary if desired_version.nil? && File.exist?(binary) | ||
|
||
# Can't find desired and no existing binary | ||
if desired_version.nil? | ||
msg = "Unable to find the latest version of #{file_name}; try downloading manually from #{base_url} and place in #{install_dir}" | ||
raise StandardError, msg | ||
end | ||
|
||
if correct_binary? | ||
Webdrivers.logger.debug 'Expected webdriver version found' | ||
return binary | ||
end | ||
raise StandardError, update_failed_msg if desired_version.nil? | ||
|
||
remove # Remove outdated exe | ||
download # Fetch latest | ||
download # Fetch desired or latest | ||
end | ||
|
||
def desired_version | ||
if version.is_a?(Gem::Version) | ||
version | ||
elsif version.nil? | ||
latest_version | ||
else | ||
Gem::Version.new(version.to_s) | ||
end | ||
ver = if version.is_a?(Gem::Version) | ||
version | ||
elsif version.nil? | ||
latest_version | ||
else | ||
Gem::Version.new(version.to_s) | ||
end | ||
|
||
Webdrivers.logger.debug "Desired version: #{ver}" | ||
ver | ||
end | ||
|
||
def latest_version | ||
raise StandardError, 'Can not reach site' unless site_available? | ||
unless site_available? | ||
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. Let's swap this around; put this at the beginning:
That'll let you remove the the unless block. |
||
cur_ver = current_version | ||
raise StandardError, update_failed_msg if cur_ver.nil? # Site is down and no existing binary | ||
|
||
Webdrivers.logger.error "Can not reach update site. Using existing #{file_name} #{cur_ver}" | ||
return cur_ver | ||
end | ||
|
||
downloads.keys.max | ||
end | ||
|
@@ -51,9 +63,10 @@ def remove | |
end | ||
|
||
def download | ||
raise StandardError, 'Can not reach site' unless site_available? | ||
raise StandardError, update_failed_msg unless site_available? | ||
|
||
url = downloads[desired_version] | ||
Webdrivers.logger.debug "Downloading #{url}" | ||
filename = File.basename url | ||
|
||
FileUtils.mkdir_p(install_dir) unless File.exist?(install_dir) | ||
|
@@ -91,13 +104,31 @@ def binary | |
File.join install_dir, file_name | ||
end | ||
|
||
# | ||
# Returns count of network request made to the base url. Used for debugging | ||
# purpose only. | ||
# | ||
def network_requests | ||
@network_requests || 0 | ||
end | ||
|
||
# | ||
# Resets network request count to 0. | ||
# | ||
def reset_network_requests | ||
@network_requests = 0 | ||
end | ||
|
||
protected | ||
|
||
def get(url, limit = 10) | ||
raise StandardError, 'Too many HTTP redirects' if limit.zero? | ||
|
||
@network_requests ||= 0 | ||
response = http.get_response(URI(url)) | ||
Webdrivers.logger.debug "Get response: #{response.inspect}" | ||
@network_requests += 1 | ||
Webdrivers.logger.debug "Successful network request ##{@network_requests}" | ||
|
||
case response | ||
when Net::HTTPSuccess | ||
|
@@ -200,6 +231,11 @@ def correct_binary? | |
def normalize(string) | ||
Gem::Version.new(string) | ||
end | ||
|
||
def update_failed_msg | ||
"Update site is unreachable. Try downloading '#{file_name}' manually from " \ | ||
"#{base_url} and place in '#{install_dir}'" | ||
end | ||
end | ||
end | ||
end |
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.
Just a thought, if we could add a configuration option to disable all network calls, then we could make an early
return current_version
before this line, if the option is set.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'm planning on doing that very soon.