Optional selenium webdriver #970

Closed
wants to merge 7 commits into
from

Projects

None yet

2 participants

morgoth commented Feb 22, 2013

References jnicklas#887

Tests are passing.
If you like it I can prepare another pull request with optional rack-test

@jnicklas jnicklas commented on the diff Feb 22, 2013
lib/capybara/selenium/driver.rb
@@ -1,4 +1,9 @@
-require 'selenium-webdriver'
+begin
+ require 'selenium-webdriver'
+rescue LoadError
jnicklas
jnicklas Feb 22, 2013 Collaborator

This really needs to check that the file it failed to load is 'selenium-webdriver', otherwise it will swallow load errors raised for other reasons. Also, once the exception is caught, why not just raise a new one, instead of doing exit 1?

morgoth
morgoth Feb 23, 2013

Not sure if regexp on error message is a good thing.
This will definitelly be catched when loading selenium-webdriver gem

jnicklas
jnicklas Feb 24, 2013 Collaborator

Yes, a regexp on the error message is a good thing. Otherwise this will mask load errors raised inside selenium-webdriver, which could lead to a lot of confusion.

@jnicklas jnicklas commented on an outdated diff Feb 22, 2013
lib/capybara/selenium.rb
@@ -0,0 +1,10 @@
+module Capybara
+ module Selenium
+ autoload :Node, 'capybara/selenium/node'
+ autoload :Driver,'capybara/selenium/driver'
+ end
+end
jnicklas
jnicklas Feb 22, 2013 Collaborator

Why is this change necessary? That just seems like it adds an extra step which is unneeded, having to require this file.

@jnicklas jnicklas commented on an outdated diff Feb 22, 2013
Gemfile
@@ -1,6 +1,5 @@
-source :rubygems
+source 'https://rubygems.org'
jnicklas
jnicklas Feb 22, 2013 Collaborator

Don't put unrelated stuff in your pull request, please.

morgoth commented Feb 23, 2013

I will squash commits to one, when all is good.

@jnicklas jnicklas and 1 other commented on an outdated diff Feb 24, 2013
lib/capybara/selenium/driver.rb
@@ -1,4 +1,9 @@
-require 'selenium-webdriver'
+begin
+ require 'selenium-webdriver'
+rescue LoadError
+ $stderr.puts "You don't have 'selenium-webdriver' gem installed. Please add it to your Gemfile and run bundle install"
jnicklas
jnicklas Feb 24, 2013 Collaborator

Still, why not just do a raise? Why $stderr.puts, that's just weird. Also, let's raise a LoadError, instead of a RuntimeError.

morgoth
morgoth Feb 24, 2013

We are reraising exception in next line, so it will be LoadError.
Do you want to raise custom error instead?

Collaborator

Sorry, I went ahead and implemented this, too much back and forth over a simple change. Thank you for getting the ball rolling on this!

@jnicklas jnicklas closed this Feb 25, 2013
morgoth commented Feb 25, 2013

@jnicklas no problem. Do you plan remove rack-test dependency as well?

Collaborator

I looked at it, but it seems a bit tricky. There are a couple of other gems, like mime-types which we only really need for the RackTest driver, should we then remove the dependency on those as well?

morgoth commented Feb 25, 2013

That would be consequent action, but I can imagine annoyed people.

Collaborator

Yeah, it feels fine for selenium, but let's leave racktest as is, for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment