-
Notifications
You must be signed in to change notification settings - Fork 111
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
Fixed Webdrivers::VersionError with chrome version greater than 115 #249
Conversation
9ef214c
to
de1dffc
Compare
Thanks for the PR. It looks like it isn't backwards compatible. Can you look at why the tests are failing? |
@titusfortner |
@titusfortner |
Just need to change from |
|
8438f32
to
4378b7d
Compare
StandardError is the superclass, it is the value of the String it failed on. Not sure if it is happenstance or common that MS doesn't do releases for all platforms for all patch versions, but not good 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.
- Thanks for putting this together and adding tests
- I really hate looking at old code I've written.
- We probably need to exclude
chromedriver.rb
fromMetrics/ClassLength
in.rubocop.yml
.
@@ -77,8 +81,14 @@ def latest_point_release(version) | |||
"#{msg} A network issue is preventing determination of latest chromedriver release." | |||
end | |||
|
|||
url = if version >= normalize_version('115') |
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.
We can use https://chromedriver.chromium.org/downloads/version-selection for the error message
@@ -149,6 +172,33 @@ def sufficient_binary? | |||
super && current_version && (current_version < normalize_version('70.0.3538') || | |||
current_build_version == browser_build_version) | |||
end | |||
|
|||
def chrome_for_testing_base_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.
Not sure if it matters, but this breaks https://github.com/titusfortner/webdrivers/wiki/Using-with-VCR-or-WebMock
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.
Do we need to support a version that we cannot get from this endpoint?
I believe we can change the base_url to the API endpoint if we don't need it.
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.
yes, we still need to support the older versions
@@ -115,7 +134,11 @@ def driver_filename(driver_version) | |||
elsif System.platform == 'linux' | |||
'linux64' | |||
elsif System.platform == 'mac' | |||
apple_filename(driver_version) | |||
if driver_version >= normalize_version('115') |
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.
If we move this conditional to #apple_filename
we don't need to add #apple_filename_api
.
@@ -66,7 +66,7 @@ | |||
allow(Net::HTTP).to receive(:get_response).and_raise(SocketError) | |||
allow(chromedriver).to receive(:exists?).and_return(false) | |||
|
|||
msg = %r{Can not reach https://chromedriver.storage.googleapis.com} | |||
msg = %r{Can not reach https://googlechromelabs.github.io/chrome-for-testing/latest-patch-versions-per-build.json} |
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.
How about considering the both cases for v115+ and v114 or earlier? The Gem is expected to print "Can not reach ...", and the target URL is not important here.
msg = %r{Can not reach https://googlechromelabs.github.io/chrome-for-testing/latest-patch-versions-per-build.json} | |
msg = %r{Can not reach (?:https://googlechromelabs.github.io|https://chromedriver.storage.googleapis.com)} |
@@ -101,7 +101,7 @@ | |||
it 'raises ConnectionError if offline' do | |||
allow(Net::HTTP).to receive(:get_response).and_raise(SocketError) | |||
|
|||
msg = %r{Can not reach https://chromedriver.storage.googleapis.com/} | |||
msg = %r{Can not reach https://googlechromelabs.github.io/chrome-for-testing/latest-patch-versions-per-build.json} |
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.
msg = %r{Can not reach https://googlechromelabs.github.io/chrome-for-testing/latest-patch-versions-per-build.json} | |
msg = %r{Can not reach (?:https://googlechromelabs.github.io|https://chromedriver.storage.googleapis.com)} |
@@ -223,11 +241,12 @@ | |||
it 'raises ConnectionError when offline' do | |||
allow(Net::HTTP).to receive(:get_response).and_raise(SocketError) | |||
|
|||
msg = %r{^Can not reach https://chromedriver.storage.googleapis.com} | |||
msg = %r{^Can not reach https://googlechromelabs.github.io/chrome-for-testing/latest-patch-versions-per-build.json} |
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.
msg = %r{^Can not reach https://googlechromelabs.github.io/chrome-for-testing/latest-patch-versions-per-build.json} | |
msg = %r{Can not reach (?:https://googlechromelabs.github.io|https://chromedriver.storage.googleapis.com)} |
Merged here — 97fb1e2 Thank you! |
similar Issue: #247
f the version is 115 or higher, the API can be used, so if the version is 115 or higher, the stable version is obtained and downloaded from the API.
refarence