titusfortner / webdrivers

Keep your Selenium WebDrivers updated automatically
MIT License
595 stars 110 forks source link

Add new `WD_DRIVER_PATH` environment variable #150

Closed ybiquitous closed 5 years ago

ybiquitous commented 5 years ago

Hi, thank you very much for the awesome gem!

I suggest a new feature to add a new WD_DRIVER_PATH environment variable and overwrite a driver path. Please let me explain the reason.

Currently, I use the gem to auto-download and auto-update ChromeDriver on my local machine and on my CircleCI environment. CircleCI has a Docker image which bundles Ruby and the latest ChromeDriver and I use the image.

I want to reduce the CI build time, so I want to use the bundled ChromeDriver on the CI. Furthermore, when running parallel tests on the CI, I want to prevent duplicate download.

Thus, by using this new feature I think that I can realize the CI speed-up.

What do you think about the proposal? It would be great if I could take good feedback. Thanks.

twalpole commented 5 years ago

If you want to use the already bundled Chromedriver - why are you using this gem?? This gem is specifically for managing the driver (chromedriver, geckodriver, etc), if you already have the driver installed you don't need this gem in the CI environment.

ybiquitous commented 5 years ago

Thank you for your quick reply.

Sorry, my explanation is not enough. This gem is so convenient because my team members do not need to think of a driver update. When running tests, the gem updates it automatically.

However, this gem is not necessary on the CI environment.

twalpole commented 5 years ago

So it seems like you could do something like

require 'webdrivers' unless ENV['CI']
ybiquitous commented 5 years ago

Thanks. Your suggestion looks good, but I need to modify also Gemfile in addition to rails_helper.rb:

-gem 'webdrivers'
+gem 'webdrivers', require: false
+require 'webdrivers' unless ENV['CI']

The solution needs to change 2 files, so I think it not DRY. 😓

ybiquitous commented 5 years ago

In addition, I don't want to increase ENV['CI'] logic in my code as possible.

twalpole commented 5 years ago

I don't think needing to change 2 files makes it not DRY - but... I'm 👎 on this change, but maybe @titusfortner thinks it's more useful. If he does it would have to at least change to something like WD_CHROME_DRIVER_PATH because there are people using multiple drivers in their test suites so you can't just override all of them to the same thing.

twalpole commented 5 years ago

Additionally, I don't think this would actually prevent downloading a new driver if there was a browser mismatch, but it would prevent the newly downloaded driver from being used - which would be potentially confusing.

Another option for you could be to set WD_INSTALL_DIR to the directory where chromedriver is already installed (or create a link from ~/.webdrivers/chromedriver to the installed chromedriver if you're using multiple drivers) - which then wouldn't download since it already exists.

ybiquitous commented 5 years ago

Thank you, I will try the another option!

Your thought about "potentially confusing" makes sense to me, surely.

ybiquitous commented 5 years ago

When I set the WD_INSTALL_DIR: /usr/local/bin environment variable into my CircleCI configuration file, the following error raises. 😢

Error: Permission denied @ rb_sysopen - /usr/local/bin/chromedriver.version

image

ybiquitous commented 5 years ago

Finally, I accepted the solution which works on my CI environment. https://github.com/titusfortner/webdrivers/pull/150#issuecomment-519999495

Thank you for your kindness! I close this unneeded PR.