Skip to content
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

Add support for WD_CHROME_PATH. #138

Merged
merged 3 commits into from
Jun 20, 2019

Conversation

kapoorlakshya
Copy link
Collaborator

@kapoorlakshya kapoorlakshya commented Jun 17, 2019

Other changes:

  • WD_INSTALL_DIR now has the highest precedence when determining the driver install (download) location. This is to make the behavior consistent with WD_CACHE_TIME and WD_CHROME_PATH.
  • Removed ENV handing logic from the rake tasks. They now rely on the core code to handle these.

UPDATE:

  • Fix bug where ENV vars took precedence over user defined configs in their project. The new order is:
  1. User defined config
  2. User defined ENV var value
  3. Default value

@twalpole
Copy link
Collaborator

twalpole commented Jun 17, 2019

As implemented ENV['WD_CHROME_PATH'] is preferred over Selenium::WebDriver::Chrome.path (and the equivalents for other drivers)- I think that should probably be the other way around since code setting is more deliberate than environment variable.

@kapoorlakshya
Copy link
Collaborator Author

kapoorlakshya commented Jun 17, 2019

As discussed on Slack, the current behavior of ENV taking precedence over code is now categorized as a bug. I'll update the PR tonight with the new order:

  1. User defined config
  2. User defined ENV var
  3. Default value

This way any deliberate changes in the code will take precedence over ENV values.

@kapoorlakshya kapoorlakshya force-pushed the 137_wd_chrome_binary branch from 1c6c0a6 to aa4ed8e Compare June 18, 2019 03:10
@kapoorlakshya
Copy link
Collaborator Author

@twalpole PR is ready for second review.

@kapoorlakshya kapoorlakshya force-pushed the 137_wd_chrome_binary branch 9 times, most recently from 18f986c to 50366ea Compare June 19, 2019 18:28
Copy link
Collaborator

@twalpole twalpole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- other than possibly adding location method to finder classes

spec/webdrivers/chrome_finder_spec.rb Outdated Show resolved Hide resolved
spec/webdrivers/chrome_finder_spec.rb Outdated Show resolved Hide resolved
@kapoorlakshya kapoorlakshya force-pushed the 137_wd_chrome_binary branch from 50366ea to ba7f65d Compare June 20, 2019 17:45
@twalpole
Copy link
Collaborator

That looks better - 👍

@kapoorlakshya kapoorlakshya merged commit a545f1c into titusfortner:master Jun 20, 2019
@kapoorlakshya kapoorlakshya deleted the 137_wd_chrome_binary branch June 25, 2019 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants