Check if we already have the latest/desired version before checking for updates. #49

kapoorlakshya · 2019-03-27T21:59:50Z

Make 0 network calls if user already has desired or latest chromedriver.
Return currently downloaded version as latest version if update site is down. Also print a warning to the console to let the user we're using the existing driver version.
Wrap the update failure message in a method for re-usability and consistency.

Partially addresses #29.

jrochkind · 2019-03-28T01:46:01Z

Nice. Might it be a reasonable pattern, with this code, to download the pinned version of your drivers to a local dir in your project (Ie not "~/something" but "./something"), and then check it into your repo? To guarantee that (eg) CI will definitely not have to make any network requests.

If so, might make sense to document that.. and maybe provide some additional API to facilitate it. Like I know webdriver will download what it needs if it's not there when it needs it... but is there API to be like "Yeah, download it right now", to force the download, so you'd have it to check into your repo?

kapoorlakshya · 2019-03-28T04:27:37Z

Hi @jrochkind, you can define a custom download/storage location relative to your project root:

Webdrivers.install_dir = '../webdrivers'

This was added to the README in v3.7.1 (on 03/25).

but is there API to be like "Yeah, download it right now", to force the download, so you'd have it to check into your repo?

You mean like a shell command to download it without having to run your project (launching the browser)? Sorry, I need a little clarification on what you're suggesting.

titusfortner

There is a reason #site_available? was checked first.

Consider the situation where you have a working driver and aren't connected to the internet. This code will have #update call #correct_binary which will call #desired_version which will call #latest_version, which will raise an exception rather than just using the available local driver.

Take another look, and we can pair on this later if we need to.

kapoorlakshya · 2019-03-28T06:30:28Z

Yeah, good point.

To work around that I have updated #latest_version to check #current_version if site is unreachable. If current version is nil (no existing binary), it will raise an error (ask user to manually download) as there is nothing the gem can do now. Otherwise, it will return the current version as desired/latest and allow the user to use the existing binary. It will also log a warning to console to let the user know the update failed.

Let me know if I am missing any other cases. I'll take another look in the morning with a fresh mind.

jrochkind · 2019-03-28T12:47:43Z

You mean like a shell command to download it without having to run your project

No, I just mean, what do I call to force a download? Is there a Webdrivers.download_now, or a Webdrivers.download_now(to_this_location) or whatever. There may be already, if so mentioning it in the README would be helpful.

titusfortner · 2019-03-28T18:23:09Z

@jrochkind it's happening in Webdrivers::Chromedriver.update
so you can do:

Webdrivers::Chromedriver.version = '2.46'
Webdrivers::Chromedriver.update

kapoorlakshya · 2019-03-28T18:31:14Z

Ah, Titus beat me to it :)

You can call #update on all driver classes as shown here.

jrochkind · 2019-03-28T18:37:07Z

If this PR gets merged, I think there will be more demand for that, and would recommend a README section on it, or including mention of the update example in the docs for this feature, explaining use case of setting it's location and checking it into the repo to avoid any network calls on CI. Is what I was trying to suggest. :)

titusfortner · 2019-03-28T18:49:08Z

yeah, we're going to try to get everything cleaned up and put a 4.0 out there with everything. Not sure we'll make it by 3/31, but definitely in April.

…or updates.

titusfortner · 2019-04-08T19:24:05Z

lib/webdrivers/common.rb

+          return binary
+        end
+
+        # If site is down
        unless site_available?


We've already checked for site availability, no need to make another call to get the same info. Let's memoize this method.

titusfortner

Couple things. I think we're constrained by some more fundamental issues in the code now. Let's discuss on Slack.

titusfortner · 2019-04-08T19:26:06Z

lib/webdrivers/common.rb

      end

      def latest_version
-        raise StandardError, 'Can not reach site' unless site_available?
+        unless site_available?


Let's swap this around; put this at the beginning:

return downloads.keys.max if site_available?

That'll let you remove the the unless block.

titusfortner · 2019-04-08T19:33:49Z

lib/webdrivers/chromedriver.rb

+          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


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.

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.

titusfortner · 2019-04-08T19:40:53Z

lib/webdrivers/chromedriver.rb

@@ -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


Not as big of a deal since they aren't network calls, but some paths through this code are also calling #current_version multiple times in a single update. Let's memoize it and then set it back to nil in the #remove method.

januszm · 2019-04-24T17:06:45Z

WebMock::NetConnectNotAllowedError: Real HTTP connections are disabled. Unregistered request: GET https://chromedriver.storage.googleapis.com/ led me to this Pull Request.

I was hoping that this code (for TravisCI):

before_script:
  - bundle exec rails runner "Webdrivers::Chromedriver.update"

would solve issues with getting the correct chromedriver versions after Chrome 73 and 74 releases. But people/teams who block all network connections in tests have to deal with:

def update
  unless site_available?

code.

I would like to confirm one thing about the proposed change: assuming that Google Chrome Stable is installed in the system during CI build (e.g., in TravisCI via addons: chrome: stable) and Webdrivers::Chromedriver.update is executed before automated tests are started - no HTTP request will be needed anymore, right? I'd like to make sure that 0 network calls are required when automated tests run.
What would you say about adding a configuration option disable_latest_version_check or similar (disable network calls) that, if set, would cause early return (with the version of the existing binary) from the latest_version method?

januszm · 2019-04-24T17:13:10Z

lib/webdrivers/chromedriver.rb

@@ -16,7 +16,13 @@ def current_version
      end

      def latest_version
-        raise StandardError, 'Can not reach site' unless site_available?
+        unless site_available?


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.

Yeah, I'm planning on doing that very soon.

kapoorlakshya · 2019-04-24T20:27:39Z

WebMock::NetConnectNotAllowedError: Real HTTP connections are disabled. Unregistered request: GET https://chromedriver.storage.googleapis.com/ led me to this Pull Request.

@januszm Check if this user suggested workaround solves the problem on Travis: https://github.com/titusfortner/webdrivers/wiki/Using-with-VCR-or-WebMock

januszm · 2019-04-24T20:35:10Z

@januszm Check if this user suggested workaround solves the problem on Travis: https://github.com/titusfortner/webdrivers/wiki/Using-with-VCR-or-WebMock

Yes it solves the problem, I used that method.

After giving some more thought to it, I think that any network calls should be blocked by default. Perhaps it'd be better to only make network call after manually calling the 'update' method. I'm referring to The Principle of Least Surprise. This may be surprising to many people that the gem secretly performs network connections while automated tests are running.

kapoorlakshya · 2019-04-24T20:46:41Z

After giving some more thought to it, I think that any network calls should be blocked by default.

My thought is to give the user the power to choose between automated (forced) and on-demand update checks.

The reason the current code always perform an update check is because Selenium users were constantly running into errors when their browser would update, but the driver would still be at the older version. The forced update makes sure you always have a compatible driver for your browser.

Now that this gem has more users (because of rails 6), there are a lot of new use cases which we will try to cover in v4 of this gem.

kapoorlakshya · 2019-04-27T07:25:31Z

@januszm Please see if #76 is along the lines of what you were envisioning.

I've decided to break this PR down into smaller changes which will be easier to implement and review.

kapoorlakshya · 2019-04-29T21:31:01Z

Closing for same reason as #76.

kapoorlakshya requested a review from titusfortner March 27, 2019 22:00

titusfortner requested changes Mar 28, 2019

View reviewed changes

kapoorlakshya force-pushed the limit_network_call_count branch from 7ad8456 to c78bcaa Compare March 28, 2019 06:36

kapoorlakshya force-pushed the limit_network_call_count branch 3 times, most recently from 8f3e3ff to 437b01f Compare April 1, 2019 01:13

Check if we already have the latest/desired version before checking f…

5325ecb

…or updates.

kapoorlakshya force-pushed the limit_network_call_count branch from 437b01f to 5325ecb Compare April 1, 2019 01:15

kapoorlakshya added 2 commits March 31, 2019 22:34

Print update failure message to ERROR log.

72d5d07

Remove binary before running offline tests for Chromedriver.

aba1bff

titusfortner reviewed Apr 8, 2019

View reviewed changes

titusfortner requested changes Apr 8, 2019

View reviewed changes

januszm reviewed Apr 24, 2019

View reviewed changes

kapoorlakshya mentioned this pull request Apr 29, 2019

Add config to disable automatic updates. #76

Closed

kapoorlakshya closed this Apr 29, 2019

kapoorlakshya deleted the limit_network_call_count branch April 29, 2019 21:31

kapoorlakshya restored the limit_network_call_count branch April 29, 2019 21:31

kapoorlakshya deleted the limit_network_call_count branch April 29, 2019 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if we already have the latest/desired version before checking for updates. #49

Check if we already have the latest/desired version before checking for updates. #49

kapoorlakshya commented Mar 27, 2019 •

edited

Loading

jrochkind commented Mar 28, 2019

kapoorlakshya commented Mar 28, 2019

titusfortner left a comment •

edited

Loading

kapoorlakshya commented Mar 28, 2019

jrochkind commented Mar 28, 2019

titusfortner commented Mar 28, 2019 •

edited

Loading

kapoorlakshya commented Mar 28, 2019

jrochkind commented Mar 28, 2019

titusfortner commented Mar 28, 2019

titusfortner Apr 8, 2019

titusfortner left a comment

titusfortner Apr 8, 2019

titusfortner Apr 8, 2019

kapoorlakshya Apr 24, 2019

titusfortner Apr 8, 2019

januszm commented Apr 24, 2019

januszm Apr 24, 2019

kapoorlakshya Apr 24, 2019

kapoorlakshya commented Apr 24, 2019

januszm commented Apr 24, 2019

kapoorlakshya commented Apr 24, 2019

kapoorlakshya commented Apr 27, 2019

kapoorlakshya commented Apr 29, 2019

Check if we already have the latest/desired version before checking for updates. #49

Check if we already have the latest/desired version before checking for updates. #49

Conversation

kapoorlakshya commented Mar 27, 2019 • edited Loading

jrochkind commented Mar 28, 2019

kapoorlakshya commented Mar 28, 2019

titusfortner left a comment • edited Loading

Choose a reason for hiding this comment

kapoorlakshya commented Mar 28, 2019

jrochkind commented Mar 28, 2019

titusfortner commented Mar 28, 2019 • edited Loading

kapoorlakshya commented Mar 28, 2019

jrochkind commented Mar 28, 2019

titusfortner commented Mar 28, 2019

titusfortner Apr 8, 2019

Choose a reason for hiding this comment

titusfortner left a comment

Choose a reason for hiding this comment

titusfortner Apr 8, 2019

Choose a reason for hiding this comment

titusfortner Apr 8, 2019

Choose a reason for hiding this comment

kapoorlakshya Apr 24, 2019

Choose a reason for hiding this comment

titusfortner Apr 8, 2019

Choose a reason for hiding this comment

januszm commented Apr 24, 2019

januszm Apr 24, 2019

Choose a reason for hiding this comment

kapoorlakshya Apr 24, 2019

Choose a reason for hiding this comment

kapoorlakshya commented Apr 24, 2019

januszm commented Apr 24, 2019

kapoorlakshya commented Apr 24, 2019

kapoorlakshya commented Apr 27, 2019

kapoorlakshya commented Apr 29, 2019

kapoorlakshya commented Mar 27, 2019 •

edited

Loading

titusfortner left a comment •

edited

Loading

titusfortner commented Mar 28, 2019 •

edited

Loading