Use load_selenium class method for requiring selenium-webdriver.
#2013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change relates to this issue: #2011 `Capybara::Selenium::Driver#load_selenium` is bumped to a class method which is called either during `Selenium::Driver` instantiation or before Selenium is used. Tests have also been updated to remove now unnecessary requires of `selenium-webdriver`. This should make `Selenium::Driver` behave more like `RackTest::Driver`. It also has the benefit of making sure the misnamed Selenium errors are patched ~everywhere. This change passes local tests for me that were run with: `bundle exec rake chrome_spec` `CI=true bundle exec rake travis` All in all, I'm not sure the lazy-loading is worthwhile (though that's, of course, a choice for the maintainers). A small aside, that there's also the use of the `webdrivers` gem which is conditional on the `CI` environment variable. This means that CI behaves differently to other environments in a way that's perhaps not expected (though the failure case looks to be that tests would succeed in CI but fail locally, which seems better than the opposite). Maintainers, if this isn't the direction you want to go feel free to close the pull request without merging.
|
Maintainers, if this isn't the direction you want to go feel free to close the pull request without merging. Let me know if there's anything I can do to help and thanks for Capybara! |
twalpole
reviewed
Apr 16, 2018
lib/capybara/selenium/driver.rb
Outdated
| @@ -38,7 +55,7 @@ def browser | |||
| end | |||
|
|
|||
| def initialize(app, **options) | |||
| load_selenium | |||
| Capybara::Selenium::Driver.load_selenium | |||
twalpole
reviewed
Apr 16, 2018
spec/selenium_spec_chrome.rb
Outdated
| @@ -1,7 +1,6 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'spec_helper' | |||
| require 'selenium-webdriver' | |||
There was a problem hiding this comment.
Please leave these - Sometimes we want to access things from Selenium before the driver is initialized
twalpole
reviewed
Apr 16, 2018
spec/selenium_spec_edge.rb
Outdated
| @@ -1,7 +1,6 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'spec_helper' | |||
| require "selenium-webdriver" | |||
twalpole
reviewed
Apr 16, 2018
spec/selenium_spec_ie.rb
Outdated
| require 'shared_selenium_session' | ||
| require 'rspec/shared_spec_matchers' | ||
|
|
||
| Capybara.register_driver :selenium_ie do |app| | ||
| # ::Selenium::WebDriver.logger.level = "debug" | ||
| Capybara::Selenium::Driver.load_selenium |
There was a problem hiding this comment.
This isn't necessary since the Capybara::Selenium::Driver initialization calls it
twalpole
reviewed
Apr 16, 2018
spec/shared_selenium_session.rb
Outdated
| @@ -1,7 +1,6 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'spec_helper' | |||
| require "selenium-webdriver" | |||
twalpole
reviewed
Apr 16, 2018
spec/selenium_spec_marionette.rb
Outdated
| @@ -1,10 +1,10 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'spec_helper' | |||
| require "selenium-webdriver" | |||
|
I commented on some things, basically this is fine but I don't really want the changes to the test files - just leave them how they were. |
I'm leaving the quotation marks as they were, though they don't match others, to keep the change history clean.
|
Updated -- thanks again for Capybara! |
|
Thanks for this, and you're welcome |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change relates to this issue:
#2011
Capybara::Selenium::Driver#load_seleniumis bumped to a class method which iscalled either during
Selenium::Driverinstantiation or before Selenium isused. Tests have also been updated to remove now unnecessary requires of
selenium-webdriver.This should make
Selenium::Driverbehave more likeRackTest::Driver. It alsohas the benefit of making sure the misnamed Selenium errors are patched
~everywhere.
This change passes local tests for me that were run with:
bundle exec rake chrome_specCI=true bundle exec rake travisAll in all, I'm not sure the lazy-loading is worthwhile (though that's, of
course, a choice for the maintainers).
A small aside, that there's also the use of the
webdriversgem which isconditional on the
CIenvironment variable. This means that CI behavesdifferently to other environments in a way that's perhaps not expected (though
the failure case looks to be that tests would succeed in CI but fail locally,
which seems better than the opposite).